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

adds channel metrics #3360

Merged
merged 21 commits into from
Mar 25, 2019
Merged

adds channel metrics #3360

merged 21 commits into from
Mar 25, 2019

Conversation

gmmeyer
Copy link
Contributor

@gmmeyer gmmeyer commented Mar 22, 2019

What does this PR do?

This adds channel metrics for IBM MQ

Motivation

customer request

Additional Notes

The number of channels metric was requested. There aren't really many other metrics for channels that are exposed, so the only other thing we could do is status.

We let them set the channels so that we can send a warning if a channel is not there. A channel that exists has only two statuses, on and off. A channel that's not there won't trigger anything when we look for the status automatically.

Review checklist (to be filled by reviewers)

  • 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
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@gmmeyer
Copy link
Contributor Author

gmmeyer commented Mar 22, 2019

I haven't written tests yet, I'll write them next week

@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #3360 into master will increase coverage by 1.95%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #3360      +/-   ##
==========================================
+ Coverage   79.11%   81.07%   +1.95%     
==========================================
  Files         153       10     -143     
  Lines        7747      354    -7393     
  Branches      945       32     -913     
==========================================
- Hits         6129      287    -5842     
+ Misses       1395       60    -1335     
+ Partials      223        7     -216

Copy link
Contributor

@zippolyte zippolyte left a comment

Choose a reason for hiding this comment

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

Not sure I understand completely. You're collecting a metric for the number of channels, then checking for the status of each of them, and then checking for the status of specified channels in the config ? Why is that ?

@gmmeyer
Copy link
Contributor Author

gmmeyer commented Mar 25, 2019

@zippolyte The number of channels metric was requested. There aren't really many other metrics for channels that are exposed, so the only other thing we could do is status.

We let them set the channels so that we can send a warning if a channel is not there. A channel that exists has only two statuses, on and off. A channel that's not there won't trigger anything when we look for the status automatically.

Copy link
Contributor

@zippolyte zippolyte left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. Can you try to factorize a bit the code then ? It feels like a lot of duplication that could be avoided.

@gmmeyer
Copy link
Contributor Author

gmmeyer commented Mar 25, 2019

Yea I can do that

zippolyte
zippolyte previously approved these changes Mar 25, 2019
Copy link
Contributor

@zippolyte zippolyte left a comment

Choose a reason for hiding this comment

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

Awesome ! Thanks for the inline comments as well 👍
Let's make sure we don't forget to add tests later.

@gmmeyer
Copy link
Contributor Author

gmmeyer commented Mar 25, 2019

@zippolyte I did add it to the tests!

@@ -69,6 +71,17 @@ def test_check(aggregator, instance, seed_data):

aggregator.assert_all_metrics_covered()

from six import iteritems
for sc, data in iteritems(aggregator._service_checks):
log.warning("{} {}".format(sc, data))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove those 3 lines

@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #3360 into master will increase coverage by 1.79%.
The diff coverage is 88.09%.

@@            Coverage Diff             @@
##           master    #3360      +/-   ##
==========================================
+ Coverage   79.11%   80.91%   +1.79%     
==========================================
  Files         153       10     -143     
  Lines        7747      351    -7396     
  Branches      945       31     -914     
==========================================
- Hits         6129      284    -5845     
+ Misses       1395       60    -1335     
+ Partials      223        7     -216

@gmmeyer gmmeyer merged commit e272d03 into master Mar 25, 2019
@gmmeyer gmmeyer deleted the greg/ibmmq_channels branch March 25, 2019 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants