Change internal tracing to use otel trace#3567
Change internal tracing to use otel trace#3567bogdandrutu merged 1 commit intoopen-telemetry:mainfrom
Conversation
| sr := new(oteltest.SpanRecorder) | ||
| tp := oteltest.NewTracerProvider(oteltest.WithSpanRecorder(sr)) | ||
| otel.SetTracerProvider(tp) | ||
| defer otel.SetTracerProvider(trace.NewNoopTracerProvider()) |
There was a problem hiding this comment.
This is now the 5th time the same lines are repeated in tests. Wouldn't it make sense to externalize it?
tigrannajaryan
left a comment
tigrannajaryan
left a comment
There was a problem hiding this comment.
Great to see this coming.
I am curious how performance of Otel tracing compares to OC tracing? Do we have any benchmarks that measure the impact of tracing on component operations?
| // Enable OpenTelemetry observability plugin. | ||
| opts = append(opts, grpc.WithUnaryInterceptor(otelgrpc.UnaryClientInterceptor())) | ||
| opts = append(opts, grpc.WithStreamInterceptor(otelgrpc.StreamClientInterceptor())) | ||
|
|
There was a problem hiding this comment.
This enables context propagation, right? So, we will propagate context for grpc-based protocols, like OTLP. Is this something we want?
There was a problem hiding this comment.
This is similar with what we had before with opencensus using the stats handler.
There was a problem hiding this comment.
I don't think any propagators are configured in this PR, so no propagation should happen by default. If a custom build (or later PR for this repo/contrib) were to configure a global propagator then it would be used by these interceptors.
There was a problem hiding this comment.
Is it not the case that w3c is the default?
There was a problem hiding this comment.
No, unfortunately it isn't.
The OpenTelemetry API MUST use no-op propagators unless explicitly configured otherwise.
Based on our loadtests no significant difference, the otel seems actually to be slightly faster. @MrAlias @Aneurysm9 do you have these benchmarks? |
We have some basic tracer benchmarks, starting and stopping spans, ID handling, etc. This should get all the benchmarks in the core repo. |
| // Enable OpenTelemetry observability plugin. | ||
| opts = append(opts, grpc.WithUnaryInterceptor(otelgrpc.UnaryClientInterceptor())) | ||
| opts = append(opts, grpc.WithStreamInterceptor(otelgrpc.StreamClientInterceptor())) | ||
|
|
There was a problem hiding this comment.
I don't think any propagators are configured in this PR, so no propagation should happen by default. If a custom build (or later PR for this repo/contrib) were to configure a global propagator then it would be used by these interceptors.
| _, span = rec.tracer.Start(context.Background(), spanName, trace.WithLinks(trace.Link{ | ||
| SpanContext: trace.SpanContextFromContext(receiverCtx), | ||
| })) |
There was a problem hiding this comment.
| _, span = rec.tracer.Start(context.Background(), spanName, trace.WithLinks(trace.Link{ | |
| SpanContext: trace.SpanContextFromContext(receiverCtx), | |
| })) | |
| ctx, span = rec.tracer.Start(ctx, spanName, trace.WithLinks(trace.Link{ | |
| SpanContext: trace.SpanContextFromContext(receiverCtx), | |
| }), trace.WithNewRoot()) |
I believe this does what you're trying to do without needing the later call to ContextWithSpan().
There was a problem hiding this comment.
Not working per open-telemetry/opentelemetry-go#2071
There was a problem hiding this comment.
Please file a PR when that issue is fixed.
For the moment we rely on the global TracerProvider, in a future PR will change that. Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
4 participants
For the moment we rely on the global TracerProvider, in a future PR will change that.
Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com