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.

Sign up for GitHub

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

Jump to bottom

Add `metric_patterns` options to filter all metric submission with a list of regexes #11508

Merged

sarah-witt merged 16 commits into master from sarah/filter-metrics-regex

Mar 11, 2022

Contributor

sarah-witt commented Feb 15, 2022 •

edited

Loading

What does this PR do?

Adds instance-level options to the base check to filter all metric submission with a regex. The main option is metric_patterns with two sub-options are exclude and include.

Motivation

Allow users to have more control over what metrics are submitted for each integration

Additional Notes

These behave very similarly to exclude_labels and include_labels, where exclude takes precedence over include
These can either be strings or regexes.
They also use the same regex construction as exclude_metrics
The namespace is included in the regex
I could add a debug log line if a metric is skipped, but that would be noisy

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
PR title must be written as a CHANGELOG entry (see why)
Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
PR must have changelog/ and integration/ labels attached


          Add option for global metrics filter

63d1d88

ghost added the integration/datadog_checks_base label

sarah-witt added the changelog/Added label

codecov bot commented Feb 15, 2022 •

edited

Loading

Codecov Report

Merging #11508 (02b30f8) into master (31548f9) will increase coverage by 0.26%.
The diff coverage is 98.19%.

Flag	Coverage Δ
activemq_xml	`82.31% <ø> (ø)`
ambari	`85.75% <ø> (ø)`
avi_vantage	`91.92% <ø> (ø)`
btrfs	`82.91% <ø> (ø)`
coredns	`94.54% <ø> (ø)`
directory	`94.87% <ø> (ø)`
ibm_i	`80.65% <ø> (ø)`
kube_proxy	`100.00% <ø> (ø)`
kube_scheduler	`96.20% <ø> (ø)`
kubelet	`90.42% <ø> (+0.79%)`	⬆️
kyototycoon	`85.96% <ø> (ø)`
linkerd	`85.14% <ø> (+1.14%)`	⬆️
nagios	`89.53% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

sarah-witt marked this pull request as ready for review

February 15, 2022 22:24

sarah-witt requested review from a team as code owners

February 15, 2022 22:24


          Filter before context limiter

87e2933

vickenty reviewed

View reviewed changes

Contributor

vickenty left a comment

Writing a single regular expression to match a collection of strings might not be the best UX for this. Would it make sense to let the user specify a list of glob patterns instead? (We could still use a compiled regular expression under the hood).

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

sarah-witt added 2 commits

February 16, 2022 14:51


          Compile regex and check for invalid regex

785d0e5


          Add test for invalid regex

595e5f1

sarah-witt requested a review from vickenty

February 16, 2022 20:58

fanny-jiang reviewed

View reviewed changes

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

ofek requested changes

View reviewed changes

Contributor

ofek left a comment •

edited

Loading

We should have both an inclusion and exclusion option
They should be a list of patterns as @vickenty and @fanny-jiang suggested
We should save like this

integrations-core/datadog_checks_base/datadog_checks/base/checks/openmetrics/v2/scraper.py

Lines 149 to 150 in 40a66fe

if exclude_metrics_patterns:

self.exclude_metrics_pattern = re.compile('|'.join(exclude_metrics_patterns))

(see Improve performance of pattern matching in OpenMetrics #5764 (comment))

Contributor Author

sarah-witt commented Feb 16, 2022 •

edited

Loading

Ah thanks everyone- I'll update the field to be a list and also model the behavior of exclude_labels and include_labels by having both an inclusion and exclusion list as well as compiling everything faster.

sarah-witt added 4 commits

March 1, 2022 10:31


          Add exclude and include filters

0dec9ab


          move regex logic down, add more tests

0550b10


          Remove space

49db9ff


          Add more invalid tests

7d82582

sarah-witt commented

View reviewed changes

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

sarah-witt changed the title ~~Add option to filter all metric submission with a regex~~ Add options to filter all metric submission with a list of regexes

sarah-witt changed the title ~~Add options to filter all metric submission with a list of regexes~~ Add exclude_metrics_filters and include_metrics_filters options to filter all metric submission with a list of regexes

ofek approved these changes

View reviewed changes

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

ofek requested changes

View reviewed changes

Contributor

ofek left a comment

^

vickenty reviewed

View reviewed changes

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

datadog_checks_base/tests/base/checks/test_agent_check.py Outdated Show resolved Hide resolved

datadog_checks_base/tests/base/checks/test_agent_check.py Show resolved Hide resolved

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

ofek requested changes

View reviewed changes

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved

sarah-witt added 3 commits

March 9, 2022 13:44


          Refactor to helper functions, address comments, and add more tests

8c289e3


          Add another test case

2f6ab9c


          Add test case for include glob

63ad480

ofek requested changes

View reviewed changes

datadog_checks_base/datadog_checks/base/checks/base.py Outdated Show resolved Hide resolved


          Remove duplicate logic and further extract

16129b5

vickenty previously approved these changes

View reviewed changes

Contributor

vickenty left a comment

LGTM, just a small nit / suggestion.

datadog_checks_base/datadog_checks/base/checks/base.py Outdated

+                      if not isinstance(filters, list):
+                          raise ConfigurationError('Setting `{}` must be an array'.format(option_name))
+                      metrics_pattern = None

Contributor

vickenty Mar 10, 2022

Suggested change

metrics_pattern = None

datadog_checks_base/datadog_checks/base/checks/base.py Outdated

Comment on lines 305 to 307

+                      if metrics_patterns:
+                          metrics_pattern = re.compile('|'.join(metrics_patterns))
+                      return metrics_pattern

Contributor

vickenty Mar 10, 2022

Would it make sense to return the values directly, instead of going through a variable?

Suggested change

      
                    if metrics_patterns:
          
                        metrics_pattern = re.compile('|'.join(metrics_patterns))
          
                    return metrics_pattern
          
                    if metrics_patterns:
          
                        return re.compile('|'.join(metrics_patterns))
          
                    return None

Contributor Author

sarah-witt Mar 10, 2022

Sure, I agree it's a little more readable this way


          Don't use metric_pattern veriable

090d93c

sarah-witt dismissed vickenty’s stale review via

090d93c

March 10, 2022 14:51

ofek requested changes

View reviewed changes

Contributor

ofek left a comment

🔥

Last thing, WDYTA naming them (ex|in)clude_metric_patterns?


          Add overall metrics_filters param

d852539

sarah-witt changed the title ~~Add exclude_metrics_filters and include_metrics_filters options to filter all metric submission with a list of regexes~~ Add metric_filters options to filter all metric submission with a list of regexes

Contributor Author

sarah-witt commented Mar 10, 2022

Thanks, updated with the suggestion to have an overall param metric_filters

metric_filters:
  include:
    - ...
  exclude:

sarah-witt added 2 commits

March 10, 2022 14:43


          Update message and tests

dced482


          Rename to metric_patterns

02b30f8

sarah-witt changed the title ~~Add metric_filters options to filter all metric submission with a list of regexes~~ Add metric_patterns options to filter all metric submission with a list of regexes

ofek approved these changes

View reviewed changes

sarah-witt merged commit ddcf5dc into master

sarah-witt deleted the sarah/filter-metrics-regex branch

March 11, 2022 19:12

github-actions bot pushed a commit that referenced this pull request


          Add metric_patterns options to filter all metric submission with a …

21afa04

…list of regexes (#11508)

* Add option for global metrics filter

* Filter before context limiter

* Compile regex and check for invalid regex

* Add test for invalid regex

* Add exclude and include filters

* move regex logic down, add more tests

* Remove space

* Add more invalid tests

* Refactor to helper functions, address comments, and add more tests

* Add another test case

* Add test case for include glob

* Remove duplicate logic and further extract

* Don't use metric_pattern veriable

* Add overall metrics_filters param

* Update message and tests

* Rename to metric_patterns ddcf5dc

This was referenced Mar 11, 2022

[Release] Bumped datadog_checks_base version to 25.1.0 #11651

Closed

[Release] Bumped datadog_checks_base version to 25.1.0 #11683

Merged

Add metric_patterns options to filter all metric submission with a list of regexes #11695

Merged

Add metric_patterns to base template #11696

Merged

ofek mentioned this pull request

Fix metric_patterns option to support namespaces #11795

Merged

fanny-jiang mentioned this pull request

Add ignore_metrics to Prometheus scraper #12713

Closed

4 tasks

CrashLaker commented Sep 18, 2024

hi all. i'm struggling to find how can this be added to the prometheusScrape section in helm values.yaml
can anyone please point me to the right direction/documentation? @fanny-jiang @ofek @sarah-witt
thanks,c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration/datadog_checks_base