UpdateName doesn't trigger sampling decision if the span is not sampled when created.

[paivagustavo]

Currently, when create a Span we run the makeSamplingDecision. If the span is sampled, we create export.SpanData, attributes, events and links base from the SpanOptions. If it was not sampled we just ignored this all.

As per specs, UpdateName should re-trigger the makeSamplingDecision. Because of the current implementation, if a Span has not been sampled from the first time it contains a simpler noop Span (e.g., trying to add Links, Events or Childs would be ignored on the noop Span since isRecording would return false). Trying to trigger a makeSamplingDecision on this noop Span will return false and not even call the sampler.

To accommodate this behavior we would need to create "complete" Span for every single span. This would increase drastically the amount of resources needed by the otel, even with a NeverSample.

The solution is easily implemented but the consequences should be discussed.

This is a follow up from the comment of #223.

[jmacd]

This suggests that the sampling APIs for up-front sampling and tail-sampling ought to be different.

[jmacd]

As posted in gitter,

I'm pretty confused about all the PRs and issues relating to UpdateName vs SetName, sampling decisions, and WithRecord. I personally see this as an absurdity--that the spec says we should re-trigger sampling decisions because of a name change. Why not re-trigger sampling decisions because of a SetAttribute? Why not re-trigger sampling because of an AddEvent()? There's very little information available at the start of a span, and it's fine to have a sampling policy that uses that information up front. But most interesting sampling decisions happen at the end. IMO the way out of this absurdity is to have two sampling APIs. The first sampling decision is made at the beginning, and it's job is to set IsRecording. The second sampling decision is made at the end, and it has a full and finalized SpanData at its disposal. This is not a priority to me, but if you're planning on using this sampling API you should care.

[paivagustavo]

I agree with you @jmacd, I think this should be covered with the open-telemetry/opentelemetry-specification#307? Do you think that raising these questions there is sufficient for this issue being resolved?

[jmacd]

Thanks @paivagustavo. I see we both just updated the spec issue 307. 😄

[rghetia]

@Aneurysm9 to look at this.

[Aneurysm9]

@jmacd is this still an issue? I don't see an UpdateName() on any of the span types and sdktrace.Span#SetName() already re-runs the sampler.

[paivagustavo]

Yes @Aneurysm9, if a span was not sampled when started, it's data is nil, i.e., the span is a Noop Span, which results in a premature return of the setName(), here.

[Aneurysm9]

Gotcha @paivagustavo. I think I'm in agreement with you and @jmacd then, given the discussion on open-telemetry/opentelemetry-specification#307, that this SDK should implement the simple sampling behavior and thus SetName() shouldn't invoke the sampler at all.

Given that this is blocked on the spec and the issue over there is in the v0.5 milestone, we should probably move this out of our v0.3 milestone.

[evantorrie]

With #1360, the span.data field was eliminated and SetName() now no longer early exits. Instead, the Sampler is always called with the updated samplingData, including the new name.

If it's the case that IsRecording() == false, then the various other span fields such as attributes, links and the span itself are still passing in the samplingData, even though these fields will be unchanged (i.e. still empty) from when the initial sampling decision was made.

[MrAlias]

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

Upon this update, any sampling behavior based on Span name will depend on the implementation.
...
Note that Samplers can only consider information already present during span creation. Any changes done later, including updated span name, cannot change their decisions.

With the goal of forwards compatibility, let's remove this. We can add it back later in a backwards compatible manner.