Delete jaeger exporters #6119
Delete jaeger exporters #6119
Conversation
…y-java into delete-jaeger-exporter
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6119 +/- ##
============================================
- Coverage 91.01% 90.96% -0.05%
+ Complexity 5702 5563 -139
============================================
Files 630 614 -16
Lines 16710 16205 -505
Branches 1656 1624 -32
============================================
- Hits 15208 14741 -467
+ Misses 1047 1015 -32
+ Partials 455 449 -6 ☔ View full report in Codecov by Sentry. |
| @@ -14,3 +14,5 @@ otelBom.addFallback("opentelemetry-extension-annotations", "1.18.0") | |||
| otelBom.addFallback("opentelemetry-sdk-extension-resources", "1.19.0") | |||
| otelBom.addFallback("opentelemetry-sdk-extension-aws", "1.19.0") | |||
| otelBom.addFallback("opentelemetry-extension-aws", "1.20.1") | |||
| // NOTE: opentelemetry-exporter-jaeger and opentelemetry-exporter-jaeger-thift are omitted because | |||
| // they contain dependencies on internal classes, which may have breaking API changes preventing compilation. | |||
There was a problem hiding this comment.
how bad would it be to copy those internal classes into the jaeger exporter module and make one more jaeger exporter release, so that this wouldn't become a blocker for users to update to the latest SDK version?
There was a problem hiding this comment.
Its quite a lot. The entire :exporters:common module is internal, and opentelemetry-exporter-jaeger relies heavily on it perform marshaling, grpc export (including all the okhttp sender stuff), tls stuff. opentelemetry-exporter-jaeger-thrift only relies on 1 or 2 classes, but opentelemetry-exporter-jaeger-thrift is barely used so we wouldn't gain much if we only ported internal classes for it.
There was a problem hiding this comment.
would shading them in help the calculus? just worried about @jkwatson getting stuck on old SDK
There was a problem hiding this comment.
I would be open to that.. We would have to shade in opentelemetry-exporter-otlp-common and opentelemetry-exporter-sender-okhttp.
There was a problem hiding this comment.
As much as I would appreciate it, I don't think my (possibly singular) use-case should add extra work like this. If I'm not the only one who needs to keep using the old jaeger exporters, then it might be worth it. Only do the extra work if you think others will benefit as well.
There was a problem hiding this comment.
I'm thinking that we should probably shade based on the intent(?) of https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/upgrading.md, e.g.
The primary blocker to upgrading the SDK is out of date Plugins. If a new version of the SDK were to break existing Plugin Interfaces, no user would be able to upgrade their SDK until the Plugins they depend on have been upgraded. Users could be caught between instrumentation they depend on requiring a version of the API which is not compatible with the version of the SDK which supports their Plugins.
There was a problem hiding this comment.
Users could be caught between instrumentation they depend on requiring a version of the API which is not compatible with the version of the SDK which supports their Plugins.
This doesn't apply to java. Since we always provide default noop implementations with any API addition, you should always be able to upgrade to a later API version than the SDK version. So users that are forced to stay in a lower version of the API shouldn't have a problem using instrumentation that uses a later version. There may be a degraded experience as portions of the API may have noop implementations despite an SDK being installed.
There was a problem hiding this comment.
I took a look at what it would take to shade the internal dependencies, and it turns its not simple. Problems include:
- Jaeger has dependency on
:exporters:common, and relies on the internalGrpcSender/GrpcSenderProviderSPI.- Not clear which of the
GrpcSendersender implementations should be included. Probably just:exporters:sender:okhttp, since including both would force users to resolve which to use using system properties. But excluding:exporters:ssender:grpc-managed-channelwould break support for that. - We need to relocate these classes to avoid collisions, but relocating with SPI seems to have some sharp edges. I couldn't figure out how to get the
META-INF/servies/*stuff to point to the relocated classes.
- Not clear which of the
:exporters:commonincludes internal references from:api:all(io.opentelemetry.api.internal.ConfigUtil) and:sdk:common(io.opentelemetry.sdk.internal.ThrottlingLogger). These are tricky because we end up needing two copies on the classpath: one non-shaded version of:sdk:commonto resolve the things likeResourceandInstrumentationScopeInfoinJaegerGrpcSpanExporter#export(SpanData), and another shaded version of:sdk:commonjust so the shaded:exporters:commonclasses can reference internal classes without risk of breaking with api changes.
Its the type of messiness that will take a fair amount of time to figure out, and I'm not interested in doing it without a strong reason. The way I see it, we're allowed to stop publishing artifacts and still be compliant with semconv. We've included entries in the bom which we've stopped publishing as a convenience, but TBH, those artifacts may have dependencies on internal classes as well and end up stop working. While it made us feel better, its probably not right to include old artifact references in the BOM, especially if those artifacts have any transitive dependencies on internal classes.
There was a problem hiding this comment.
While it made us feel better, its probably not right to include old artifact references in the BOM, especially if those artifacts have any transitive dependencies on internal classes.
👍
No description provided.