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

Delete jaeger exporters #6119

Merged
merged 2 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,6 @@ dependency as follows, replacing `{{artifact-id}}` with the value from the "Arti
| [OTLP Exporters](./exporters/otlp/all) | OTLP gRPC & HTTP exporters, including traces, metrics, and logs | `opentelemetry-exporter-otlp` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-otlp.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-otlp) |
| [OTLP Logging Exporters](./exporters/logging-otlp) | Logging exporters in OTLP JSON encoding, including traces, metrics, and logs | `opentelemetry-exporter-logging-otlp` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-logging-otlp.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-logging-otlp) |
| [OTLP Common](./exporters/otlp/common) | Shared OTLP components (internal) | `opentelemetry-exporter-otlp-common` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-otlp-common.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-otlp-common) |
| [Jaeger gRPC Exporter](./exporters/jaeger) | Jaeger gRPC trace exporter (deprecated [1]) | `opentelemetry-exporter-jaeger` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-jaeger.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-jaeger) |
| [Jaeger Thrift Exporter](./exporters/jaeger-thrift) | Jaeger thrift trace exporter (deprecated [1]) | `opentelemetry-exporter-jaeger-thift` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-jaeger-thrift.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-jaeger-thrift) |
| [Logging Exporter](./exporters/logging) | Logging exporters, including metrics, traces, and logs | `opentelemetry-exporter-logging` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-logging.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-logging) |
| [Zipkin Exporter](./exporters/zipkin) | Zipkin trace exporter | `opentelemetry-exporter-zipkin` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-zipkin.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-zipkin) |
| [Prometheus Exporter](./exporters/prometheus) | Prometheus metric exporter | `opentelemetry-exporter-prometheus` | <!--VERSION_UNSTABLE-->1.34.0-alpha<!--/VERSION_UNSTABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-prometheus.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-prometheus) |
Expand All @@ -275,13 +273,6 @@ dependency as follows, replacing `{{artifact-id}}` with the value from the "Arti
| [JDK Sender](./exporters/sender/okhttp) | Java 11+ native HttpClient implementation of HttpSender (internal) | `opentelemetry-exporter-sender-jdk` | <!--VERSION_UNSTABLE-->1.34.0-alpha<!--/VERSION_UNSTABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-sender-jdk.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-sender-jdk) | |
| [gRPC ManagedChannel Sender](./exporters/sender/grpc-managed-channel) | gRPC ManagedChannel implementation of GrpcSender (internal) | `opentelemetry-exporter-sender-grpc-managed-channel` | <!--VERSION_STABLE-->1.34.0<!--/VERSION_STABLE--> | [![Javadocs](https://www.javadoc.io/badge/io.opentelemetry/opentelemetry-exporter-sender-grpc-managed-channel.svg)](https://www.javadoc.io/doc/io.opentelemetry/opentelemetry-exporter-sender-grpc-managed-channel) | |

**[1]**: Jaeger now
has [native support for OTLP](https://opentelemetry.io/blog/2022/jaeger-native-otlp/) and jaeger
exporters are now deprecated. `opentelemetry-exporter-jaeger-thrift` will continue to be published
until 1.34.0 (January 2024) but no new PRs will be accepted except security related bugfixes. After
1.34.0, `io.opentelemetry:opentelemetry-bom` will reference the last published version, but no
additional versions will be published.

### SDK Extensions

| Component | Description | Artifact ID | Version | Javadoc |
Expand Down
3 changes: 0 additions & 3 deletions all/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,6 @@ tasks.named<JacocoReport>("jacocoTestReport") {
// Exclude mrjar (jacoco complains), shaded, and generated code
!it.absolutePath.contains("META-INF/versions/") &&
!it.absolutePath.contains("/internal/shaded/") &&
!it.absolutePath.contains("io/opentelemetry/proto/") &&
!it.absolutePath.contains("io/opentelemetry/exporter/jaeger/proto/") &&
!it.absolutePath.contains("io/opentelemetry/exporter/jaeger/internal/protobuf/") &&
!it.absolutePath.contains("io/opentelemetry/sdk/extension/trace/jaeger/proto/") &&
!it.absolutePath.contains("AutoValue_")
},
Expand Down
2 changes: 2 additions & 0 deletions bom/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

would shading them in help the calculus? just worried about @jkwatson getting stuck on old SDK

Copy link
Member Author

Choose a reason for hiding this comment

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

I would be open to that.. We would have to shade in opentelemetry-exporter-otlp-common and opentelemetry-exporter-sender-okhttp.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 internal GrpcSender / GrpcSenderProvider SPI.
    • Not clear which of the GrpcSender sender 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-channel would 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.
  • :exporters:common includes 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:common to resolve the things like Resource and InstrumentationScopeInfo in JaegerGrpcSpanExporter#export(SpanData), and another shaded version of :sdk:common just so the shaded :exporters:common classes 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

👍

3 changes: 0 additions & 3 deletions exporters/jaeger-proto/README.md

This file was deleted.

25 changes: 0 additions & 25 deletions exporters/jaeger-proto/build.gradle.kts

This file was deleted.

This file was deleted.

89 changes: 0 additions & 89 deletions exporters/jaeger-proto/src/main/proto/jaeger/api_v2/model.proto

This file was deleted.

58 changes: 0 additions & 58 deletions exporters/jaeger-proto/src/main/proto/jaeger/api_v2/time.proto

This file was deleted.

27 changes: 0 additions & 27 deletions exporters/jaeger-thrift/build.gradle.kts

This file was deleted.

Loading