Skip to content

[SDK] support aggregation of identical instruments #3358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

dbarker
Copy link
Contributor

@dbarker dbarker commented Apr 9, 2025

Fixes #3356

Why?
The spec requires identical instruments to be aggregated and duplicate instruments to be detected and exported.

  • Instruments are identical if all identifying fields equal: case-insensitive name, kind, unit, description
  • Instruments are duplicates if the case-insensitive names match but another field is different.

Current behavior:

  • Instrument names are treated as case sensitive and the only identifier used to track instruments by meter.
  • Identical instruments are not aggregated.
    • If a user creates another instrument with the same name then the first instrument's data is no longer collected/exported.
  • Duplicate instruments are not detected and reported to the user

New behavior:

  • Instruments are identified based on all identifying fields.
  • Names are treated as case-insensitive per the spec and name case conflicts result in the first-seen instrument by name being used when all other fields equal.
  • Identical instruments are aggregated
  • Duplicate instruments are detected and reported to the user. Duplicates are functional instruments (will export data) per the spec.

Changes

  • checks if an instrument already has a storage object in the meter's registry before creating a new one.
  • add tests for the spec requirements on aggregation of identical instruments
  • add tests and log messages following the spec on Duplicate instrument registration.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

Copy link

netlify bot commented Apr 9, 2025

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit 53b98c5
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/681d8fef6b798100086fa4b8

@dbarker dbarker changed the title use the existing storage for sync or async instruments of the same na… [SDK] support aggregation of identical instruments Apr 9, 2025
…ate storage registry to use the hash and equality structs. Add tests.
Copy link

codecov bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 85.36585% with 18 lines in your changes missing coverage. Please review.

Project coverage is 90.05%. Comparing base (4e4d8de) to head (53b98c5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...dk/include/opentelemetry/sdk/metrics/instruments.h 75.76% 16 Missing ⚠️
sdk/src/metrics/meter.cc 96.50% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3358      +/-   ##
==========================================
+ Coverage   89.98%   90.05%   +0.08%     
==========================================
  Files         211      212       +1     
  Lines        6812     6932     +120     
==========================================
+ Hits         6129     6242     +113     
- Misses        683      690       +7     
Files with missing lines Coverage Δ
sdk/include/opentelemetry/sdk/metrics/meter.h 57.15% <ø> (ø)
sdk/src/metrics/meter.cc 85.89% <96.50%> (+3.39%) ⬆️
...dk/include/opentelemetry/sdk/metrics/instruments.h 75.76% <75.76%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dbarker added 9 commits April 10, 2025 13:56
…ak the ostream header and keep the meter instrument creation warning implementation in the same file
…umentDescriptorUtils struct. Add log streamable wrappers for scopes and instrument descriptors. Add tests for correcitve views for name and description duplicates
@dbarker dbarker marked this pull request as ready for review April 15, 2025 02:53
@dbarker dbarker requested a review from a team as a code owner April 15, 2025 02:53
@ThomsonTan ThomsonTan added pr:fix-merge-conflicts Please fix merge conflicts for this pr and removed pr:fix-merge-conflicts Please fix merge conflicts for this pr labels Apr 30, 2025
@dbarker
Copy link
Contributor Author

dbarker commented May 6, 2025

Hi @lalitb, Could you take a look at this PR and share your feedback? Thanks!

@lalitb
Copy link
Member

lalitb commented May 6, 2025

Hi @lalitb, Could you take a look at this PR and share your feedback? Thanks!

Apologies for the delay—will review and share feedback in the next couple of days.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbarker Thank you for your PR. This is very nicely done, thoroughly covers all test scenarios :)

@lalitb lalitb enabled auto-merge (squash) May 9, 2025 05:19
@lalitb lalitb merged commit 92dd28c into open-telemetry:main May 9, 2025
66 checks passed
malkia added a commit to malkia/opentelemetry-cpp that referenced this pull request May 9, 2025
[SDK] support aggregation of identical instruments (open-telemetry#3358)
@dbarker dbarker deleted the fix_aggregate_identical_instruments branch May 9, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

identical instruments are not aggregated by the SDK
4 participants