-
Notifications
You must be signed in to change notification settings - Fork 811
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
[Api] support for adding default tags to tracer #6137
[Api] support for adding default tags to tracer #6137
Conversation
@alimahboubi, it is good practice to execute build locally. In this case it should be done in Could you please fix it? |
- update "PublicAPI.Shipped" file
@Kielek Thanks for pointing that out! I've fixed the compilation error. I've also run a local build in Release mode to confirm |
…dding-default-tags-to-tracer
- Moved new GetTracer overload to the Unshipped file. - Updated GetTracer signature to include nullable `version` and `tags` parameters. - Removed the default parameter value from the previous line (existing GetTracer overload) as it was redundant after adding the new overload.
@Kielek Thanks for the detailed feedback! I've incorporated both of your suggestions:
I've updated the commit with these changes. Please take another look! |
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.
Your assertions are to weak. The goal is to have one instance per tags set. Now, you have multiple instances for the same set of tags.
If you include all changes, tests will be failing. You need to fix the TracerKey.
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.
Worth to add entry in CHANGELOG under OpenTelemetry.Api
…dding-default-tags-to-tracer # Conflicts: # src/OpenTelemetry.Api/CHANGELOG.md
…lt-tags-to-tracer' into feature/Support-for-adding-default-tags-to-tracer
Dear @alanwest and @rajkumar-rangaraj, |
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.
Overall looks good.
…dding-default-tags-to-tracer
… value and add unit tests for tag comparison
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6137 +/- ##
==========================================
+ Coverage 86.23% 86.31% +0.08%
==========================================
Files 259 259
Lines 11776 11845 +69
==========================================
+ Hits 10155 10224 +69
Misses 1621 1621
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
@alimahboubi Please resolve the conflicts. |
…dding-default-tags-to-tracer
|
Fixes #6016
Design discussion issue #6016
Changes
This pull request adds a new overload to the
TracerProvider.GetTracer
method to support the inclusion of tags, similar to the newActivitySource
constructor overload in .NET 9. This allows users to associate initial tags with aTracer
(and therefore the underlyingActivitySource
), which can be used to differentiate application instances with different configurations. These tags are applied to all activities created by theActivitySource
.The new overload is: