Skip to content
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

[SDK] Fix include instrumentation scope attributes in equal method #3214

Conversation

dbarker
Copy link
Contributor

@dbarker dbarker commented Dec 17, 2024

Fixes #3208

Changes

This PR updates the GetTracer, GetMeter, and GetLogger methods to include instrumentation scope attributes in the equality check before returning a cached instance.

  • Adds AttributeEqualToVisitor to sdk/common/attribute_utils.h that compares an OwnedAttributeValue to a AttributeValue
  • Adds EqualTo method to the AttributeMap class to support checking equality with a KeyValueIterable
  • Updates the InstrumenationScope::equal method to accept attributes and evaluate them
  • Updates tests for Get<Logger,Tracer,Meter> methods to check for instance uniqueness based on the OTEL spec.
  • Updates tests for InstrumentationScope and AttributeMap to cover the equal and EqualTo methods

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 Dec 17, 2024

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

Name Link
🔨 Latest commit 5ae1b09
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/67630b1f3a301300088b21f4

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.16%. Comparing base (0b94d71) to head (5ae1b09).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3214      +/-   ##
==========================================
+ Coverage   87.82%   88.16%   +0.35%     
==========================================
  Files         195      198       +3     
  Lines        6154     6224      +70     
==========================================
+ Hits         5404     5487      +83     
+ Misses        750      737      -13     
Files with missing lines Coverage Δ
...include/opentelemetry/sdk/common/attribute_utils.h 93.69% <100.00%> (+2.78%) ⬆️
...y/sdk/instrumentationscope/instrumentation_scope.h 100.00% <100.00%> (ø)
sdk/src/logs/logger_provider.cc 90.91% <100.00%> (ø)
sdk/src/metrics/meter_provider.cc 87.18% <100.00%> (ø)
sdk/src/trace/tracer_provider.cc 89.37% <100.00%> (ø)

... and 10 files with indirect coverage changes

dbarker and others added 3 commits December 17, 2024 17:13
Co-authored-by: Marc Alff <marc.alff@free.fr>
Co-authored-by: Marc Alff <marc.alff@free.fr>
@dbarker dbarker marked this pull request as ready for review December 18, 2024 00:50
@dbarker dbarker requested a review from a team as a code owner December 18, 2024 00:50
@dbarker
Copy link
Contributor Author

dbarker commented Dec 18, 2024

CI has passed. Ready for review.

@dbarker dbarker changed the title Fix include instrumentation scope attributes in equal method [SDK] Fix include instrumentation scope attributes in equal method Dec 18, 2024
const opentelemetry::common::AttributeValue &value) noexcept {
// Perform a linear search to find the key assuming the map is small
// This avoids temporary string creation from this->find(std::string(key))
for (const auto &kv : *this)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can use this->find(kv.first) here for better performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems we have to start the search from the KeyValueIterable::ForEachKeyValue method and it provides a nostd::string_view key while the key type of the map is std::string. So we either have to iterate directly like this or create a temp string every time as in auto iter = this->find(std::string(key)). I chose the first option since it I've heard it could be faster than find for small maps :) and I didn't feel like string allocation (and its potential to throw) was ideal. I could profile both options if it seems worth it.

C++20 gives the "Heterogeneous comparison lookup" version of the std::unordered_map::find method of unordered_map which is really what we need here. Here's a neat example https://godbolt.org/z/Yqsx4rsY1

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the details. Looks ok to me as is.

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

Very well done, and with good test coverage.

@dbarker
Copy link
Contributor Author

dbarker commented Dec 18, 2024

I realized i missed a comment and update to operator== of the InstrumentationScope. Will push a commit shortly.

…erator to check attributes. add a test and minor fixes/comments to the tests
@dbarker
Copy link
Contributor Author

dbarker commented Dec 18, 2024

I realized i missed a comment and update to operator== of the InstrumentationScope. Will push a commit shortly.

Done. CI passed.

@marcalff marcalff merged commit 92bf8da into open-telemetry:main Dec 18, 2024
57 checks passed
@dbarker dbarker deleted the fix_include_instrumentation_scope_attributes_in_equal_method branch December 19, 2024 01:27
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.

Providers do not check instrumentation scope attributes when returning cached loggers, tracers, or meters
3 participants