[jmx-scraper] Assertions refactoring - Tomcat integration test converted #1589
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,9 +5,9 @@ | |||||
|
|
||||||
| package io.opentelemetry.contrib.jmxscraper.target_systems; | ||||||
|
|
||||||
| import static io.opentelemetry.contrib.jmxscraper.assertions.DataPointAttributes.attribute; | ||||||
| import static io.opentelemetry.contrib.jmxscraper.assertions.DataPointAttributes.attributeGroup; | ||||||
| import static io.opentelemetry.contrib.jmxscraper.assertions.DataPointAttributes.attributeWithAnyValue; | ||||||
|
|
||||||
| import io.opentelemetry.contrib.jmxscraper.JmxScraperContainer; | ||||||
| import java.nio.file.Path; | ||||||
|
|
@@ -46,67 +46,76 @@ protected JmxScraperContainer customizeScraperContainer( | |||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| protected MetricsVerifier createMetricsVerifier() { | ||||||
| return MetricsVerifier.create() | ||||||
| .add( | ||||||
| "tomcat.sessions", | ||||||
| metric -> | ||||||
| metric | ||||||
| .hasDescription("The number of active sessions") | ||||||
| .hasUnit("sessions") // TODO: not aligned with semconv. Should be "{session}" | ||||||
| .isGauge() | ||||||
| .hasDataPointsWithOneAttribute(attributeWithAnyValue("context"))) | ||||||
| .add( | ||||||
| "tomcat.errors", | ||||||
| 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. |
||||||
| .isCounter() | ||||||
| .hasDataPointsWithOneAttribute(attribute("proto_handler", "\"http-nio-8080\""))) | ||||||
| .add( | ||||||
| "tomcat.processing_time", | ||||||
| metric -> | ||||||
| metric | ||||||
| .hasDescription("The total processing time") | ||||||
| .hasUnit("ms") | ||||||
| .isCounter() | ||||||
| .hasDataPointsWithOneAttribute(attribute("proto_handler", "\"http-nio-8080\""))) | ||||||
| .add( | ||||||
| "tomcat.traffic", | ||||||
| metric -> | ||||||
| metric | ||||||
| .hasDescription("The number of bytes transmitted and received") | ||||||
| .hasUnit("by") | ||||||
|
There was a problem hiding this comment.
Suggested change
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 |
||||||
| .isCounter() | ||||||
| .hasDataPointsWithAttributes( | ||||||
| attributeGroup( | ||||||
| attribute("direction", "sent"), | ||||||
| attribute("proto_handler", "\"http-nio-8080\"")), | ||||||
| attributeGroup( | ||||||
| attribute("direction", "received"), | ||||||
| attribute("proto_handler", "\"http-nio-8080\"")))) | ||||||
| .add( | ||||||
| "tomcat.threads", | ||||||
| metric -> | ||||||
| metric | ||||||
| .hasDescription("The number of threads") | ||||||
| .hasUnit("threads") // TODO: not aligned with semconv. Should be "{thread}" | ||||||
| .isGauge() | ||||||
| .hasDataPointsWithAttributes( | ||||||
| attributeGroup( | ||||||
| attribute("state", "idle"), | ||||||
| attribute("proto_handler", "\"http-nio-8080\"")), | ||||||
| attributeGroup( | ||||||
| attribute("state", "busy"), | ||||||
| attribute("proto_handler", "\"http-nio-8080\"")))) | ||||||
| .add( | ||||||
| "tomcat.max_time", | ||||||
| metric -> | ||||||
| metric | ||||||
| .hasDescription("Maximum time to process a request") | ||||||
| .hasUnit("ms") | ||||||
| .isGauge() | ||||||
| .hasDataPointsWithOneAttribute(attribute("proto_handler", "\"http-nio-8080\""))) | ||||||
| .add( | ||||||
| "tomcat.request_count", | ||||||
| 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. |
||||||
| .isCounter() | ||||||
| .hasDataPointsWithOneAttribute( | ||||||
| attribute("proto_handler", "\"http-nio-8080\""))); | ||||||
| } | ||||||
| } | ||||||
Oops, something went wrong.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! I'll make the fix then