Skip to content

Make use of prometheus filter settings on IBM-MQ Metricbeat module#16971

Merged
ChrsMark merged 1 commit intoelastic:masterfrom
ma-hei:16513
Mar 17, 2020
Merged

Make use of prometheus filter settings on IBM-MQ Metricbeat module#16971
ChrsMark merged 1 commit intoelastic:masterfrom
ma-hei:16513

Conversation

@ma-hei
Copy link
Copy Markdown
Contributor

@ma-hei ma-hei commented Mar 12, 2020

What does this PR do?

Refactoring the manifest file of the qmgr metricset. Previously a processor was used to filter out non ibmmq metrics. This commit replaces the processor with the more elegant metrics_filter that was introduced recently.

Why is it important?

Using the metrics_filter is cleaner and easier to read.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works

How to test this PR locally

  • Run the IBM MQ container ("docker-compose up" in x-pack/metricbeat/module/ibmmq) locally.
  • Run metricbeats with the ibmmq module enabled.
  • In Kibana look at the imported metric data -> For metricset qmgr, you should see only metrics beginning with "prometheus.metrics.ibmmq_qmgr"
  • Remove the metrics_filter from the manifest file and restart metricbeats
  • In Kibana look at the imported metric data -> For metricset qmgr, you should see a number of metrics not beginning with "prometheus.metrics.ibmmq_qmgr" besides metrics beginning with that prefix.

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 12, 2020

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 12, 2020

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ma-hei ma-hei mentioned this pull request Mar 12, 2020
Closed
@ChrsMark
Copy link
Copy Markdown
Member

ChrsMark commented Mar 12, 2020

jenkins, test this

@ChrsMark
Copy link
Copy Markdown
Member

ChrsMark commented Mar 12, 2020

@ma-hei thank you for contributing this! This is looking good to me!

@mtojek any comments?

@ChrsMark ChrsMark requested a review from mtojek March 12, 2020 08:41
@ChrsMark ChrsMark added module refactoring Team:Integrations Label for the Integrations team Team:Services (Deprecated) Label for the former Integrations-Services team labels Mar 12, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 12, 2020

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 12, 2020

Pinging @elastic/integrations-services (Team:Services)

@ma-hei
Copy link
Copy Markdown
Contributor Author

ma-hei commented Mar 12, 2020

looking at test failures..

@jsoriano jsoriano mentioned this pull request Mar 12, 2020
Merged
@ma-hei
Copy link
Copy Markdown
Contributor Author

ma-hei commented Mar 14, 2020 •
edited
Loading

@ChrsMark The test build was previously failing because the test expects the "up" event in the returned metrics. The "up" event is now filtered out by the metrics_filter. Before this commit, this test was succeeding because the previously used javascript processor doesn't get applied in the test setting (correct me if I'm wrong). I removed the "up" event from the expected data file because I believe it's filtered out on purpose (judging from logic in "addUpEvent" in prometheus/collector/collector.go). The test should succeed now (and I now know how to run tests locally, before creating a PR :) ) The other option would be to add the "up" metric to the default metrics_filter.

@ChrsMark
Copy link
Copy Markdown
Member

ChrsMark commented Mar 14, 2020 •
edited
Loading

Hey @ma-hei, good job with this! I would see value in reporting up metric since it would give the user the insight of the monitoring endpoint 's health. Since we are collecting from a Prometheus Exporter, it would be meaningful to report the health of it along with the ibmmq specific metrics. By health we actually mean that the endpoint is reachable.

@ma-hei
Copy link
Copy Markdown
Contributor Author

ma-hei commented Mar 14, 2020

@ChrsMark makes sense. I added "up" to the default metrics_filters and re-added the "up" event to the expected test metrics.

@ChrsMark
Copy link
Copy Markdown
Member

ChrsMark commented Mar 16, 2020

jenkins, test this

ChrsMark
ChrsMark reviewed Mar 16, 2020
View reviewed changes
@ma-hei ma-hei force-pushed the 16513 branch 2 times, most recently from fdf447f to a582344 Compare March 16, 2020 16:48
@ChrsMark
Copy link
Copy Markdown
Member

ChrsMark commented Mar 16, 2020

jenkins, test this please

This was referenced Mar 16, 2020
Merged
Merged
@ma-hei
Copy link
Copy Markdown
Contributor Author

ma-hei commented Mar 17, 2020

@ChrsMark could you run Jenkins on this once more? I'm fairly confident that the test will succeed now (I forgot to add "up" as an expected metric to another integration test that I wasn't aware of). I can't seem to get this test to run locally.. I'm running "mage update build integTest" from x-pack/metricbeat. It seems to stall at this point.
>> go test: Integration-aws Testing
It might be that my dev environment is in a bad state. If there's any documentation on how to run these tests, I'd be happy to read it.

@ycombinator
Copy link
Copy Markdown
Contributor

ycombinator commented Mar 17, 2020

jenkins, test this please

ChrsMark
ChrsMark approved these changes Mar 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

lgtm @ma-hei! Thank you for working on this!

@ChrsMark ChrsMark added needs_backport PR is waiting to be backported to other branches. v7.7.0 labels Mar 17, 2020
@ChrsMark ChrsMark merged commit 0208a25 into elastic:master Mar 17, 2020
ChrsMark pushed a commit to ChrsMark/beats that referenced this pull request Mar 17, 2020
@ma-hei @ChrsMark
Make use of prometheus filter settings on IBM-MQ Metricbeat module (e…
aa76646 
…lastic#16971)

(cherry picked from commit 0208a25)
@ChrsMark ChrsMark mentioned this pull request Mar 17, 2020
Merged
3 tasks
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Mar 17, 2020
ChrsMark added a commit that referenced this pull request Mar 17, 2020
@ChrsMark @ma-hei
Make use of prometheus filter settings on IBM-MQ Metricbeat module (#…
1e5ae90 
…16971) (#17048)

(cherry picked from commit 0208a25)

Co-authored-by: ma-hei <marten_heidemeyer@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrsMark ChrsMark ChrsMark approved these changes

@mtojek mtojek Awaiting requested review from mtojek

Assignees

No one assigned

Labels

module refactoring Team:Integrations Label for the Integrations team Team:Services (Deprecated) Label for the former Integrations-Services team v7.7.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ma-hei @elasticmachine @ChrsMark @ycombinator @andresrc