[processor/tailsampling] fix InvertNotSampled decision precedence when inside and sub policy
#33671
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes the handling of AND policies that contain a sub-policy with invert_match=true. Previously if the decision from a policy evaluation was NotSampled or InvertNotSampled it would return a NotSampled decision regardless, effectively downgrading the result. This was breaking the documented behaviour that inverted decisions should take precedence over all others. This is related to the changes made in open-telemetry#9768 that introduced support for using invert_match within and sub policies.
jamesrwhite
changed the title
InvertNotSampled decision precedence when inside and sub policy
|
Thank you for the PR! This might need a changelog entry: |
jpkrohling
approved these changes
Jun 20, 2024
|
@jpkrohling I've added a changelog entry but I wasn't sure whether to classify this as a breaking change or not? The behaviour now matches the documentation with this change but it could be a breaking change for people if they were relying on the old behaviour. |
jpkrohling
approved these changes
Jun 20, 2024
|
I believe this is a bug fix. |
cparkins
pushed a commit
to AmadeusITGroup/opentelemetry-collector-contrib
that referenced
this pull request
Jul 11, 2024
…hen inside and sub policy (open-telemetry#33671) **Description:** This fixes the handling of AND policies that contain a sub-policy with invert_match=true. Previously if the decision from a policy evaluation was `NotSampled` or `InvertNotSampled` it would return a `NotSampled` decision regardless, effectively downgrading the result. This was breaking the documented behaviour that inverted decisions should take precedence over all others. This is related to the changes made in open-telemetry#9768 that introduced support for using invert_match within and sub policies. **Link to tracking Issue:** open-telemetry#33656 **Testing:** I tested manually that this fixes the issue described in open-telemetry#33656 and also updated the tests. If you have any suggestions for more tests we could add let me know. **Documentation:**
hyang023
pushed a commit
to hyang023/opentelemetry-collector-contrib
that referenced
this pull request
Jul 16, 2024
…edence when inside and sub policy (open-telemetry#33671)" This reverts commit e2fda02.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This fixes the handling of AND policies that contain a sub-policy with invert_match=true. Previously if the decision from a policy evaluation was
NotSampledorInvertNotSampledit would return aNotSampleddecision regardless, effectively downgrading the result.This was breaking the documented behaviour that inverted decisions should take precedence over all others.
This is related to the changes made in #9768 that introduced support for using invert_match within and sub policies.
Link to tracking Issue: #33656
Testing:
I tested manually that this fixes the issue described in #33656 and also updated the tests. If you have any suggestions for more tests we could add let me know.
Documentation: