-
Notifications
You must be signed in to change notification settings - Fork 494
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
Calls to with_resource
for signal builders (Metrics, Logs, Traces) are now additive
#2677
Calls to with_resource
for signal builders (Metrics, Logs, Traces) are now additive
#2677
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2677 +/- ##
=======================================
+ Coverage 79.1% 79.2% +0.1%
=======================================
Files 122 122
Lines 22530 22621 +91
=======================================
+ Hits 17839 17934 +95
+ Misses 4691 4687 -4 ☔ View full report in Codecov by Sentry. |
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.
I agree with this change. Requesting changes to address below.
- [Feature]: add ability for with_resource to merge Resources on providers #2324 (comment) - Check this and see if this makes sense, given there are more than one ideas suggested in the linked issue.
- Please make the change consistent across signals. (Okay to do that in separate PR to keep scope smaller)
2fa8aa0
to
cfc37e6
Compare
MeterProviderBuilder::with_resource
are now additivewith_resource
for signal builders (Metrics, Logs, Traces) are now additive
@martintmk Edited the PR Desc slightly to mark the associated issue to be resolved with this PR. |
Fixes #2324
Changes
The calls to:
MeterProviderBuilder::with_resource
TracerProviderBuilder::with_resource
LoggerProviderBuilder::with_resource
Are now additive and the previous resource is not discarded anymore. Additionally, I fixed a bug where setting the
schema_url
was discarded due to a bug how merging of resources was handled.Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes