Current Behavior
The redisotel skips the span creation by only checking the IsRecording of the parent span. This breaks the span creation defined by opentelemetry specification.
Create a span depending on the decision returned by ShouldSample: see description of ShouldSample's return value below for how to set IsRecording and Sampled on the Span, and the table above on whether to pass the Span to SpanProcessors.
Considering a user configures an always-on sampler for debugging and expects every operation taken by Redis that has instrumentation. With the current implementation, this would drop spans even if the user wants to see it.
https://github.com/open-telemetry/opentelemetry-go/blob/ba5d1268082fcc4ccb8fa967eb5133fa23e4f2ba/sdk/trace/sampling.go#L128-L130
Possible Solution
Remove codes like this before calling tracer.Start
|
if !trace.SpanFromContext(ctx).IsRecording() { |
|
return hook(ctx, network, addr) |
|
} |
More context
My concern only comes from OTel's perspective; let me know if it is a special case for go-redis. I am also happy to raise a PR to solve this issue.
Current Behavior
The
redisotelskips the span creation by only checking theIsRecordingof the parent span. This breaks the span creation defined by opentelemetry specification.Considering a user configures an always-on sampler for debugging and expects every operation taken by Redis that has instrumentation. With the current implementation, this would drop spans even if the user wants to see it.
https://github.com/open-telemetry/opentelemetry-go/blob/ba5d1268082fcc4ccb8fa967eb5133fa23e4f2ba/sdk/trace/sampling.go#L128-L130
Possible Solution
Remove codes like this before calling
tracer.Startgo-redis/extra/redisotel/tracing.go
Lines 94 to 96 in f3fe611
More context
My concern only comes from OTel's perspective; let me know if it is a special case for go-redis. I am also happy to raise a PR to solve this issue.