-
Notifications
You must be signed in to change notification settings - Fork 782
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
Jaeger integration #628
Comments
I've read the issue, and it seems jaeger strangely encodes the endpoint with a query parameter to say json. Seems better for it to just act like normal http there, right? |
Changing to accept For options: easiest is to have the endpoint that's emulating zipkin do that. Zipkin's accepts a json formatting or thrift. If thrift is needed, it goes along with the content-type header. If for some reason jaeger cannot accept json, but it can accept thrift, it would be relatively straightforward to add an option to change the encoding to (zipkin's) thrift. The least easy would be for you to create your own jaeger reporter. It is way early to ask for sleuth to create one. We have an order of magnitude more interest in AWS X-Ray for example, so more likely to do something with that if we are adding new things. (even that might not end up in this repo) |
To correct myself I would create a PR with jaeger reporter. |
The type of changes you seem to need for reporter imply a temporary change
while jaeger requires a query parameter to use its zipkin adapter. Why not
suggest a ZipkinRestTemplateCustomizer workaround this is fixed? Once
that's fixed, users will not need anything special.
|
For now we're not considering adding another reporter due to reasons that Adrian has suggested. I'm closing this for now. |
Hi, Do you plan to support jaeger libraries instead of zipkin only? |
You can use the Brave OpenTracing bridge to be OpenTracing compliant. Also Jaeger has the Zipkin endpoint so you can send requests successfuly to Jaeger. Unless I misunderstood you and I'm missing sth. |
Yes, this I am doing right now - both use Brave OpenTracing and Jaeger's zipkin endpoint but when using this "workaround" I am missing functionality like adaptive sampler -- this requires jaeger-agent integration instead of sending spans directly to jaeger-collector (jaeger's zipkin endpoint). The other thing is that jaeger looks more mature despite of it's age. They also plan to support W3C Trace Context headers as mentioned here which IMO is the way to go. All this makes jaeger more attractive than zipkin. That's why I am asking about native (in spring-cloud-sleuth) support plans for jaeger instead of using workarounds like using compatible endpoints. |
there's no change in this topic, the information you've mentioned isn't new. Jaeger has had the thrift rpc agent for a long time. Personally, I've done quite a lot of work on the w3c headers (more than folks in jaeger frankly), and there's literally a PR in brave on this, so that is not a differentiator. |
If there's demand for w3c tracecontext, we should fork a different issue btw as it has nothing to do with jaeger or opentracing. If that happens, here's the issue to link that to openzipkin/brave#693 |
also if there is earnest interest in jaeger support, options besides expecting us to break all apis include:
The zipkin folks are in the middle of a lot of work moving to the ASF as well as supporting the very new support for brave (which we aren't likely to want to break). If someone wanted to make jaeger work better they could make a repo similar to what exists for stackdriver like this https://github.com/openzipkin/zipkin-gcp Then, they could use normal spring autoconfiguration. It is well understood that folks want us to pin to opentracing 0.31 in order to break again for 0.32. If you instead consider the above approach, you'll get much farther in a stable way, without burdening others. |
Hi,
we have some users asking about sending data to Jaeger server. Would you accept a new module with Jaeger integration? It would be similar to
spring-cloud-sleuth-zipkin
.The text was updated successfully, but these errors were encountered: