Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
JMX Scraper - YAML config and integration test for HBase #1538
JMX Scraper - YAML config and integration test for HBase #1538
Changes from 9 commits
2dd5fc5
9e3a340
bf36533
7ade121
ac3ff6d
c3a0e23
07c6713
002d6ab
db42889
8d46c94
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably misremembering, but I thought the plan was to not change things in the existing
jmx-metrics
component, and fix them only in thejmx-scraper
implementation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was our initial plan, however we (or at least I) changed a bit our mind about it for units because:
We already did similar changes with other JMX scraper PRs, for example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may only add that this change should be perfectly safe since the only changes were made in unit annotations that are discarded by the parsers anyway (according to these docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[for reviewer] Some small improvements to report meaningful errors for few failed assertions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpicking] Maybe simplify this even further as everything here is a metric (and do the same with others that match
Group of properties to build .* metric
regex)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] the spacing between blocks is intentional to keep things a bit readable here, maybe adding an explicit "header comment" with for example the metric name/prefix/suffix would help to understand this is intentional and needs to be preserved when maintaining this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to this PR, but I am pretty sure those are metrics created with dropwizard, we need to make sure those are mapped consistently for every supported system where they are used. The downside of those here is that there is no way for us to translate that easily to an otel histogram as we don't have access to the individual samples.