Allow denylisting in the string-attribute-filter

[majelbstoat]

Currently, the string-attribute-filter in OpenCensus collector allows spans to be exported if they match specified strings, i.e. whitelisted. It is, however, currently not possible to create a blacklist.

One specific use-case for this is to exclude traces for grpc.health.v1.HealthCheck, which are a standard way of adding liveness and readiness endpoints to GRPC servers, but which add lots of noise to trace lists. (The current SpanContext for OpenCensus does not include the span name, so exclusion at that point is not possible either.)

From a flexibility point of view, it would be ideal if there was some sort of simple syntax for including or excluding partial matches too, whether through regexes, globs etc, though latency might be a consideration. At a minimum though, the ability to blacklist certain strings would be great.

Per @songy23, this blocked by open-telemetry/opentelemetry-collector#221.

[ccaraman]

When this does get implemented, please use the term denylist or blockedlist instead of blacklist. Thank you!

[majelbstoat]

Ah, yes! Thank you, title updated. (Description kept to retain context.)

[pedro-ribeiro]

Hi. Do we have any suggestions on how the configuration could look like for this?

[majelbstoat]

here's a starting suggestion based on testdata examples for the tailsampling and attributes processors:

processors:
  tail_sampling:
    decision_wait: 10s
    num_traces: 100
    expected_new_traces_per_sec: 10
    policies:
      include:
        numeric_attribute:
          - key: httpRequest.latency
          - min_value: 1000
       exclude:
         string_attribute:
           - key: route
              values:
                - "/v[0-9]+\.Health\.Check$/"
                - "/^admin/"

This example would include for sampling any span that had a request latency of greater than 1 second. It would exclude anything on a route matching the specified regexes, like grpc.health.v1.Health.Check or admin/dashboard. Include takes precedence over exclude, so slow requests are logged even if they match the exclusions. And any span with a parent that is already sampled should be sampled, as is currently the case.

In general, it might be nice to harmonise the config for these processors as much as possible. The AttributesProcessor runs a pre-process stage that decides whether or not to perform any actions. In this it is quite similar to the TailSamplingProcessor, only differing in what action to take (update/add a key/value vs send/drop the trace). The config to determine to perform this very similar filtering is very different though.

If you're amenable to the spirit of this, I'm happy to have a go at modifying the StringAttributeProcessor to support it. And then we could decide if it makes sense to extract the logic to work for other processors.

[mxiamxia]

Hi @majelbstoat , I am not sure if it is a good idea to have denylist in tailsamplingprocessor, we may only want to keep the sampling processor in a simple responsibility that only take care of sampling job on the interested data. If this issue is to ignore the noise tracing data. The attributesprocessor should perfectly handle this job.
https://github.com/open-telemetry/opentelemetry-collector/blob/66d460700e7fd8d316c15cc7bb815f9ddb353649/internal/processor/filterconfig/config.go#L45

I also sent a PR to enhance string-attribute-filter config in tailsamplingprocessor to support regex rules but I don't feel it's a good idea to add exclude | denylist in string-attribute-filter

[spaletta]

Hi @mxiamxia, from my understanding, filtering with the attributes processor (or the span processor) can not achieve the same effect as filtering in the tailsampling processor. Attributes and span processors can include or exclude individual spans based on their individual properties, while filtering in the tailsampling processor includes or excludes whole traces based on any of the comprising spans.
Noise rarely comes as individual spans only, and so this is a crucial difference. The insufficient filtering capabilities for complete traces (which arguably could be separated from the tailsampling processor) have required me to run a patched version of the tailsampling processor with inverted matching logic at $work for several months now.

[alolita]

@mxiamxia are you working on this issue? Will re-assign otherwise. Lmk. Thx.

[alolita]

@mxiamxia are you still planning to submit a PR for this issue? Else I will re-assign. Let me know. Thx.

[shivanshuraj1333]

I think the issue isn't progressing, I'd love to come up with a fix for this.
@alolita can I work on it?
Thanks!!

[Chinwendu20]

Heyy, is this issue still valid?

[mattsains]

I was looking into this issue and I have some questions.

Firstly, I was wondering if having denylisting is functionally the same as string_attribute's invert_match field: #4393

I think you can rewrite @majelbstoat 's example like this: (which predates the invert_match feature btw)

processors:
  tail_sampling:
    decision_wait: 10s
    num_traces: 100
    expected_new_traces_per_sec: 10
    policies:
      - numeric_attribute:
          key: httpRequest.latency
          min_value: 1000
      - string_attribute:
          key: route
          values:
            - "/v[0-9]+\.Health\.Check$/"
            - "/^admin/"
          invert_match: true  // <--- inverts the rule and turns it into a deny list

Currently, it seems that invert_match is only available for string_attribute, but it seems like we could add it to the other policy types as well.

The benefit of doing this instead of adding include/exclude fields is that it's backward compatible (moving policies into a new attribute — ie., into include — might be disruptive to existing processor configs). I also feel like people might get confused with two ways to negate a rule if we added an exclude list.

I'm new to this project, so let me know if I've misunderstood something. Would also appreciate some direction on what to do next. Thanks!

[github-actions]

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 @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.