Skip to content
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 warnings about Span.UpdateName #754

Merged
merged 3 commits into from
Aug 3, 2020

Conversation

iNikem
Copy link
Contributor

@iNikem iNikem commented Jul 31, 2020

Fixes #468

Remove warnings against using Span.UpdateName. Adds notes about Samplers being unable to recalculate their decision after Span.UpdateName or Span.SetAttribute.

This PR somewhat duplicates #506 submitted by @toumorokoshi . I made new PR because that one gone stale and author does not answer.

Citing myself from #506 (comment)

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.

@iNikem iNikem requested review from a team July 31, 2020 14:20
Comment on lines 418 to 419
Note that [samplers](sdk.md#sampler) can only consider information already
present during span creation. Any changes done later, including new or changed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is accurate, especially given open-telemetry/oteps#115

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reflects current state of the spec:

Sampler interface allows users to create custom samplers which will return a sampling SamplingResult based on information that is typically available just before the Span was created.

I cannot propose a PR to spec based on not yet merged OTEP, can I?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why remove the warning? Until that OTEP is implemented, samplers cannot react to changes in the span name, can they?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was discussed at length in #506 . Span.updateName has legit use-cases and having this warning confuses people: "why have a method but discourage to use it?" We cannot remove the method, thus I propose to remove the warning.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, the discussion in OTEP 115 clarifies that there are two distinct kinds of sampling decision. There is a head sampling decision that is irrevocable, made when the span starts, but this is only a provisional decision to begin gathering a span. There is still a tail sampling decision to be made that can incorporate new information, and we shouldn't mix terms and call these both a sampling decision.

This conversation appears to relate to the first decision, which practically by definition can't be changed because it's history by the time any new information arrives. I would just be clear that when we say "Sampler" (capital "S") we are referring to an initial decision that can't be revoked, not a final decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with that. And link here to "samplers" as defined in SDK spec. I can change "sampler" to "Samplers" if that will make it clearer :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, @yurishkuro , I would like to point your attention that this PR solves issue #468 which is triaged for GA and assigned P1. If you disagree with this, that issue would be a better place to try and change this decision.

@carlosalberto carlosalberto self-requested a review August 3, 2020 12:34
@carlosalberto carlosalberto merged commit fcc4ac6 into open-telemetry:master Aug 3, 2020
@iNikem iNikem deleted the updateName branch August 3, 2020 14:36
carlosalberto pushed a commit to carlosalberto/opentelemetry-specification that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove trace.UpdateName from the spec, or remove warnings against using it
6 participants