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.

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

Apply filters on prometheus 'up' metrics #16568

Merged
merged 5 commits into from
Feb 26, 2020
Merged

Apply filters on prometheus 'up' metrics #16568

merged 5 commits into from
Feb 26, 2020

Conversation

ChrsMark
Copy link
Member

Closes #16564

@ChrsMark
Filter 'up' metric in prometheus collector
6526bff 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Update docs
5dabfe4 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
improvements
6cd664e 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added enhancement Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team labels Feb 25, 2020
@ChrsMark ChrsMark requested a review from a team February 25, 2020 16:19
@ChrsMark ChrsMark self-assigned this Feb 25, 2020
@ChrsMark
fixes
f41e541 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
mtojek
mtojek reviewed Feb 26, 2020
View reviewed changes
metricbeat/module/prometheus/collector/collector.go
@@ -173,19 +179,15 @@ func (m *MetricSet) skipFamily(family *dto.MetricFamily) bool {
// This would mean that we want to keep only the metrics that start with node_ prefix but
// are not related to disk so we exclude node_disk_* metrics from them.

if family == nil {
Copy link
Contributor

@mtojek mtojek Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I see what you did here.

I'm not sure if it isn't safer to keep the family == nil check here, but split the whole method body into two methods: skipFamily and skipFamilyName. WDYT? It might prevent next developers from making mistakes (nil structure).

@ChrsMark
fixes
10c448d 
Signed-off-by: chrismark <chrismarkou92@gmail.com>
mtojek
mtojek approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

If it's doesn't blow up any system tests, please consider these changes approved. Thanks!

@ChrsMark ChrsMark added needs_backport PR is waiting to be backported to other branches. v7.7.0 labels Feb 26, 2020
@ChrsMark ChrsMark merged commit ae34415 into elastic:master Feb 26, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Feb 26, 2020
@ChrsMark
Apply filters on prometheus 'up' metrics (elastic#16568)
5be71f8 
(cherry picked from commit ae34415)
@ChrsMark ChrsMark mentioned this pull request Feb 26, 2020
Merged
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Feb 26, 2020
ChrsMark added a commit that referenced this pull request Feb 26, 2020
@ChrsMark
Apply filters on prometheus 'up' metrics (#16568) (#16593)
1a66157 
(cherry picked from commit ae34415)
@dagwieers
Copy link

We cannot make the regexp matching work. So if we want to exclude all metrics ending in e.g. _sum or _max it currently does not seem possible.

This PR also did not add any tests wrt. regexp matching.

@ChrsMark
Copy link
Member Author

We cannot make the regexp matching work. So if we want to exclude all metrics ending in e.g. _sum or _max it currently does not seem possible.

This PR also did not add any tests wrt. regexp matching.

Hi!

Tests had been added on the PR(#16420) that originally added this functionality (https://github.com/elastic/beats/pull/16420/files#diff-5945e5b443352401ece4bbac9bbb653cR205).
In addition this feature is also used by other modules under the hood like redisenterprise

beats/x-pack/metricbeat/module/redisenterprise/proxy/manifest.yml

Line 7 in d01106b

metrics_filters:
and is there are no issues reported so far.

For such questions please refer to https://discuss.elastic.co/tags/c/elastic-stack/81/metricbeat.

@dagwieers
Copy link

There are no regexp tests, I added some myself and they do work correctly locally on the master build. But it does fail to process/match regex excludes using metricbeat 7.9.1 on CentOS (installed using the RPM). Even the example regex (^async_dgcjobs_seconds_sum$) fails to exclude it on our system. No regex matching works (not even .*_sum$).

I will propose a PR adding those regex matching examples to the tests.

@dagwieers dagwieers mentioned this pull request Sep 22, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtojek mtojek mtojek approved these changes

Assignees

@ChrsMark ChrsMark

Labels
enhancement review Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Skip “up” metrics too when not in Prometheus filters
4 participants
@ChrsMark @dagwieers @mtojek @andresrc