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

ddtrace/tracer: allow changes of priority even when the root is non-local #1241

Merged
merged 4 commits into from Apr 28, 2022

Conversation

dumontg
Copy link
Contributor

@dumontg dumontg commented Apr 11, 2022

The tracer was not allowing the user to override the sampling decision
in downstream services, causing a manual keep to not work for traces
where the root span wasn't local.

Notes:

  • Running the new tests without the change in ddtrace/tracer/spancontext.go will fail. Those are an easy way to reproduce the issue on your end.
  • This is my first contribution so please be extra cautious when approving this change.

…ocal

The tracer was not allowing the user to override the sampling decision
in downstream services, causing a manual keep to not work for traces
where the root span wasn't local.
@dumontg dumontg requested a review from a team April 11, 2022 18:30
@gbbr
Copy link
Contributor

gbbr commented Apr 12, 2022

IIRC the sampling decision for a trace can only be made with the initial chunk and it applies to the whole distributed trace. It is not allowed to set different sampling decisions for different parts of a trace. I'm pretty sure this was intended, as the comment you are trying to delete mentions too.

@dumontg
Copy link
Contributor Author

dumontg commented Apr 12, 2022

IIRC the sampling decision for a trace can only be made with the initial chunk and it applies to the whole distributed trace. It is not allowed to set different sampling decisions for different parts of a trace. I'm pretty sure this was intended, as the comment you are trying to delete mentions too.

Right, I probably should not have called it an issue but rather a requested behavior change, as you mentioned the intent behind the code is quite clear. The public documentation implies that if the user uses a manual keep/drop, the tracer will do its best to enforce it. It would be super useful to be allowed to keep specific errors occurring in downstream services, even if the resulting trace will be incomplete. Likewise, being able to drop uninteresting parts of traces explicitly would be nice.

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

This is fine. But left some comments to make sure we're aligned with the rest of the repository style (https://github.com/DataDog/dd-trace-go/wiki/Style-guidelines)

ddtrace/tracer/span_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/span_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/spancontext_test.go Outdated Show resolved Hide resolved
dumontg and others added 2 commits April 21, 2022 17:17
@dumontg
Copy link
Contributor Author

dumontg commented Apr 21, 2022

This is fine. But left some comments to make sure we're aligned with the rest of the repository style (https://github.com/DataDog/dd-trace-go/wiki/Style-guidelines)

Thanks for all the comments/suggestions! I believe I addressed all of them. Also:

  • Not sure how to get the CircleCI to pass
  • Are you ok with rebase+merge?

@ajgajg1134 ajgajg1134 added this to the 1.39.0 milestone Apr 21, 2022
@dianashevchenko
Copy link
Contributor

@dumontg 🙌 and yes

@gbbr gbbr merged commit 11df452 into v1 Apr 28, 2022
@gbbr gbbr deleted the guillaume.dumont/fix-manual-keep-non-local-root branch April 28, 2022 13:21
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.

None yet

4 participants