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

[trace-sdk-sampler] Differentiate between root and child span creation in trace sampling spec #3888

Closed

Conversation

CodeBlanch
Copy link
Member

Changes

  • Allow SDKs to bypass span creation for children.

Reasoning

The .NET SDK works like this today: If the sampler returns DROP we always create a span for the root. For children, we don't create a span. This takes .NET out of compliance with the spec (as-is) but we have been resistant to change this behavior because it is expensive to create these spans (memory, cpu, gc pressure, etc.) for benefits which seem unclear. If the purpose is just to give a valid non-empty SpanId to logs (and friends) then the root creation behavior seems to accomplish the goal more cheaply.

Also, .NET also already has a mechanism which is more or less #3867 where users can drop a source/instrumentation scope. Really everything is no-op by default users have to explicitly enable the sources/instrumentation scopes they care about. Even if .NET was spec-compliant w.r.t. the span creation traces could still have gaps for things users haven't switched on. This isn't something .NET SDK can change either, as it comes from the underlying .NET platform.

What this PR is seeking to do is officially bless what .NET has done in order to bring it into spec-compliance so we may resolve things like open-telemetry/opentelemetry-dotnet#3290.

TODOs

Sorry, something went wrong.

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.
3. If there isn't currently a span (the span being created will become the root
Copy link
Member

Choose a reason for hiding this comment

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

@Oberon00 to share early comments. (the PR is still draft)

CodeBlanch and others added 2 commits February 27, 2024 15:15

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
Copy link

github-actions bot commented Mar 6, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 6, 2024
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 13, 2024
@CodeBlanch
Copy link
Member Author

@cijothomas Are you able to reopen this? Doesn't seem like I can do it.

@cijothomas cijothomas reopened this Mar 18, 2024
@CodeBlanch CodeBlanch marked this pull request as ready for review March 18, 2024 17:20
@CodeBlanch CodeBlanch requested review from a team March 18, 2024 17:20
@github-actions github-actions bot removed the Stale label Mar 19, 2024
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 27, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 10, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 18, 2024
@Oberon00
Copy link
Member

While the change seems sensible (without thinking very deep about it), it is also a breaking change to a stable specification part.

@cijothomas
Copy link
Member

While the change seems sensible (without thinking very deep about it), it is also a breaking change to a stable specification part.

I think the proposed wording in this PR is written is such a way so as to be back-compatible, very similar to the OTLP Exporter defaulting to http wording from diff. section of spec.

If no configuration is provided the default transport SHOULD be http/protobuf unless SDKs have good reasons to choose grpc as the default (e.g. for backward compatibility reasons when grpc was already the default in a stable SDK release).

@cijothomas cijothomas reopened this Apr 18, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@github-actions github-actions bot removed the Stale label Apr 19, 2024
@CodeBlanch
Copy link
Member Author

@Oberon00

it is also a breaking change to a stable specification part

@jack-berg Had the same feedback for me when I first shared this idea with him. We are in no way recommending SDKs change their behavior. What we want to do is retroactively bless the direction .NET has already taken.

I just pushed an update which makes this explicit. Feels very unnecessary to me, but if it helps makes this clear doesn't seem like a problem to have it.

@Oberon00
Copy link
Member

It's OK, you don't need to add this wording just for me, as long as we have something like that also in the general versioning/stability spec it should be fine (and I think we have, now that you brought it up)

@arminru arminru added area:sampling Related to trace sampling spec:trace Related to the specification/trace directory labels Apr 23, 2024
Copy link

github-actions bot commented May 1, 2024

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 1, 2024
cijothomas added 2 commits May 6, 2024 09:06

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sampling Related to trace sampling spec:trace Related to the specification/trace directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants