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 filtering option for prometheus collector #16420

Merged
merged 14 commits into from
Feb 24, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Feb 19, 2020

Drafting what was discussed on #15493 to start implementation specific discussions.For the time being only filters are implemented, but mappings will be added if we agree on this.
@jsoriano I preferred to add the processing on collector layer to avoid touching GetFamilies since it is used by many other places like the state_* metricsets of k8s module. Not sure if we would like to have it in there (in the helper method).

What does this PR do?

This PR introduces filtering options for prometheus collector metricset.

Why is it important?

This is important when someone want to filter out metrics that come in a single response for instance in case of prometheus exporters.

This will be useful in light-modules like ibmmq module where currently we use script processor to filter out metrics as well as in OpenMetrics module where a user may want to explicitly define which metrics want to keep.

How to test this PR locally

  1. One can use a nodexporter as an example Prometheus app to collect from. For instance this forked project should be enough: https://github.com/ChrsMark/dockprom. Download and docker-compose up. Then metrics should be exported at http://localhost:9100/metrics.
  2. Configuration:
- module: prometheus
  period: 10s
  hosts: ["localhost:9100"]
  metrics_path: /metrics
  metrics_filters:
    include: ["node_filesystem_*"]
    exclude: ["node_filesystem_device_*", "^node_filesystem_readonly$"]

And expect to see only events that match node_filesystem_* but also not node_filesystem_device_* and are not node_filesystem_readonly.
3. Try to remove ^node_filesystem_readonly$ from exclude filters and see that this metric is included in events. One can try different combinations.

Related issues

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark requested a review from a team as a code owner February 19, 2020 14:48
@ChrsMark ChrsMark self-assigned this Feb 19, 2020
@ChrsMark ChrsMark added enhancement in progress Pull request is currently in progress. Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team labels Feb 19, 2020
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

This approach LGTM, I agree with limiting it to filtering by now, and we can consider adding mapping in the future.

I think we could accept using include_metrics and ignore_metrics at the same time, I added a comment about that.

I preferred to add the processing on collector layer to avoid touching GetFamilies since it is used by many other places like the state_* metricsets of k8s module. Not sure if we would like to have it in there (in the helper method).

👍 we can revisit this later if needed.

metricbeat/module/prometheus/collector/config.go Outdated Show resolved Hide resolved
metricbeat/module/prometheus/collector/collector.go Outdated Show resolved Hide resolved
metricbeat/module/prometheus/collector/collector.go Outdated Show resolved Hide resolved
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

A couple of comments more, and I'd be ok with continuing with this proposed implementation. Thanks!

@ChrsMark
Copy link
Member Author

Thanks for reviewing! I will work on the comments and I also plan to add some unit-tests maybe for the helper functions so as to secure the logic.

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

ChrsMark commented Feb 21, 2020

Giving a heads-up on this:

Still need to update the docs with these new settings description.

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added [zube]: In Review test-plan Add this PR to be manual test plan v7.7.0 needs_backport PR is waiting to be backported to other branches. and removed [zube]: In Progress in progress Pull request is currently in progress. labels Feb 24, 2020
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark merged commit db85050 into elastic:master Feb 24, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Feb 24, 2020
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Feb 24, 2020
ChrsMark added a commit that referenced this pull request Feb 24, 2020
@andresrc andresrc added the test-plan-added This PR has been added to the test plan label Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team test-plan Add this PR to be manual test plan test-plan-added This PR has been added to the test plan v7.7.0 [zube]: Done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants