Skip to content

Commit

Permalink
[processor/tailsampling] fix InvertNotSampled decision precedence w…
Browse files Browse the repository at this point in the history
…hen inside and sub policy (#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
#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:**
  • Loading branch information
jamesrwhite authored Jun 20, 2024
1 parent 40b334a commit e2fda02
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 3 deletions.
9 changes: 9 additions & 0 deletions .chloggen/fix-tailsampling-and-policy-precedence.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
change_type: bug_fix
component: tailsamplingprocessor
note: Fix precedence of inverted match in and policy
issues: [33671]
subtext: |
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.
change_logs: [user]
4 changes: 2 additions & 2 deletions processor/tailsamplingprocessor/internal/sampling/and.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ func NewAnd(
// Evaluate looks at the trace data and returns a corresponding SamplingDecision.
func (c *And) Evaluate(ctx context.Context, traceID pcommon.TraceID, trace *TraceData) (Decision, error) {
// The policy iterates over all sub-policies and returns Sampled if all sub-policies returned a Sampled Decision.
// If any subpolicy returns NotSampled, it returns NotSampled Decision.
// If any subpolicy returns NotSampled or InvertNotSampled it returns that
for _, sub := range c.subpolicies {
decision, err := sub.Evaluate(ctx, traceID, trace)
if err != nil {
return Unspecified, err
}
if decision == NotSampled || decision == InvertNotSampled {
return NotSampled, nil
return decision, nil
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,6 @@ func TestAndEvaluatorStringInvertNotSampled(t *testing.T) {
}
decision, err := and.Evaluate(context.Background(), traceID, trace)
require.NoError(t, err, "Failed to evaluate and policy: %v", err)
assert.Equal(t, decision, NotSampled)
assert.Equal(t, decision, InvertNotSampled)

}

0 comments on commit e2fda02

Please sign in to comment.