Remove skipping span creation by checking parent spans

[XSAM]

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

go-redis/extra/redisotel/tracing.go

Lines 94 to 96 in f3fe611

	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.

[vmihailenco]

The current behavior is useful when you only instrument certain routes/operations and don't care about dangling/orphan redis calls. It is hard to achieve it otherwise.

Can we make this configurable? AFAIR some instrumentations provide an option like WithCreateRootSpans that achieves what you suggst.

[XSAM]

Users can use their sampler to allow/disallow orphan spans. For instance, they can use ParentBased(root=AlwaysOff) to prevent creating dangling/orphan spans, though it is a global-like config of otel, which affects all libs (not just redis-go)

https://github.com/open-telemetry/opentelemetry-specification/blob/a03382ada8afa9415266a84dafac0510ec8c160f/specification/trace/sdk.md#parentbased

So, I am not sure WithCreateRootSpans is necessary. And, having this following code would bypass users' sampler even if they want to use always-on sampling.

go-redis/extra/redisotel/tracing.go

Lines 94 to 96 in f3fe611

	if !trace.SpanFromContext(ctx).IsRecording() {
	return hook(ctx, network, addr)
	}

---

BTW, using the IsRecording method may drop non-orphan spans, as the parent span might be ended (means not recording) before creating a child span, but that does not make the child span an orphan. We should use IsValid to decide if it is a valid parent span and use IsSampled to know if it is kept.

https://github.com/open-telemetry/opentelemetry-specification/blob/a03382ada8afa9415266a84dafac0510ec8c160f/specification/trace/api.md#isrecording

After a Span is ended, it SHOULD become non-recording and IsRecording SHOULD always return false.

[XSAM]

My suggestion is to remove this kind of code, and mention the ParentBased(root=AlwaysOff) sampler as an alternative on the changelog. So users know how to keep this behavior if they want to.

go-redis/extra/redisotel/tracing.go

Lines 94 to 96 in f3fe611

	if !trace.SpanFromContext(ctx).IsRecording() {
	return hook(ctx, network, addr)
	}

[vmihailenco]

I was not aware about root=AlwaysOff 👍

Then yes, let's document it and remove the code you've mentioned.