-
Notifications
You must be signed in to change notification settings - Fork 847
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
Scope config #6375
Scope config #6375
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6375 +/- ##
============================================
+ Coverage 91.08% 91.11% +0.03%
- Complexity 5774 5825 +51
============================================
Files 627 633 +6
Lines 16855 16948 +93
Branches 1721 1728 +7
============================================
+ Hits 15352 15442 +90
- Misses 1007 1009 +2
- Partials 496 497 +1 ☔ 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.
Thank you 🙏
* | ||
* @see LoggerConfig#configuratorBuilder() | ||
*/ | ||
public SdkLoggerProviderBuilder setLoggerConfigurator( |
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.
These ScopeConfigurator
setters are great!
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.
Yeah this is great. I had a few ideas that I think are worth considering, and I'd really like to see the glob tests beefed up slightly, but otherwise looking cool to me. 😎
sdk/common/src/main/java/io/opentelemetry/sdk/common/ScopeConfiguratorBuilder.java
Outdated
Show resolved
Hide resolved
sdk/common/src/test/java/io/opentelemetry/sdk/internal/GlobUtilTest.java
Show resolved
Hide resolved
sdk/all/src/test/java/io/opentelemetry/sdk/ScopeConfiguratorTest.java
Outdated
Show resolved
Hide resolved
sdk/all/src/test/java/io/opentelemetry/sdk/ScopeConfiguratorTest.java
Outdated
Show resolved
Hide resolved
sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLogger.java
Outdated
Show resolved
Hide resolved
sdk/logs/src/main/java/io/opentelemetry/sdk/logs/SdkLoggerProvider.java
Outdated
Show resolved
Hide resolved
sdk/logs/src/main/java/io/opentelemetry/sdk/logs/LoggerSharedState.java
Outdated
Show resolved
Hide resolved
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.
Nice!
Thanks for the review @breedx-splk / @LikeTheSalad! Now that its received some eyes, I've went ahead and pushed a commit to move all the public APIs to internal packages using patterns we've established accessing experimental APIs. Should be good to go ahead with this now! |
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.
Seems good to me. 🚢
Implements spec PR: open-telemetry/opentelemetry-specification#3877
Replaces prototype PR #6201. Fixes #6197.
To simplify reviewing, I've added all the APIs to the public API surface area. Will plan on moving them to internal packages before merging, but don't want to muddy the waters with reflective access.
For examples of usage see:
ScopeConfiguratorTest
LoggerConfigTest
TracerConfigTest
MeterConfigTest
Simple demonstration. The API in this is aspirational - we won't be able to achieve it until the spec is stabilized. For example of the actual API, see ScopeConfiguratorTest.