Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add expr pdata.Metric filtering support to filterprocessor 2/2 #1996

Merged
merged 5 commits into from
Oct 29, 2020
Merged

Add expr pdata.Metric filtering support to filterprocessor 2/2 #1996

merged 5 commits into from
Oct 29, 2020

Conversation

pmcollins
Copy link
Member

@pmcollins pmcollins commented Oct 21, 2020

This is the second part of the expr metric filtering implementation.

The first PR is here (the description has some additional explanation): #1940

There's also usage info in the README.

@codecov
Copy link

codecov bot commented Oct 21, 2020

Codecov Report

Merging #1996 into master will increase coverage by 0.07%.
The diff coverage is 97.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1996      +/-   ##
==========================================
+ Coverage   91.49%   91.57%   +0.07%     
==========================================
  Files         282      285       +3     
  Lines       16631    16784     +153     
==========================================
+ Hits        15217    15370     +153     
  Misses        978      978              
  Partials      436      436              
Impacted Files Coverage Δ
internal/processor/filtermetric/expr_matcher.go 83.33% <83.33%> (ø)
processor/filterprocessor/filter_processor.go 91.66% <95.00%> (+1.19%) ⬆️
internal/processor/filterexpr/matcher.go 100.00% <100.00%> (ø)
internal/processor/filtermetric/filtermetric.go 100.00% <100.00%> (ø)
internal/processor/filtermetric/name_matcher.go 100.00% <100.00%> (ø)
processor/filterprocessor/factory.go 100.00% <100.00%> (ø)
translator/internaldata/resource_to_oc.go 94.52% <0.00%> (+5.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96a4455...9c64168. Read the comment docs.

@pmcollins pmcollins marked this pull request as ready for review October 22, 2020 17:42
@pmcollins pmcollins requested a review from a team October 22, 2020 17:42
@pmcollins pmcollins changed the title Implement expr metric filtering Add expr pdata.Metric filtering support to filterprocessor 2/2 Oct 22, 2020
Comment on lines +14 to +15
- Label("foo") == "bar"
- HasLabel("baz")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to support multiple expressions? This is equivalent to AND-ing the expressions, right? Should we just allow a single expressions, like this:

                match_type: expr
                expression: Label("foo") == "bar" && HasLabel("baz")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we could just support a single expression. But the idea was to try to be consistent with the other match types, which do support multiple statements. So, for example, the following two filter processors have the same effect:

processors:
  filter/expr:
    metrics:
      exclude:
        match_type: expr
        expressions:
          - MetricName == "system.cpu.time"
          - MetricName == "system.disk.io"
  filter/strict:
    metrics:
      exclude:
        match_type: strict
        metric_names:
          - system.cpu.time
          - system.disk.io

I think when users think of the expressions in English they might think of them as AND, as in "I want to exclude metrics named 'system.cpu.time' and I want to exclude metrics named 'system.disk.io'", but since the conditions are evaluated for each metric or each datapoint, they are effectively ORed, as in "Is this metric/datapoint named 'system.cpu.time' or is it named 'system.disk.io'? If so, exclude it."

I think in practice users are going to want to filter out "abusive" metrics and set the expression to something like MetricName == "some.metric" && Label("mylabel") == "some_value" then come back later and filter out another abusive metric. I'm thinking that adding another expression requires less thought than figuring out the logical grouping and logical operator, and ultimately ending up with a possibly large, compound statement. Having said that, if we allow multiple expressions, users could still create a single large expression if they wanted.

Either way, I can take a look at the README and see if it could be updated to make this clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please add an example in README to make it clear. Otherwise LGTM but would also want @jrcamp to review the PR.

@tigrannajaryan tigrannajaryan merged commit 469e827 into open-telemetry:master Oct 29, 2020
@pmcollins pmcollins deleted the filterproc-expr-impl branch January 13, 2021 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants