-
Notifications
You must be signed in to change notification settings - Fork 431
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
Add support for instrumentation scope attributes #1021
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1021 +/- ##
=====================================
Coverage 56.4% 56.4%
=====================================
Files 144 144
Lines 17717 17752 +35
=====================================
+ Hits 9997 10017 +20
- Misses 7720 7735 +15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
How does codecov work? Does it run on documentation testing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Could you add an entry to changelog file as well?
https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-api/CHANGELOG.md
I haven't seen any other PRs add entries to the CHANGELOG. @TommyCpp is there a practice for this? Do we populate the CHANGELOG all at once during release or per relevant PR? |
Irrespective of what was done in other PRs, I think all PRs affecting end users should include an entry in changelog as a good practice for any product. |
We do have a CHANGELOG for every create. But so far the practice has been one of us collecting all PRs before a release and add them to CHANGELOG |
Personally I'd love to require CHANGELOG entry for every PR. Makes release easier |
I agree. Adding a PR template to encourage changelogs : #1036 |
@TommyCpp |
Will take a look on remaining PRs tonight |
Fixes #815
Note that according to the specs the uniqueness of an
InstrumentationScope
does not include the attributes.I am new to Rust and OT-Rust so am ramping up. Please advise if
Option<Vec<KeyValue>>
is not the correct typing and there's some Rust-specific nuances related to memory management that I have overlooked :)