opentelemetry-java icon indicating copy to clipboard operation
opentelemetry-java copied to clipboard

Drop support for jaeger-thrift

Open breedx-splk opened this issue 2 years ago • 11 comments

I'd like to propose that we drop support for the jaeger-thrift exporter. The upstream repo has been marked deprecated and they're actively encouraging folks to use OpenTelemetry instead. They're also refusing security patches.

Furthermore, the docs indicate

Since v1.35, the Jaeger backend can receive trace data from the OpenTelemetry 
SDKs in their native OpenTelemetry Protocol (OTLP)].

Overall, I think this just makes sense and reduces an overwhelming number of choices for end users. Also curious what @pavolloffay thinks.

breedx-splk avatar Jun 30 '22 21:06 breedx-splk

Related: https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/1973

breedx-splk avatar Jun 30 '22 21:06 breedx-splk

We're currently using this at Verta, so I would prefer that we don't drop support for it quite yet.

jkwatson avatar Jun 30 '22 23:06 jkwatson

Yeah I'm proposing that we keep it in the splunk distro until we deprecate the Smart Agent, which won't happen until December, but I figured upstream could get ahead of that (or that we'd need this very discussion 😁 )

breedx-splk avatar Jun 30 '22 23:06 breedx-splk

Yeah I'm proposing that we keep it in the splunk distro until we deprecate the Smart Agent, which won't happen until December, but I figured upstream could get ahead of that (or that we'd need this very discussion 😁 )

👍🏽 It won't be hard for us to switch over to using grpc, rather than thrift for our jaeger exporter; I just need some lead time to make sure it gets on the schedule.

jkwatson avatar Jul 01 '22 16:07 jkwatson

No objections from the Jaeger community.

We recommend people to use Jaeger gRPC and/or switch to OTLP.

pavolloffay avatar Jul 04 '22 06:07 pavolloffay

@pavolloffay I have heard from some users that while gRPC is the standard for Jaeger collector, UDP+Thrift is still idiomatic for Jaeger agent so they try to use it. I guess the jaeger-agent at least doesn't document supporting gRPC

https://www.jaegertracing.io/docs/1.35/deployment/#agent

Is it safe to say that we don't support the Jaeger agent with OTel SDKs and they must switch to using the OTel collector as an agent?

(Note for context, our README mentions "Jaeger over Thrift Over HTTP", but while it may not have been intentional, we support UDP by accepting ThriftSender)

anuraaga avatar Jul 04 '22 06:07 anuraaga

I guess the jaeger-agent at least doesn't document supporting gRPC

Correct, Jaeger agent does not support gRPC nor thrift over HTTP (the thrift exporter in this repo https://github.com/open-telemetry/opentelemetry-java/tree/main/exporters/jaeger-thrift)

I didn't know a sender can be configured https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/jaeger-thrift/src/main/java/io/opentelemetry/exporter/jaeger/thrift/JaegerThriftSpanExporterBuilder.java#L26 I don't know if people are using this API.

@breedx-splk what is the motivation to remove jaeger-thrift? Is there any technical reason or just drop it because Jaeger clients are deprecated?

pavolloffay avatar Jul 04 '22 07:07 pavolloffay

It may not have been SDK usage, I think I was remembering this ask for support in the javaagent

https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/5821

There does seem to be one OSS usage

https://github.com/xkazxx/jaeger_demo/blob/e4e04be299e923beeb0434a5989775fa264ec4b7/src/main/java/com/xkazxx/jaegerdemo/config/JaegerDemoWebConfigure.java#L27

In general, even though the use of the deprecated dependency isn't ideal, it looks like the final release was made 3 days ago, so it's not an ancient library yet. Without a known security issue, I'd still err towards keeping what we have for now without a more proactive push for removal. A proactive push for removal would probably mean

  1. Removing Jaeger Thrift from spec compliance table https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix.md#exporters
  2. Documenting that OpenTelemetry doesn't officially support Jaeger thrift in the jaeger exporter spec, or at least that it is fine for SDKs to ignore it. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk_exporters/jaeger.md
  3. (Maybe) Jaeger deprecating jaeger-agent officially, or otherwise adding text to their document suggesting that OpenTelemetry users should be using the OTel collector, not the jaeger-agent https://www.jaegertracing.io/docs/1.35/deployment/#agent

I'd be supportive of that happening if someone wanted to drive it through ;)

anuraaga avatar Jul 04 '22 07:07 anuraaga

(Maybe) Jaeger deprecating jaeger-agent officially

There is no plan to deprecate jaeger-agent. At least I haven't heard anything about it in the Jaeger community.

The jaeger-java client was released recently to fix some CVEs.

pavolloffay avatar Jul 04 '22 07:07 pavolloffay

Correct, Jaeger agent does not support gRPC nor thrift over HTTP (the thrift exporter in this repo https://github.com/open-telemetry/opentelemetry-java/tree/main/exporters/jaeger-thrift)

Right, and that repo depends on jaeger-client which is now formally deprecated and the repo has been archived.

@breedx-splk what is the motivation to remove jaeger-thrift? Is there any technical reason or just drop it because Jaeger clients are deprecated?

Yeah, the main thing is that it's a liability. If something comes up with jaeger-client (like a bug or a security fix or whatever) we (opentelemetry) will have no recourse other than forking the archived repo. It also creates a confusing story for the user -- users who land on the jaeger page that says to use OTLP might get confused by the fact that there's a jaeger exporter and configure that into their app. If the exporter doesn't exist any more, it reduces the chances of that happening and improves the OpenTelemetry as the standard.

The jaeger-java client was released recently to fix some CVEs.

I know, one of those was mine...and it was literally the day before the repo was archived.

breedx-splk avatar Jul 07 '22 20:07 breedx-splk

it reduces the chances of that happening and improves the OpenTelemetry as the standard.

I agree with this :) So listed out steps that we should probably take to remove it from OpenTelemetry completely. I think we just need a volunteer to drive that (hint hint :P)

anuraaga avatar Jul 08 '22 01:07 anuraaga