[processor/tailsampling] invert_match not given precedence when inside and policy #33656
Comments
jamesrwhite
added
bug
|
Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
|
@jamesrwhite, thank you for the detailed report! I agree with your assessment: the and policy should definitely return the original |
|
@jpkrohling thanks for taking a look at this so soon, I'll open a PR shortly 👍 |
…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:**
|
It looks like this was resolved by #33671, but please correct me if I'm missing something. |
|
I'm reopening this given what we learned in #34085. |
|
I've recently started migrating our Honeycomb based stack to Grafana Tempo and now I'm leveraging I've been following this trail of issues #34085 , #33671 and at least for me it seems we're running into this overcomplication because we're missing a |
|
This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping Pinging code owners:
See Adding Labels via Comments if you do not have permissions to add labels yourself. |
|
We have almost exactly the same scenario as described in the original issue (a string attribute and span count should result in not being sampled). Given that the proposed fix for this was reverted is there some other way to get around this for now? |
#### Description
This pull-request introduces 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. The `Dropped` decision is not new, however, it was
unused and therefor safe to use for this purpose. This new policy type
is very **approachable** and capable.
This new policy type can be used to replace the top-level (not within an
`and` policy) use of `invert_match` to control the final sampling
decision. We could deprecate the current `invert_match` top-level
decision logic. 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.
Example:
```
{
name: drop-it-like-its-hot,
type: drop,
drop: {
drop_sub_policy:
[
{
name: do-not-sample,
type: boolean_attribute,
boolean_attribute: { key: app.do_not_sample, value: true },
}
]
}
}
```
This is a reduced version of a previous PR
#37760
#### Related Issues
-
#36673
-
#33656
-
#36795
-
#34296
-
#34085
-
#29637
-
#27049
- Probably more 😅
---------
Signed-off-by: Sean Porter <[email protected]>
Component(s)
processor/tailsampling
What happened?
Description
From my understanding of the
tailsamplingprocessorREADME whenever an "inverted not sample" decision is made that trace should not be sampled, regardless of any other policies. Specifically it states:The "inverted not sample" decision having the highest precedence seems to be confirmed by how this is actually implemented here.
Based upon that I have been trying to put together a policy similar to the following:
In practice our policy is more complicated than this but I've simplified it as much as possible for the purpose of explaining this issue.
The policy looks like this in config:
What I'm finding in practice though is that traces with the
jobattribute set toboringthat only contain a single span are still being sampled. Unless I'm misunderstanding how the tail sampling processor is working this seems like a bug to me.I debugged this a bit by adding several extra log statements into the processor to see what decisions it was making on each policy and I believe the issue lies in how AND policies handled not sampled decisions here.
As you can see regardless of whether the decision is
NotSampledorInvertNotSampledit always returnsNotSampled. This is problematic because aNotSampleddecision won't take precedence over aSampleddecision so anyInverNotSampleddecision that occurs within an AND policy is effectively getting downgraded to aNotSampleddecision.If it's useful for confirming this behaviour these were the extra log statements I added:
I believe the fix for this would be to return the
decisionso either theNotSampledorInvertNotSampledare returned respectively. I have tested this and it seems to fix the problem.I've made those changes in a branch here: main...jamesrwhite:opentelemetry-collector-contrib:fix-and-policy-invert-not-sampled.
I'm happy to open a PR with the above changes if you agree that's the correct fix, also if you need any more information please let me know.
Thanks!
Steps to Reproduce
Expected Result
Trace is not sampled.
Actual Result
Trace is sampled.
Collector version
0.102.0-dev
Environment information
Environment
OS: MacOS 14.5
Go: 1.21.0
Python: 3.12.4
OpenTelemetry Collector configuration
Log output
Additional context
This is related to support being added for
invert_matchin AND policies in #9768The text was updated successfully, but these errors were encountered: