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

add SdkTracerProvider.setScopeConfigurator() and support #7021

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jackshirazi
Copy link
Contributor

per discussion here from PR

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.89%. Comparing base (c8da020) to head (8e4eebf).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
...etry/sdk/trace/internal/SdkTracerProviderUtil.java 75.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7021      +/-   ##
============================================
- Coverage     89.95%   89.89%   -0.06%     
- Complexity     6636     6660      +24     
============================================
  Files           745      748       +3     
  Lines         20010    20095      +85     
  Branches       1962     1969       +7     
============================================
+ Hits          17999    18064      +65     
- Misses         1415     1437      +22     
+ Partials        596      594       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jackshirazi
Copy link
Contributor Author

@open-telemetry/java-approvers any idea why the graal build is failing?

@trask
Copy link
Member

trask commented Jan 21, 2025

looks like it was a sporadic issue, @laurit re-ran and it's passing now

@@ -100,6 +102,17 @@ public Sampler getSampler() {
return sharedState.getSampler();
}

// currently not public as experimental
Copy link
Member

Choose a reason for hiding this comment

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

The pattern for giving access to these experimental features has been via Sdk{Signal}ProviderUtil, a public class in an internal package, with static helper methods for reflectively accessing package private methods.

For tracing, this is SdkTracerProviderUtil.

I'd recommend adding an accessor method there as part of this PR. If you do, please add javadoc to this pointing to that method.

If you choose to punt on that, let's add a TODO to followup:

Suggested change
// currently not public as experimental
// TODO: add public experimental accessor in SdkTracerProviderUtil

Speaking of followups, after landing this, should open an issue to track extending metrics and logs with the same capability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the static helper, and opened #7038 for extending to metrics and logs

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.

3 participants