[processor/tailsampling] numeric attribute invert match evaluation not prioritized

hyang023 commented 3 months ago

Component(s)

processor/tailsampling

What happened?

Description

as described in the tail sampling processor readme (https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/processor/tailsamplingprocessor),

When there's an "inverted not sample" decision, the trace is not sampled;

this functionality does not work for numeric attribute policies because its Evaluate function only returns Sampled or NotSampled values not InvertNotSampled or InvertSampled even if invertMatch is true

The Evaluate function in https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/tailsamplingprocessor/internal/sampling/numeric_tag_filter.go#L37 needs to be updated so that it can return InvertNotSampled or InvertSampled when appropriate

This would likely require the addition of an invertHasSpanWithCondition in https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/tailsamplingprocessor/internal/sampling/util.go and then calling it from https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/tailsamplingprocessor/internal/sampling/numeric_tag_filter.go if invertMatch is true

Steps to Reproduce

use invert_match in numeric_attribute sampling policy
have other policy where spans that match overlap with the number_attribute invert_match

Expected Result

any trace with spans that invert_match the numeric_attribute should not be sampled

Actual Result

some traces with spans that invert_match the numeric_attribute will still be sampled because they match some other policy

Collector version

v0.105.0 but previous versions would also be affected

Environment information

Environment

this is not environment specific

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response

github-actions[bot] commented 3 months ago

Pinging code owners:

processor/tailsampling: @jpkrohling

See Adding Labels via Comments if you do not have permissions to add labels yourself.

bacherfl commented 2 months ago

I'd be happy to look into this and work on a fix

bacherfl commented 2 months ago

After looking into this, I have two questions related to the policy evaluators:

While the string tag filter checks both the resource and the span attributes (see https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/23de1761434a8d069362cd233cbbbc77725def19/processor/tailsamplingprocessor/internal/sampling/string_tag_filter.go#L102), the numeric filter only considers span attributes, but no resource attributes (see https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/23de1761434a8d069362cd233cbbbc77725def19/processor/tailsamplingprocessor/internal/sampling/numeric_tag_filter.go#L42) - Is this intended, or should the numeric tag filter also take attributes of the resource into account?
Currently, the boolean tag filter seems to not support the inverse_match option at all (see https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/processor/tailsamplingprocessor/internal/sampling/boolean_tag_filter.go) - Should that filter be extended to also suppport this option, or is there a reason to not include this option here? I guess this would need to be addressed in a separate PR as it's not directly related to this issue, but if needed I can also work on this

jpkrohling commented 2 months ago

Is this intended, or should the numeric tag filter also take attributes of the resource into account?

Probably just an oversight/bug.

is there a reason to not include this option here

The invert match was added originally for the string only. In the name of consistency, it would be good to have it for all types though.

bacherfl commented 2 months ago

Is this intended, or should the numeric tag filter also take attributes of the resource into account?

Probably just an oversight/bug.

is there a reason to not include this option here

The invert match was added originally for the string only. In the name of consistency, it would be good to have it for all types though.

Thanks @jpkrohling - In this case I will also consider the resource attributes in the numeric filter and for the boolean filter I would create a separate PR to also support the inverse option there

open-telemetry / opentelemetry-collector-contrib