[jmx-scraper] Assertions refactoring - Tomcat integration test converted #1589
Conversation
Method names improvement.
refactor with custom assertj assertions
Minor refactorings, mostly renaming. Fixed typos.
…emetry-java-contrib into assertions-refactoring
Assertions refactoring
| metric -> | ||
| metric | ||
| .hasDescription("The number of active sessions") | ||
| .hasUnit("sessions") // TODO: not aligned with semconv. Should be "{session}" |
There was a problem hiding this comment.
Like in the previous PRs where such units inconsistencies were found I think it's worth fixing them straight away as they aren't "breaking changes" most of the time and we will have to fix it at some point because it's now part of the stable semantic conventtions.
There was a problem hiding this comment.
So my understanding is that in this case it may be a kind of breaking change. This is not the annotation in curly braces that is supposed to be skipped by parser.
I've been hesitating if I should update it or not and finally decided to leave it, but if reviewers agree that it should be changed then I'll gladly update it.
There was a problem hiding this comment.
I think it's ok to update it since we are updating units elsewhere (in OTel we've said changing units is breaking whether they're curly braces or not)
There was a problem hiding this comment.
Great! I'll make the fix then
| metric -> | ||
| metric | ||
| .hasDescription("The number of errors encountered") | ||
| .hasUnit("errors") // TODO: not aligned with semconv. Should be "{error}" |
There was a problem hiding this comment.
same as previous comment.
| metric -> | ||
| metric | ||
| .hasDescription("The number of bytes transmitted and received") | ||
| .hasUnit("by") |
There was a problem hiding this comment.
| .hasUnit("by") | |
| .hasUnit("By") |
See https://opentelemetry.io/docs/specs/semconv/general/metrics/#instrument-units https://unitsofmeasure.org/ucum#para-48 and
There was a problem hiding this comment.
Yes, however the same doubts apply here
| metric -> | ||
| metric | ||
| .hasDescription("The total requests") | ||
| .hasUnit("requests") // TODO: not aligned with semconv. Should be "{request}" |
There was a problem hiding this comment.
same as previous, we'd better fix those units inconsistencies when we find them.
There was a problem hiding this comment.
As discussed today, for now we can leave the units as is as those are not in curly braces and we are not just changing from plural to singular form, same for bytes we can leave this inconsistency with a TODO and deal with it later. "Later" here means likely when we revisit those definitions and align with instrumentation.
Description:
Part of #1573