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

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

Merged
merged 1 commit into from
Mar 17, 2020

Conversation

ma-hei
Copy link
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
Collaborator

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
Collaborator

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?

@ChrsMark
Copy link
Member

jenkins, test this

@ChrsMark
Copy link
Member

@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
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link
Collaborator

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

@ma-hei
Copy link
Contributor Author

ma-hei commented Mar 12, 2020

looking at test failures..

@ma-hei
Copy link
Contributor Author

ma-hei commented Mar 14, 2020

@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
Member

ChrsMark commented Mar 14, 2020

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
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
Member

jenkins, test this

@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
Member

jenkins, test this please

@ma-hei
Copy link
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
Contributor

jenkins, test this please

Copy link
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
@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
…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
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
Development

Successfully merging this pull request may close these issues.

5 participants