remove warnings for trace.UpdateName

[toumorokoshi]

Fixes #468

The section discouraging trace.UpdateName clear in terms of the reasoning,
but practically it's common for consumers to not read the full specification
or documentation. As such, one should expect that UpdateName use will not
be uncommon, and those who process spans cannot rely on the span name being
immutable.

Removing the section discouraging it, and adding a section to the samplers
themselves to ensure that samplers are aware of mutable span names.

[jmacd]

Now that we have normalized its usage, I would propose we replace UpdateName with SetName.

[toumorokoshi]

Now that we have normalized its usage, I would propose we replace UpdateName with SetName.

Yes! Will follow up this PR with a subsequent one for that.

[yurishkuro]

I don't quite follow the reasoning for removing the warning. Samplers have to deal with changing span name anyway, but it may lead to more expensive implementation than in cases where the name is provided upfront. For example, in Jaeger it's possible to configure the sampler to expect that the name won't change after creation, leading to a more efficient operation. And in many cases instrumentation does have options for not relying on UpdateName, e.g. by deferring span creation. So I feel that the paragraph discouraging the use of UpdateName was useful.

[jmacd]

I want to say that this issue is not very important to me, but looks like a major wrinkle from the user's perspective.

I don't like the way that name has been made out to be a special case. It's true that some applications cannot know their name until after the span starts, but it's also true that some span attributes are not known until after the span starts. We caution in the specification that span attributes set after start may not be used in the sampling decision, but we carve out a special case for span names.

If a span attribute or the span name can change multiple times before it ends, and you wish to continually update the sampling decision, then what you're describing is tail sampling. The cost of tail sampling is too high, you say? That's why we have head sampling, it's simple and cheap.

The complaint is that head sampling is what you want except for the irritating fact that sometimes a name is not known at the head of a span. If head sampling isn't working, either use tail sampling or use a more sophisticated Sampler API (*). By creating an UpdateName API with a special caveat about its use, you're pushing this complexity to the user.

(*) For example, a Sampler API lies on a spectrum between head sampling and tail sampling; it would have three decision states: YES, NO, and MAYBE. The sampling decision should return MAYBE until it a decision can be made, and the SDK should buffer span data until the decision is reached. This kind of sampler logic is definitely possible, but they're complex--they start to look like solvers for a logic program.

[Oberon00]

Another alternative is to allow not setting the name at start when it is known that the final name is unknown.

[jmacd]

Yes. I would be much more comfortable if we allowed the name to be set exactly once for the Sampler--either when the span is started or at one later point in time. Then, just like SetAttribute can override an existing value, SetName might update the name but not reconsider the sampling decision. Then, if the Sampler wants to use the span name, and the span name is not given at Start, the span can defer any decision making until the span name is set once. If you really want to update the sampling decision more than once, it's called tail sampling.

[c24t]

The change LGTM for the reasons @jmacd points out, and in general I think it's better for the spec to describe how implementations ought to be written than how users ought to use them. That is, if we provide an UpdateName API method the onus should be on the author of the sampler to explain that changes to the name won't affect the sampling decision, not on the user to know not to use that particular method.

We caution in the specification that span attributes set after start may not be used in the sampling decision, but we carve out a special case for span names.

We did something similar for links after OTEP 0006. From the user's perspective, it is surprising that the span name and attributes can be changed after creation, but not its links.

[c24t]

Suggested change

	are mutable after it's initial creation, including span attributes and the
	are mutable after its initial creation, including span attributes and the

[toumorokoshi]

And in many cases instrumentation does have options for not relying on UpdateName, e.g. by deferring span creation. So I feel that the paragraph discouraging the use of UpdateName was useful.

It's true in some cases it is possible to ensure that the span is included in the name, by refactoring the code. But I think that could also be said about other things like span attributes as well, which, to @jmacd's point, don't have any special casing around them.

By creating an UpdateName API with a special caveat about its use, you're pushing this complexity to the user.

I think that's the issue, and it goes beyond pushing complexity to the user. Practically speaking, most people will not read the full specification, and will probably use their language's API documentation or some IDE autocomplete to discover UpdateName. So many people will not even be aware of the fact that UpdateName is a discouraged API.

I worry that the existing terminology will make the authors who write sampling code to believe they can rely on reduced usage of UpdateName and handle that case differently, or not at all. That then causes the situation where end users see unexpected sampling logic because of their use of UpdateName, file an issue with the sampler author, and be told that they should not be using that API.

It's a realm of complexity that we can avoid if we push the caveat that the value is mutable into the sampling side.

I don't like the way that name has been made out to be a special case. It's true that some applications cannot know their name until after the span starts, but it's also true that some span attributes are not known until after the span starts. We caution in the specification that span attributes set after start may not be used in the sampling decision, but we carve out a special case for span names.

Agreed. I think that's more impetus to put these caveats on the sampler side.

Another alternative is to allow not setting the name at start when it is known that the final name is unknown.

I think this is hard behavior to enforce in practice, but conceptually that make sense.

[arminru]

What if we rename it to overwriteName to make it even clearer that this is not the intended way of using it? This would also make it obvious to people only reading the method name and they would be more likely to read it up on the doc.

[toumorokoshi]

What if we rename it to overwriteName to make it even clearer that this is not the intended way of using it? This would also make it obvious to people only reading the method name and they would be more likely to read it up on the doc.

For what it's worth, it's not clear to me that "overwriteName" is any more dangerous that "updateName" - might just be adjective classifications I'm not really familiar with.

[toumorokoshi]

I spoke with @bogdandrutu, who asked for some clarification on why we couldn't remove this API from the specification, which would simplify things if possible.

One major use case is when http requests are started before a canonical span name is provided. @SergeyKanzhelev has highlighted this for C#: #468 (comment)

The general idea applies to any language: A common span name to use is the "route" name in the web framework, such as a templatized route /api/v1/user/{user_id}. Because the http request has to at least stream the metadata before that route can be resolved (as the URI is part of the request itself), the order of events are:

1. request starts, data is read (span started)
2. once metadata is read, resolve route (updateName)
3. requests ends, all data has been read (span stopped)

The alternative to providing updateName is to store the start time and other data in some object, and use that in the creation of the span once metadata is read. To enable that, that would require a pattern where such span data is stored is some temporary object (probably in the context) before span creation. IMO this is a much more complex pattern to facilitate, and would then require a second convention on where to store this span data pre-span creation.

Next Steps

As I understand it, the main concern is the fact that the Sampler API is designed such that it is not possible to re-evaluate the early sampling decision based on changes to the span (or it is expensive to do so).

I think this can come in as a separate PR, but how about adding APIs to the Sampler in the spec to handle changes to the span as they come in? That would handle a currently unhandled edge case of span attributes changing throughout the lifetime of the span too. This is especially useful on things like response code, which is unknown until near the span end itself.

[toumorokoshi]

@SergeyKanzhelev question for you: before this PR, the strategy to store the span start time and overwrite start time was called out in the spec:

Alternatives for the name update may be late
Span creation, when Span is started with the explicit timestamp from the past
at the moment where the final Span name is known, or reporting a Span with
the desired name as a child Span.

It sounds like ASP.net has explicitly chosen to use UpdateName over this strategy. What was the rationale? If we don't have official opentelemetry packages following the recommendations in the spec, I feel like we need to change the spec, or the implementation.

[Oberon00]

FWIW, Python's flask instrumentation also uses the store timestamp + start later approach and has to go to quite some lengths (boilerplate-wise) for that. EDIT: But I view that as a Quality-of-Implementation thing. You can take the shortcut of calling UpdateName to get more maintainable instrumentation code, or you can write some more complicated, longer instrumentation to be able to provide better information to the sampler.

[Oberon00]

@toumorokoshi

how about adding APIs to the Sampler in the spec to handle changes to the span as they come in

This opens a can of worms, like what to do if child spans with that span's ID as parent have already been created (even exported) and now that span is sampled away? The parent-child chain in the trace would be broken. EDIT: #307 (still open) discusses this.

[yurishkuro]

I agree with @Oberon00 - the real issue is discussed in #307, without a resolution there the debates about UpdateName are not fruitful.

[iNikem]

Please, can we keep the scope of our discussion to the scope of this PR?

I support the opinion that we cannot remove Span.updateName completely, because doing that will make many instrumentations much more complicated. As was demonstrated several times both in this PR and in the related task, there are valid use-case where "good" span name is unknown until after span is created.

I don't think that span name differs in any way from any other span attribute from Sampler perspective. So if sampler deals somehow with attribute changes, it has to deal with span name change as well.

This comment from @c24t summarises it very nicely:

it's better for the spec to describe how implementations ought to be written than how users ought to use them. That is, if we provide an UpdateName API method the onus should be on the author of the sampler to explain that changes to the name won't affect the sampling decision, not on the user to know not to use that particular method.

For these reasons I think we have to merge this PR. As long as we have Span.updateName method in our API we should NOT discourage its usage. If later we decide to remove that method, that will be done in later and separately. Current PR does not depend on that future potential.

@toumorokoshi Can you please rebase the PR?

@open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers If you agree with this PR and my argument above, please approve this PR.

[Oberon00]

What does "consider" mean here? I don't think samplers can sensibly do anything about this. I think we should rather write for SetAttribute and UpdateName (I think we have something like that in the span creation API spec already):

Note that samplers can only consider information already set during span creation. Any changes done later cannot be considered for sampling.

[carlosalberto]

Closing on behalf of #754