-
Notifications
You must be signed in to change notification settings - Fork 869
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
Consider removing jaeger-thrift exporter from default agent #1973
Comments
I think @iNikem put in the original request to add it...I wonder what he thinks about this idea. |
Removing it makes sense to me, but we'll definitely wait for @iNikem to chime in. |
I don't oppose it. |
Hey all, sorry to comment on such an old issue but my reading of the documentation is different. To me it looks like the gRPC recommendation is for the Agent forwarding spans to the Collector, not the protocol that Java exports. We're currently integrating with Jaeger for the reason that the UDP-based agents have an low profile for our various runtimes, and can resiliently batch and retry span collection without a single point of failure. Is it possible to revisit this decision? |
hi @noahseger, we can definitely discuss, do you mind opening a fresh issue for it and referencing this one where the original decision was made? |
The official and recommended protocol between an agent and a collector is gRPC.
https://www.jaegertracing.io/docs/1.21/apis/#protobuf-via-grpc-stable
Actually, thrift is meant for use with UDP on localhost, something we don't support in Java
https://www.jaegertracing.io/docs/1.21/apis/#thrift-over-udp-stable
Thrift over HTTP is the legacy format for sending data to non-localhost from what I understand.
I feel as if bundling thrift into our agent is a bit much considering it's legacy, and figure Splunk's distribution, which I believe is the main user of this exporter, can include it instead?
Otherwise, we should get it added to the environment variables spec
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-environment-variables.md
Not only is
jaeger_thrift
not there, but more problematically,jaeger
andjaeger_thrift
can't be enabled together because of tension in the endpoint argument, one must be something likelocalhost:1234
, the other must be something likehttp://localhost:14268/api/traces
but we use the same endpoint variable for both. While there is likely little use case for setting both, I don't see a reason our config should just ban that completely due to a corner case in variable parsing.We would also want to rename to
jaeger_thrifthttp
I think to remove confusion with UDP.I'm not interested in making this spec change and am inclined to delete our integration unless someone volunteers to spec this :) I do very much want the endpoint parsing tension to be resolved if we do go with including it.
@jkwatson @breedx-splk
The text was updated successfully, but these errors were encountered: