-
Notifications
You must be signed in to change notification settings - Fork 936
feat: always return ExtendedOpenTelemetry when incubator is available #7991
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7991 +/- ##
============================================
- Coverage 90.17% 90.14% -0.04%
- Complexity 7478 7486 +8
============================================
Files 836 838 +2
Lines 22550 22592 +42
Branches 2224 2226 +2
============================================
+ Hits 20335 20365 +30
- Misses 1513 1524 +11
- Partials 702 703 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| * | ||
| * @return the no-op {@link SdkConfigProvider} | ||
| */ | ||
| public static ConfigProvider noop() { |
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.
Unused and a duplicate of ConfigProvider#noop()
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.
Called from https://github.com/zeitlinger/opentelemetry-java/blob/0db77eb839b0af1e7f32b0f7ced29e60edcce005/sdk/all/src/main/java/io/opentelemetry/sdk/IncubatingUtil.java#L26 to get an instance for ExtendedOpenTelemetrySdk.
I tried to make it return SdkConfigProvider - but then a design test failed.
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.
Called from https://github.com/zeitlinger/opentelemetry-java/blob/0db77eb839b0af1e7f32b0f7ced29e60edcce005/sdk/all/src/main/java/io/opentelemetry/sdk/IncubatingUtil.java#L26 to get an instance for ExtendedOpenTelemetrySdk.
I tried to make it return SdkConfigProvider - but then a design test failed.
| LogLimits.builder() | ||
| .setMaxAttributeValueLength(1) | ||
| .setMaxNumberOfAttributes(2) | ||
| OpenTelemetrySdk expectedSdk = |
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.
This change isn't strictly necessary is it since the test code could still call ExtendedOpenTelemetrySdk.create(..)? Not suggesting reverting - just trying to confirm my understanding.
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.
see #7991 (comment)
|
IIUC, the benefit for this PR for callers is that now whenever |
Yes that has benefits
|
|
I think we can go even further to align I think long term, we end up with a |
this is the behavior we have for other extended classes and a first step towards #7961