A span with a new span ID MUST be created even for a sampling decision of DROP.

[Oberon00]

Bug Report

This bug should be present in master and at least since 0.5.0-beta (probably since the beginning)

Symptom

If the sampler returns a decision of DROP, StartActivity returns null.

What is the expected behavior?

An unsampled activity with a new span ID is created and returned (and set as current which is not spec-conformant but expected in .NET). This allows correlating logs also to unsampled operations.

Spec references:

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

DROP - IsRecording() == false, span will not be recorded and all events and attributes will be dropped.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#sdk-span-creation

3. Generate a new span ID for the Span, independently of the sampling decision. This is done so other components (such as logs or exception handling) can rely on a unique span ID, even if the Span is a non-recording instance.
4. [...] A non-recording span MAY be implemented using the same mechanism as when a Span is created without an SDK installed or as described in wrapping a SpanContext in a Span.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#wrapping-a-spancontext-in-a-span

The behavior is defined as follows:

• GetContext MUST return the wrapped SpanContext.

What is the actual behavior?

Null is returned for the activity, no span ID is available.

Reproduce

There is a unit test for the wrong behavior here:

opentelemetry-dotnet/test/OpenTelemetry.Tests/Trace/TracerProviderSdkTest.cs

Lines 274 to 277 in 641b2f7

	// This is not a root activity.
	// If sampling returns false, no activity is created at all.
	using var innerActivity = activitySource.StartActivity("inner");
	Assert.Null(innerActivity);

Additional Context

This is a pretty big deviation from the spec that will inhibit some use cases. Creating a new span ID even for unsampled spans was a very deliberate decision. There were even two competing PRs open-telemetry/opentelemetry-specification#1225 and open-telemetry/opentelemetry-specification#998 and it was explicitly decided on always creating a span ID. See especially the thread ending at open-telemetry/opentelemetry-specification#998 (comment)

[Oberon00]

Note that in the case I observed, I even got a null activity despite there being no current span (the parent context was extracted from HTTP headers, having IsRemote=true, TraceFlags=None)

[bledogit]

Up voting this issue. Furthermore, the behavior of StartActivity should be such that an activity is created with or without the sampled flag turned on, depending on the sampler settings, and the sampler should be such that an exception can change the DROP decision of the Activity and All its parents to record failed Spans created locally.

There should be at least the option to be able to record the root activity resulting of a transaction with no parent, like an API entry point which should return the traceID in the response (https://github.com/w3c/trace-context/blob/main/spec/21-http_response_header_format.md) . The returned TraceID (with possibly an orphan flag) then can be used to query the DB for spans with exceptions coming from an unsampled chain of Spans that where not sampled.

The main objective is to sample a percentage of the healthy transactions, while recording 100% of the local spans that resulted in an error/exception. We must be able to answer the question as why a transaction 00-ttt-xxx-00 failed. Without this distributed tracing becomes no better than debug logs.

[erchirag]

Can someone comment on any performance implications of creating activity object always even when sampling decision was to not collect current span ?

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/Benchmarks/Trace/TraceBenchmarks.cs#L38
Creating an activity, and feeding it to a no-op processor, is approximately of this (see above ^) much cost.

[cijothomas]

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/Benchmarks/Trace/TraceBenchmarks.cs#L38 Creating an activity, and feeding it to a no-op processor, is approximately of this (see above ^) much cost.

https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/test/Benchmarks/Trace/OpenTelemetrySdkBenchmarksActivity.cs#L38

And can go up, if tags/attributes are added. (most likely avoidable, by checking IsAllDataRequested before populating tags)

[erchirag]

What about any GC impact if lots of these objects are created in tight loop ?

[cijothomas]

The benchmarks shows the heap allocation (416B), so yes GC has to clean it up.

[bledogit]

If performance is an issue, can't these objects be pooled?

[cijothomas]

Tagging for consideration in 1.5.0

[TimothyMothra]

+1.
I think this is the root cause of #4087.

[cijothomas]

The PR to address got stale and closed. I suggest to move this out of 1.5 timeline, as this likely requires more discussions.

[github-actions]

This issue was marked stale due to lack of activity and will be closed in 7 days. Commenting will instruct the bot to automatically remove the label. This bot runs once per day.

[cijothomas]

Not stale, spec violation.