Support for OTel tracing by colega · Pull Request #681 · grafana/dskit

colega · 2025-04-17T09:49:52Z

What this PR does

This adds support for OTel everywhere. It is similar to #385 which migrated to OTel, and takes pieces of code from there, but it does not remove support for OpenTracing.

`spanlogger` package changes

The new spanlogger.NewOTel will create a new OTel span, using the tracer provided. We're asking for a tracer instead of using one from spanlogger because that's how Tempo did it with per-package Tracer.

spanlogger.FromContext will find the existing span in the context, either OpenTracing or OTel one, and will return a SpanLogger that abstracts both.

In order to achieve that spanlogger.SpanLogger now contains either an opentelemetry.Span or an trace.Span (OTel). If there's an OTel one, it will do the operations with it, if not it will fall back to OpenTracing implementation. Some calls that were cheaper before, like LogFields now are more expensive since we have to translate them to OTel attributes.

`httpgrpc/server` package changes

When serving an HTTP request, we'll use OpenTracing to start the span if opentracing.IsGlobalTracerRegistered(). If there's an OTel span in the request context, we'll inject that one to the request.

`instrument.CollectedRequest()` changes

This function is widely used, so I migrated it from using opentracing.StartSpanFromContext to tracing.StartSpanFromContext: a new wrapper in the tracing package that will use the configured tracing library.

`tracing` package changes

The new tracing.StartSpanFromContext creates an adapter tracing.Span that acts like an opentracing.Span or trace.Span depending on the library used in the parent span from the context.Context provided to StartSpanFromContext.

This abstraction should disappear once we've migrated our dependencies to OTel. 🤞 (Famous last words)

The ExtractSampledTraceID, ExtractTraceID and ExtractTraceSpanID now support both opentracing and OTel libraries. This should solve #662 as well.

Finally, I brought from #385 the NewOTelFromJaegerEnv function that will configure OTel tracing using Jaeger env vars, with Jaeger exporter.

`server` package changes

Server now has the otelgrpc.NewServerHandler() which will inject the OTel Spans to the context.

`middleware` package changes

When propagating an HTTP or HTTPGRPC requests, we'll use OpenTracing to start the span if opentracing.IsGlobalTracerRegistered(). If there's an OTel span in the request context, we'll inject that one to the request.

Which issue(s) this PR fixes

Ref: grafana/mimir#2708

Checklist

Tests updated
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

colega · 2025-04-17T10:12:40Z

Still draft because I need to fix tests, add tests, and double check the server configuration of middlewares.

Ref: grafana/dskit#681 Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

mdisibio · 2025-04-17T11:58:22Z

The ExtractSampledTraceID, ExtractTraceID and ExtractTraceSpanID now support both opentracing and OTel libraries. This should solve #662 as well.

Yes, I wasn't sure about taking on the reference otel/trace, but it seems ok. The ones I am familiar with, Mimir/Loki/Tempo/Pyroscope all already have it. Are there any other downstream repos we should check, or do you think the impact of taking on this reference is minimal?

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

This updates dskit to grafana/dskit#681 which supports OTel tracing, but doesn't make any change, so it should still use OpenTracing Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega · 2025-04-22T07:18:41Z

I assume it's inevitable with my task to add the otel/trace dependency. The ultimate goal, once Loki & Pyroscope are migrated too, is to remove the opentracing-go one.

krajorama · 2025-04-22T10:23:46Z

-			middleware.ClientUserHeaderInterceptor,
-		),
+		grpc.WithChainUnaryInterceptor(unaryInterceptors...),
+		grpc.WithStatsHandler(otelgrpc.NewClientHandler()),


this is not protected with if opentracing.IsGlobalTracerRegistered() , should it be?

I wasn't going to... but ok, fair enough, let's add a check: c59312f

krajorama · 2025-04-22T10:31:23Z

+}
+
+func (t Tracer) wrapWithOpenTracing(next http.Handler) http.Handler {
+	// Do OpenTracing when it's registered.


nit: comment doesn't make much sense here as the if is elsewhere

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

This changes behaviour of tracing.StartSpanFromContext to always start a span, as we did previously, and we decide based on whether opentracing is registered or not. Also added tests, which I had to move to other packages as tracing installation is a global state. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

krajorama

lgtm

colega · 2025-04-23T15:47:55Z

I'm testing this PR on a local dev environment, to make sure that we didn't break any existing OpenTracing features before merging.

colega · 2025-04-24T12:55:24Z

I checked the traces and they still exist.

bboreham · 2025-06-04T11:06:47Z

+				labeler.Add(attribute.String("http.content_type", ct))
+			}
+
+			labeler.Add(attribute.String("headers", fmt.Sprintf("%v", r.Header)))


Should we worry about sensitive headers here, e.g. authorization tokens?

See here for instance.

Good catch:
#707

@bboreham

When I added the OTel tracing in #681 I went too far and added headers as tracing attributes. As @bboreham pointed out, this could log potentially sensitive headers like auth tokens. This is a quick fix to remove that from traces, we can add an allowlist or a blocklist in the future as a feature. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

@bboreham

When I added the OTel tracing in #681 I went too far and added headers as tracing attributes. As @bboreham pointed out, this could log potentially sensitive headers like auth tokens. This is a quick fix to remove that from traces, we can add an allowlist or a blocklist in the future as a feature. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

slim-bean · 2025-06-16T22:30:40Z

+		return
 	}

-	return sctx.TraceID().String(), sctx.IsSampled()


The previous behavior of ExtractSampledTraceID would return the traceID regardless if it was sampled or not, the new behavior only returns the traceID if it was sampled.

Unfortunately this is a breaking change for Loki where we would use this method to append the log line with additional information if the trace was sampled or not, but we relied on it returning the traceID to use in the log line regardless of being sampled.

) **What this PR does**: This PR fixes an issue where the tracing sampler would not be configured correctly if tracing is being configured from Jaeger environment variables and the `JAEGER_SAMPLER_TYPE` environment variable was set to `probabilistic`. **Which issue(s) this PR fixes**: Related to #681 and #700. **Checklist** - [ ] Tests updated

colega mentioned this pull request Apr 17, 2025

ObserveWithExemplar support other spancontext types #662

Draft

2 tasks

colega added a commit to grafana/mimir that referenced this pull request Apr 17, 2025

Update dskit to otel-everyting

3eb0be2

Ref: grafana/dskit#681 Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega mentioned this pull request Apr 17, 2025

Migrate Mimir tracing to opentelemetry grafana/mimir#2708

Closed

colega mentioned this pull request Apr 21, 2025

[Trace] Mimimum changes in dskit in order to migrate to opentelemetry #385

Closed

2 tasks

colega added 9 commits April 21, 2025 12:04

Add OTEL support to spanlogger & tracing packages

e3749fa

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add OTel to instrument & httpgrpc/server

f9ad9c1

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add OTel to grpc server

6690a7a

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add tracing.NewOTelFromJaegerEnv

430c43d

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add OTel to middlware tracing

8e857a4

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Fix tracing/span conditions

22400eb

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Fix schemas compatibility

8111ed6

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

go mod tidy

1d3c7df

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Fix deprecated noop tracer usage

5ded9ac

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega force-pushed the otel-everything branch from a2e778e to 5ded9ac Compare April 21, 2025 10:04

colega mentioned this pull request Apr 21, 2025

Update dskit to support OTel but use OpenTracing grafana/mimir#11267

Closed

4 tasks

colega added 2 commits April 21, 2025 17:21

Add some tests

6c79a30

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Use OpenTracing grpc interceptor when registered

ee70b35

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

simonswine mentioned this pull request Apr 22, 2025

Migrate trace instrumentation from opentracing to open-telemetry grafana/pyroscope#4127

Open

colega marked this pull request as ready for review April 22, 2025 09:12

krajorama self-requested a review April 22, 2025 10:24

krajorama reviewed Apr 22, 2025

View reviewed changes

Comment thread tracing/otel.go Outdated

Comment thread tracing/otel_test.go Outdated

Comment thread tracing/otel.go

Comment thread tracing/span.go Outdated

Comment thread tracing/span_test.go

Comment thread tracing/tracing.go

Server: add otel stats handler when no opentracing

c59312f

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

colega added 8 commits April 23, 2025 11:54

Move misplaced comment

cf8aa95

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Add content type attribute

586d82e

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

s/tracing.config/tracing.otelConfig/

06e1c24

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

s/collectorEndpoint/jaegerEndpoint/

4243e6d

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Hardcode the expected value

b8e1ea2

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Reduce nesting by checking !sampled

80751c9

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Update TestKeyValueToOTelAttribute

0b10eb1

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

Merge master commit '90fa6b9a2b76' into otel-everything

94471ca

krajorama reviewed Apr 23, 2025

View reviewed changes

Comment thread tracing/span.go Outdated

krajorama approved these changes Apr 23, 2025

View reviewed changes

colega merged commit cee08dc into main Apr 24, 2025
5 checks passed

colega deleted the otel-everything branch April 24, 2025 12:55

simonswine mentioned this pull request May 6, 2025

Migrate Pyroscope tracing to opentelemetry grafana/pyroscope#4178

Open

jmichalek132 mentioned this pull request May 12, 2025

Distributed tracing support w3c tracing headers (opentelemtry) grafana/mimir#8714

Open

bboreham reviewed Jun 4, 2025

View reviewed changes

colega mentioned this pull request Jun 4, 2025

Tracing: stop adding HTTP headers as tracing attributes #707

Merged

slim-bean reviewed Jun 16, 2025

View reviewed changes

charleskorn mentioned this pull request Aug 8, 2025

fix: tracing: correctly handle JAEGER_SAMPLER_TYPE=probabilistic #731

Merged

1 task

Conversation

colega commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

spanlogger package changes

httpgrpc/server package changes

instrument.CollectedRequest() changes

tracing package changes

server package changes

middleware package changes

Which issue(s) this PR fixes

Uh oh!

colega commented Apr 17, 2025

Uh oh!

mdisibio commented Apr 17, 2025

Uh oh!

colega commented Apr 22, 2025

Uh oh!

krajorama Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

colega Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

krajorama Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krajorama left a comment

Choose a reason for hiding this comment

Uh oh!

colega commented Apr 23, 2025

Uh oh!

colega commented Apr 24, 2025

Uh oh!

Uh oh!

bboreham Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

colega Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

slim-bean Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

colega commented Apr 17, 2025 •

edited

Loading

`spanlogger` package changes

`httpgrpc/server` package changes

`instrument.CollectedRequest()` changes

`tracing` package changes

`server` package changes

`middleware` package changes

slim-bean Jun 16, 2025 •

edited

Loading