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

[processor/tailsampling] record sampling policy #36312

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

djluck
Copy link

@djluck djluck commented Nov 12, 2024

Description

Adds support for optionally recording the policy (and any composite policy) associated with an inclusive tail processor sampling decision:

image

Link to tracking issue

Fixes
Resolves #35180.

Implementation notes

  • This functionality lives behind a feature flag that is disabled by default
  • The original issue described a solution where we might attach the attribute solely to the root span. I'm not sure I agree with the commenter that we can rely on this (e.g. we might decide to sample halfway through a long-running trace) so I have attached the attributes to all present scope spans. This feels like a decent trade off between complexity + network cost, as finding the highest non-root parent would require multiple passes of the spans and keeping all span ids in a set

Testing

  • Added automated tests to verify enabling the flag both records the expected decision while not impacting existing logic
  • Built a custom version and ran it in our preprod environment to ensure it was stable over a 1h period (still evaluating, will update PR with any further observations)

TODO

Does this require a CHANGELOG entry?

Adds support for optionally recording the policy (and any composite policy) associated with an inclusive tail processor sampling decision.

Resolves !35180.

## Implementation notes
- This functionality lives behind a feature flag that is disabled by default
- The original issue described a solution where we might attach the attribute solely to the root span. I'm not sure I agree with the commenter that we can rely on this (e.g. we might decide to sample halfway through a long-running trace) so I have attached the attributes to all present scope spans. This feels like a decent trade off between complexity + network cost, as finding the highest non-root parent would require multiple passes of the spans and keeping all span ids in a set

## Testing
- Added automated tests to verify enabling the flag both records the expected decision while not impacting existing logic
- Built a custom version and ran it in our preprod environment to ensure it was stable over a 1h period (still evaluating, will update PR with any further observations)

## TODO
Does this require a CHANGELOG entry?
@djluck djluck requested review from jpkrohling and a team as code owners November 12, 2024 07:22
Copy link

CLA Missing ID CLA Not Signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
processor/tailsampling Tail sampling processor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add policy to spans in sampled traces
2 participants