-
Notifications
You must be signed in to change notification settings - Fork 897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove trace.UpdateName from the spec, or remove warnings against using it #468
Comments
Removing update from API would make semantically clear implementation impossible, at least for C#. It will force create an extra span and do more tricks on span "End". I'd vote for improving and clarifying the implementation. Now sure if we want to make it before 1.0 though. |
I assigned the milestone "Future" for now. |
Got it, good to know. Is there some code that would help me better understand that? (out of curiosity primarily). I also see some issues with losing the API on the Python side. If that's the case, are there any arguments for discouraging UpdateName use? It seems like consumers would have to handle the UpdateName case. |
ASP.NET gives three events (oversimplifying) - We decided for now to now re-apply sampling. If sampling by span The decision above is not straightforward and will definitely cause issues for advanced users. So the note might help. And we don't want this pattern to be used more widely than required. |
Yes, that makes sense. That's similar to some of the reasoning on the python side too (the actual request starts before the http route is resolved).
I think it makes more sense to remove the note about discouraging UpdateName, and instead just add more information in the sampling section about span name not being a reliable value to sample on. The existence of the UpdateName API means that, no matter what, SDK authors would have to assume that some consumer will update their name at some point, and act accordingly. As such I'd argue it's more of a consumer of span concern (SDK, processor, sampler) rather than an span producer concern (API). |
I would prefer if we called this I see the semantics of the Sampler API with respect to |
yeah, I see that point. SetName seems fine to me too. I'm going to start a PR to at least remove warnings and include this as a caveat in the sampler side. |
Fixes open-telemetry#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.
Hi,
I'd like to propose that the specification make a clear decision about UpdateName, rather than introduce the API, but discourage it's use. The current specification reads:
The section is 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 as such those who process spans cannot rely on the span name being immutable. At that point, all consumers of spans should assume that the span name can be modified, and update their code accordingly. If all consumers handle that case, there's not much value in discouraging the use the API.
I think it would be more helpful for span publishers and consumers to make a decision in one direction or the other:
The text was updated successfully, but these errors were encountered: