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] Replace invert sampled with drop policy #37760

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

portertech
Copy link
Contributor

@portertech portertech commented Feb 6, 2025

Description

This pull-request replaces the use of InvertSampled and InvertNotSampled decisions with a new policy type to explicitly drop traces regardless of other policy decisions. The new drop policy type behaves much like an and, however, it returns a Dropped decision when all of its policies return Sampled. A Dropped decision takes precedence over all others. Other policy types will either return Sampled or NotSampled. The string, numeric, and boolean filter policies still support invert_match, which continues to flip the decision for the individual policy. Let invert_match be simple.

This is a breaking change, as existing top-level (not within an and or composite) string, numeric, and boolean filter policies using invert_match will no longer have the same impact on the final decision. These policies will need to be replaced with a drop, effectively wrapping the existing policy.

Using an example invert_match policy from the readme:

{
    name: drop-it-like-its-hot,
    type: drop,
    drop: {
        drop_sub_policy: 
        [
            {
                name: team_a-do-not-sample,
                type: boolean_attribute,
                boolean_attribute: { key: app.do_not_sample, value: true, invert_match: true },
            }
        ]
    }
}

This pull-request also optimizes decision policy evaluation. It will make an earlier decision when drop policy is not in use, making a decision on the first Sampled. This reduces tick/decision latency, leading to improved stability and performance.

Related Issues

@github-actions github-actions bot added the processor/tailsampling Tail sampling processor label Feb 6, 2025
@github-actions github-actions bot requested a review from jpkrohling February 6, 2025 20:47
@portertech
Copy link
Contributor Author

@rjtferreira I am curious to know what you think of this.

@portertech
Copy link
Contributor Author

@jpkrohling @evan-bradley I would love your input on this 🙏

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I'm the first to say that the invert logic is messed up, but unfortunately, it's used in a lot of places. There are a couple of things I'd like to have:

  • either as part of this PR or coming before this PR: a set of examples with their expected outcomes. We have some already, but I feel like we need a real database of examples for people to understand the scenarios. Bonus points if we can make them runnable/tested as part of the CI. Having real use-cases and examples for the policies and the implications when applied together will make it easier for people to use and report bugs.
  • this PR definitely needs a feature flag, to enable the new behavior without affecting current users. Take a look at our deprecation policy, but in short: current behavior is the default behavior for two more versions, we warn users about the upcoming behavior change on the next version already, we change the default behavior after N+2, and we remove the feature flag and the old behavior in a few month's time.

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.

2 participants