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.

Sign up for GitHub

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

Jump to bottom

Support Jaeger tracer #1030

Closed
d-ulyanov opened this issue Apr 14, 2019 · 16 comments
Closed

Support Jaeger tracer #1030

d-ulyanov opened this issue Apr 14, 2019 · 16 comments
Labels
difficulty: medium feature request/improvement priority: P1

Comments

@d-ulyanov
Copy link
Contributor

What happened
At the moment Thanos aleady is instrumented with OpenTracing and GCT used as hardcoded implementation.

What you expected to happen
It would be great to support Jaeger tracer in all Thanos components because its quite popular too. So, I propose to make tracer pluggable as it works for stores and add also implement Jaeger support.

We're using Jaeger a lot, so I can take care of this issue.
What do you think?
@bwplotka
Copy link
Member

Yea, I think it makes sense, but again, we want to limit ourselve in terms of maintainance.

We don't want to support every tiny tracing provider with different API etc.

We have 3 options generally:
A) Create our own API, so backend has to adapt not us.
B) Add most popular ones
C) Use some common REST API

Since C is non existent, and we now we aim for two... B might be what we want for, so it's green light from my side ✔️

cc @domgreen as he is interesting in tracing side

@bwplotka bwplotka assigned bwplotka and unassigned bwplotka Apr 14, 2019
@povilasv
Copy link
Member

povilasv commented Apr 14, 2019 •
edited
Loading

I think we should use https://opentracing.io/ they have a library for go

@bwplotka
Copy link
Member

Yea, we use it already: https://github.com/improbable-eng/thanos/blob/1f043ec4a97709939aa3addbeca5b6510b7c6da6/go.mod#L35

But still it requires different providers having different implementations.

@rafaeljesus
Copy link

rafaeljesus commented Apr 14, 2019 •
edited
Loading

I would recommend looking into https://opencensus.io besides tracing it has API for metrics as well, in addition, you can export your data to multiple backends simultaneously i.e Prometheus, Datadog, HoneyComb, Jaeger, NewRelic and so on, for both metrics and traces

@rafaeljesus
Copy link

just as a info, opentracing will get merged to https://opencensus.io pretty soon

@d-ulyanov
Copy link
Contributor Author

Making Thanos for Prometheus and using anything else for metrics collection is very strange idea :)
So I'm not agree with it :)
Also, Thanos already partially instrumented by opentracing and its also allows us to use different backends. I propose to add Jaeger backend support. This feature also covers Zipkin backend cause Jaeger client supports it.

@bwplotka
Copy link
Member

bwplotka commented Apr 15, 2019 •
edited
Loading

Agree with @d-ulyanov

@rafaeljesus
Copy link

rafaeljesus commented Apr 15, 2019 •
edited
Loading

Making Thanos for Prometheus and using anything else for metrics collection is very strange idea :)

True thing, I was more referring to other Tracing providers, but it might be too much for thanos since we already have almost all in place basically

@brancz
Copy link
Member

brancz commented May 3, 2019 •
edited
Loading

OpenCensus and the resulting merger allow to just use the tracing capabilities. Kubernetes is likely to do the exact same thing, where it will continue to use Prometheus for metrics and the merger project for tracing. As for Thanos, we have an immediate need for jaeger tracing, so if people are not opposed, I'd like to implement jaeger support in Thanos and once the merger project exists evaluate whether we should do a switch. Does that sound reasonable?

// edit

Just to be clear, by “we” I meant us at Red Hat.

@simonpasquier
Copy link
Contributor

cc @jpkrohling @pavolloffay @objectiser for awareness as they're working on Jaeger...

@rafaeljesus
Copy link

we have an immediate need for jaeger tracing

sounds good to me, I'm running Jaeger, and it would be pretty handy to operate Thanos with tracing in place.

@povilasv
Copy link
Member

povilasv commented May 3, 2019

👍 @d-ulyanov AFAIK you are working on this, right?

@brancz
Copy link
Member

brancz commented May 3, 2019

@d-ulyanov let us know if there is anything we can help with, I'd very much like to see jaeger being integrated :)

@d-ulyanov
Copy link
Contributor Author

Hi, @brancz!
We've started to work on this issue recently. In Russia we have almost a week of holidays in first half of May, sorry :) But I expect that we'll finish it in the middle of May.
Our Thanos fork already integrated with Jaeger, just need some time to clean up code.

@IKSIN
Copy link
Contributor

IKSIN commented May 15, 2019

#1147
Pull request for this issue.

bwplotka pushed a commit that referenced this issue Jun 4, 2019
@IKSIN @bwplotka
Support Jaeger tracer #1030 (#1147)
fa9ca8b 
* Add jaeger tracing feature.

* Remove comments

* Remove comments

* Refactoring tracing

* Implementing jaeger logger.

* Add jaeger force tracing header.

* Use debugName for tracing service-name

* RemoveFactory config

* Formatting fix; Use io.Closer intead func() error

* Rename gcloud => stackdriver

* Refactoring google tracing testing.

* Delete comments.

* Rename gcloud flags => stackdriver.

* Update tracing docs.

* Fix ..traceType to ..tracerType

* Refactoring NoopTracer; Comments for exported functions.

* Remove noop tracer. Fix docs.

* Remove noop tracer. Fix docs.

* Config tracing same as objstore.

* Configure jaeger tracing from YAML. Some small fixes.

* Fix errcheck

* Add X-Thanos-Trace-Id HTTP header for simplified search traces

* Cleanup

* make docs

* Add store addr to tracing tags.

* Tracing refactoring.

* Fix noop tracing closer.

* Add few tracing spans.

* Pass prometheus registry to jaeger.

* go.mod

* Refactoring

* Resolve go mod conflicts

* Remove comments.

* PR refactoring for review.

* Format files.
FUSAKLA pushed a commit to FUSAKLA/thanos that referenced this issue Jun 8, 2019
@IKSIN @FUSAKLA
Support Jaeger tracer thanos-io#1030 (thanos-io#1147)
0321573 
* Add jaeger tracing feature.

* Remove comments

* Remove comments

* Refactoring tracing

* Implementing jaeger logger.

* Add jaeger force tracing header.

* Use debugName for tracing service-name

* RemoveFactory config

* Formatting fix; Use io.Closer intead func() error

* Rename gcloud => stackdriver

* Refactoring google tracing testing.

* Delete comments.

* Rename gcloud flags => stackdriver.

* Update tracing docs.

* Fix ..traceType to ..tracerType

* Refactoring NoopTracer; Comments for exported functions.

* Remove noop tracer. Fix docs.

* Remove noop tracer. Fix docs.

* Config tracing same as objstore.

* Configure jaeger tracing from YAML. Some small fixes.

* Fix errcheck

* Add X-Thanos-Trace-Id HTTP header for simplified search traces

* Cleanup

* make docs

* Add store addr to tracing tags.

* Tracing refactoring.

* Fix noop tracing closer.

* Add few tracing spans.

* Pass prometheus registry to jaeger.

* go.mod

* Refactoring

* Resolve go mod conflicts

* Remove comments.

* PR refactoring for review.

* Format files.
@GiedriusS
Copy link
Member

This has been merged so we can close this. Will be available in the next version. Thanks everyone! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty: medium feature request/improvement priority: P1
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@rafaeljesus @GiedriusS @simonpasquier @IKSIN @d-ulyanov @brancz @bwplotka @povilasv