-
Notifications
You must be signed in to change notification settings - Fork 906
Extended opentelemetry #7496
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
Extended opentelemetry #7496
Conversation
- Create Entity/EntityBuilder class in internal package - Update serialization code Need to discuss how to work with Resource going forward.
- Remove ResourceWithEntity, no way to keep bincompat that way - Update toString tests to be a lot more forgiving, but preserve intent
- fix reflective method lookup - also add unit test in the right project.
- Creates an API matching EntityProvider OTEP - Updates EntityDetector to be ResourceDetector - Creates synchronous "update resource with this entity" method. - Adds wiring for incubating API + incubating SDK Note: Still have some thoughts and ideas about a formal API, this may still change.
- Remove `Entity` and only expose `EntityBuilder` similar to how we do `LogRecord`s. - Have methods on `Resource` which determine if you're attaching the entity, simplify "remove" or just ignore for now.
…OpenTelemetry API. - Update shared state in SDK to use Supplier<Resource> instead of Resource - Add helper utils to expose private methods to supply the supplier - Create new ExtendedOpenTelemetry* API/SDK and end-to-end test.
…internal/otlp/EntityRefMarshaler.java Co-authored-by: jack-berg <[email protected]>
…iderBuilder.java Co-authored-by: jack-berg <[email protected]>
- Hide more methods (no incubating methods in public APIs that are not internal) - Update builder API to use attributes directly - Use static instances in Noop API
…lemetrySdk.getOpenTelemetrySdk() to return the extended instance
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7496 +/- ##
============================================
+ Coverage 90.01% 90.03% +0.02%
- Complexity 7090 7101 +11
============================================
Files 803 805 +2
Lines 21444 21484 +40
Branches 2092 2093 +1
============================================
+ Hits 19302 19343 +41
+ Misses 1477 1475 -2
- Partials 665 666 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Alternative is #7500 |
api/incubator/src/main/java/io/opentelemetry/api/incubator/ExtendedOpenTelemetry.java
Outdated
Show resolved
Hide resolved
import javax.annotation.concurrent.ThreadSafe; | ||
|
||
/** A new interface for creating OpenTelemetrySdk that supports getting {@link ConfigProvider}. */ | ||
public class ExtendedOpenTelemetrySdk extends OpenTelemetrySdk |
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.
Alternatively, we could skip extending OpenTelemetrySdk and avoid awkwardness of trying to figure mesh together builder and constructors. OpenTelemetrySdk doesn't do much anyway so not much to repeat.
Maybe do something like have ExtendedOpenTelemetrySdk
accept a delegate OpenTelemetrySdk
instance during initialization and skip the builder pattern? Usage could be something like:
ExtendedOpenTelemetrySdk sdk = ExtendedOpenTelemetrySdk.create(OpenTelemetrySdk, SdkConfigProvider);
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.
The goal (IMO) is to have AutoConfiguredOpenTelemetrySdk.getOpenTelemetrySdk()
return the instance that we can work with - so that library users don't have to jump through hoops to have the correct instance.
* @param configurator A callback fleshing out tracers. | ||
* @return this | ||
*/ | ||
public ExtendedOpenTelemetrySdkBuilder withTracerProvider( |
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.
What do we get from these withers?
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 is from the entity PR where it's actually needed
* @throws DeclarativeConfigException if unable to parse or interpret | ||
*/ | ||
public static OpenTelemetrySdk parseAndCreate(InputStream inputStream) { | ||
public static ExtendedOpenTelemetrySdk parseAndCreate(InputStream inputStream) { |
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 is cool - frees up callers from needing to worry about initializing SdkConfigProvider
.matches("OpenTelemetrySdk\\{tracerProvider=SdkTracerProvider\\{.*}.*}") | ||
.matches("OpenTelemetrySdk\\{.*, meterProvider=SdkMeterProvider\\{.*}.*}") | ||
.matches("OpenTelemetrySdk\\{.*, loggerProvider=SdkLoggerProvider\\{.*}.*}") | ||
.matches("OpenTelemetrySdk\\{.*, propagators=DefaultContextPropagators\\{.*}}"); |
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.
Meh yeah this is probably an overdue improvement 😉
@jack-berg please check if you like the alternative without API changes better: #7500 |
Ok I pulled this down and started tweaking some things to see how they would work out, and eventually concluded that it would be better to commit / push my changes rather than leave a bunch of comments. I do like the overall direction of this. Changes available for review here: https://github.com/open-telemetry/opentelemetry-java/compare/main...jack-berg:opentelemetry-java:jack-extended-opentelemetry?expand=1 Summary:
Let me know what you think. If you like it, consider merge my changes into your branch and mark ready for review. |
@jack-berg great - I like it! I merged your changes 😄 The builder is from joshs PR where he needs it for the entities - but I guess we can tackle that in a later PR. |
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.
@jkwatson take a look at the change to the public API when you have a chance
*** MODIFIED METHOD: PUBLIC FINAL (<- NON_FINAL) io.opentelemetry.sdk.logs.SdkLoggerProvider getSdkLoggerProvider() | ||
*** MODIFIED METHOD: PUBLIC FINAL (<- NON_FINAL) io.opentelemetry.sdk.metrics.SdkMeterProvider getSdkMeterProvider() | ||
*** MODIFIED METHOD: PUBLIC FINAL (<- NON_FINAL) io.opentelemetry.sdk.trace.SdkTracerProvider getSdkTracerProvider() | ||
*** MODIFIED METHOD: PUBLIC FINAL (<- NON_FINAL) io.opentelemetry.api.trace.TracerProvider getTracerProvider() |
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 changes are technically breaking changes, yeah? If someone had implemented their own extension to OpenTelemetrySdk? I don't know how likely that is to be the case, but I could imagine a vendor doing it. How concerned are we about making all of these methods (and the class) final breaking someone's custom implementation?
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.
The class was final before, so it was not possible to extend OpenTelemetrySdk
before
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.
the class itself was final previously
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.
Doesn't that top line in the api diff say it's being made final? Am I reading that wrong?
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.
oh! it's the opposite. 🤦🏽
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.
So, this, then, I guess, is the only thing that's possible a concern to think about...now people will be able to extend OpenTelemetrySdk, which could potentially cause future breaking changes, if we add methods and they collide with ones that have been added externally. 🤔
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.
That's true. But I believe we're already exposed to similar risk: Consider the OpenTelemetry
interface: People can extend it, implement the extended methods, and later we could extend OpenTelemetry
with conflicting methods.
👍🏽 Seems ok to me. |
Thanks! |
this is awesome, thanks all! |
Fixes #7495