JMX Scraper - YAML config and integration test for HBase #1538
Conversation
There was a problem hiding this comment.
[for reviewer] Some small improvements to report meaningful errors for few failed assertions
robsunday
changed the title
robsunday
changed the title
...rationTest/java/io/opentelemetry/contrib/jmxmetrics/target_systems/HbaseIntegrationTest.java
Show resolved
Hide resolved
...ntegrationTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/MetricAssertions.java
Outdated
Show resolved
Hide resolved
| metric: region.count | ||
| unit: "{region}" | ||
| desc: The number of regions hosted by the region server. | ||
|
|
There was a problem hiding this comment.
[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.
| Append_99th_percentile: | ||
| metric: operation.append.latency.p99 | ||
| desc: Append operation 99th Percentile latency. | ||
| Append_max: | ||
| metric: operation.append.latency.max | ||
| desc: Append operation max latency. | ||
| Append_min: | ||
| metric: operation.append.latency.min | ||
| desc: Append operation minimum latency. | ||
| Append_mean: | ||
| metric: operation.append.latency.mean | ||
| desc: Append operation mean latency. | ||
| Append_median: | ||
| metric: operation.append.latency.median | ||
| desc: Append operation median latency. |
There was a problem hiding this comment.
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.
...rationTest/java/io/opentelemetry/contrib/jmxscraper/target_systems/HBaseIntegrationTest.java
Outdated
Show resolved
Hide resolved
| @@ -7,6 +7,7 @@ rules: | |||
| unit: "{server}" | |||
| type: updowncounter | |||
| mapping: | |||
| # Group of properties to build hbase.master.region_server.count metric | |||
There was a problem hiding this comment.
| # Group of properties to build hbase.master.region_server.count metric | |
| # hbase.master.region_server.count |
[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)
| @@ -16,45 +16,45 @@ | |||
|
|
|||
| def beanMasterServer = otel.mbeans("Hadoop:service=HBase,name=Master,sub=Server") | |||
| otel.instrument(beanMasterServer, "hbase.master.region_server.count", | |||
| "The number of region servers.", "{servers}", | |||
| "The number of region servers.", "{server}", | |||
There was a problem hiding this comment.
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 the jmx-scraper implementation?
There was a problem hiding this comment.
That was our initial plan, however we (or at least I) changed a bit our mind about it for units because:
- units is just metadata, the values captured remain the same and it's quite unlikely any implementation strongly relies on those values (for example, it might break some UI/i18n, but the values remain the same). Changing the metric or its attribute names would not be the same minor impact.
- the semconv conventions about this are stable, thus those changes will eventually have to be done at some point, and this makes one less difference to deal with when enhancing the mappings later.
We already did similar changes with other JMX scraper PRs, for example:
There was a problem hiding this comment.
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).
"Hadoop:service=HBase,name=Master,sub=AssignmentManager" metrics are defined as updowncounter, while in my opinion it should be just gauges. I kept updowncounters to minimize incompatibilities.
We may consider if updownocunters should be converted to gauges in this or the following PR