[SDK] Fix include instrumentation scope attributes in equal method#3214
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
|
Co-authored-by: Marc Alff <marc.alff@free.fr>
Co-authored-by: Marc Alff <marc.alff@free.fr>
|
CI has passed. Ready for review. |
dbarker
changed the title
| 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) |
There was a problem hiding this comment.
Do you think we can use this->find(kv.first) here for better performance?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks for the details. Looks ok to me as is.
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix.
Very well done, and with good test coverage.
…tation_scope_attributes_in_equal_method
|
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
Done. CI passed. |
dbarker
deleted the
fix_include_instrumentation_scope_attributes_in_equal_method
branch
Updates the C++ spec compliance matrix.
## Changes
Update the C++ matrix to show support for the following features:
1. Associate Tracer with InstrumentationScope
- open-telemetry/opentelemetry-cpp#3214
1. `Gauge` instrument is supported
- open-telemetry/opentelemetry-cpp#3029
1. Exponential Histogram Aggregation
- open-telemetry/opentelemetry-cpp#3346
1. A valid instrument must be created when given the same name and
duplicates result in the first seen instrument
- open-telemetry/opentelemetry-cpp#3358
For non-trivial changes, follow the [change proposal
process](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CONTRIBUTING.md#proposing-a-change).
* [ ] Related issues #
* [ ] Related [OTEP(s)](https://github.com/open-telemetry/oteps) #
* [ ] Links to the prototypes (when adding or changing features)
* [ ]
[`CHANGELOG.md`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/CHANGELOG.md)
file updated for non-trivial changes
* For trivial changes, include `[chore]` in the PR title to skip the
changelog check
* [x] [Spec compliance
matrix](https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix/template.yaml)
updated if necessary
3 participants
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.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes