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

Migrating Jaeger tracing from OpenTracing to OpenTelemetry; Add OTLP exporter #5411

Merged
merged 40 commits into from Oct 2, 2022

Conversation

metonymic-smokey
Copy link
Contributor

@metonymic-smokey metonymic-smokey commented Jun 9, 2022

part of fixing #1972

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

This PR provides two changes that brings us closer to closing the gap between migrating from OpenTracing to OpenTelemetry, in particular:
a) This moves Jaeger client from OpenTracing to OpenTelemetry, without breaking changes. This means users can keep using their existing tracing configuration (some options will now not have any effect, but these are the ones that are obsolete after move to OTEL; read more in the updated documentation in the PR)
b) This adds the OTLP exporter, meaning now users will be able to leverage this protocol as well, with support of both gRPC and HTTP client.

Verification

  • Added E2E tests to validate Jaeger tracing
  • Manual tests

@metonymic-smokey
Copy link
Contributor Author

metonymic-smokey commented Jun 12, 2022

@matej-g could you please review my current progress, when you're free.

  1. I've created a function which returns an OTLP exporter but I'm not sure how to use the exporter.
  2. Working on E2E testing and pushed a draft commit for it, but I'll probably take some time for it.

TODO:

  1. E2E:
    todo - Fix E2E draft.
  2. Jaeger:
    Add more options from config.
    Unit tests(similar to google_cloud.go)
  3. OTLP Package:
    Test locally.
    Update docs with example config for OTLP package.

Thanks!

pkg/tracing/jaeger/jaeger.go Outdated Show resolved Hide resolved
pkg/tracing/jaeger/jaeger.go Outdated Show resolved Hide resolved
pkg/tracing/jaeger/jaeger.go Outdated Show resolved Hide resolved
pkg/tracing/client/factory.go Outdated Show resolved Hide resolved
)

type TracingConfig struct {
Type TracingProvider `yaml:"type"`
Config interface{} `yaml:"config"`
}

// NewOTELTracer returns an OTLP exporter based tracer.
func NewOTELTracer(ctx context.Context, logger log.Logger) trace.Tracer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So my thought was to simply add a new TracingProvider const to the list we have above, simply named OTLP. It would be yet another package under tracing directory, just as we have jaeger, google_cloud etc.

Then, an OTLP exporter should be simply one of the switch cases in the factory function below. We should also probably take some config from YAML, same we do for other tracing provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a new package otlp in a recent commit with some options for configuration. I will try to add some more config options if possible.
Please let me know what you think.

@matej-g
Copy link
Collaborator

matej-g commented Jun 15, 2022

Thank you @metonymic-smokey! I think this is going in the right direction, I provided some feedback which will hopefully unblock you (if not feel free to ask for more details).

As for checking traces in E2E test, we could simply do a HTTP request to Jaeger and parse the reply for the trace data we expect. You can take a look at an example from our other project here: https://github.com/observatorium/api/blob/main/test/e2e/traces_test.go#L195

@metonymic-smokey metonymic-smokey marked this pull request as ready for review June 16, 2022 14:57
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Nice progress! 💪 I added a few more hints for OTLP exporter.

pkg/tracing/otlp/otlp.go Outdated Show resolved Hide resolved
pkg/tracing/otlp/otlp.go Outdated Show resolved Hide resolved
pkg/tracing/jaeger/jaeger.go Outdated Show resolved Hide resolved
pkg/tracing/jaeger/jaeger.go Outdated Show resolved Hide resolved
pkg/tracing/jaeger/jaeger.go Outdated Show resolved Hide resolved
pkg/tracing/otlp/otlp.go Outdated Show resolved Hide resolved
test/e2e/e2ethanos/services.go Show resolved Hide resolved
test/e2e/tracing_test.go Outdated Show resolved Hide resolved
test/e2e/tracing_test.go Outdated Show resolved Hide resolved
metonymic-smokey and others added 20 commits October 1, 2022 17:00
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
@metonymic-smokey
Copy link
Contributor Author

@GiedriusS @matej-g i've updated the E2E tests and looks like they all pass :)
Thank you so much for working with me on this one!

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

shipping time

@GiedriusS GiedriusS merged commit 64e7e3b into thanos-io:main Oct 2, 2022
utukJ pushed a commit to utukJ/thanos that referenced this pull request Oct 13, 2022
…exporter (thanos-io#5411)

* draft: standalone OTLP tracer

Signed-off-by: Aditi Ahuja <aditi.ahuja@couchbase.com>

* added jaeger exporter

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: tracing test

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* minor refactor and cleanup

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* created package tracing/otlp

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* minor refactor

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: added some more config options for jaeger

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: extended e2e test

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: added OTLP GRPC options

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* minor fixes; temp prints

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* refactor: removed OT code, changed tag parsing to OTel tags

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* jaeger refactor; minor updates

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* jaeger: added some more config options

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* otlp: added retry options

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* organizing and pruning

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* added TLS config for OTLP

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: temp test fix for CI

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* added unit tests for jaeger

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: added sampler types to jaeger

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* replaced objstore with exthttp

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: test fixes

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* added jaeger remote sampler

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: reverted E2E test fix

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* lint and doc test fixes

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* minor refactor for OTLP

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* jaeger: modified sampler selection

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* review fixes

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: jaeger rate limiting sampler added

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* added OTLP unit tests; minor refactor

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* minor review fixes

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: rate limiting sampler implementation

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* review fixes

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* removed OT tracer struct

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* Use service name in resource

Signed-off-by: Matej Gera <matejgera@gmail.com>

* Fix tracing E2E test

Signed-off-by: Matej Gera <matejgera@gmail.com>

* Adjust trace ID extraction for exemplars

Signed-off-by: Matej Gera <matejgera@gmail.com>

* Add OTLP docs and adjust

Signed-off-by: Matej Gera <matejgera@gmail.com>

* Add CHANGELOG

Signed-off-by: Matej Gera <matejgera@gmail.com>

* review fixes

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

* draft: updated E2E tests

Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>

Signed-off-by: Aditi Ahuja <aditi.ahuja@couchbase.com>
Signed-off-by: Aditi Ahuja <ahuja.aditi@gmail.com>
Signed-off-by: Matej Gera <matejgera@gmail.com>
Co-authored-by: Matej Gera <matejgera@gmail.com>
Signed-off-by: utukj <utukphd@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants