Skip to content

Extension of JMX metrics testing framework with data point value matching#16274

Merged
laurit merged 9 commits intoopen-telemetry:mainfrom
robsunday:metric-value-assertions
Mar 9, 2026
Merged

Extension of JMX metrics testing framework with data point value matching#16274
laurit merged 9 commits intoopen-telemetry:mainfrom
robsunday:metric-value-assertions

Conversation

@robsunday
Copy link
Copy Markdown
Contributor

Added optional mechanism for validating data point value in JMX metrics integration tests.

@robsunday robsunday marked this pull request as ready for review March 3, 2026 11:32
@robsunday robsunday requested a review from a team as a code owner March 3, 2026 11:32
Comment on lines +34 to +35
DEFAULT_EXCHANGE_DELAY_MS
+ ThreadLocalRandom.current().nextLong(MAXIMUM_EXCHANGE_DELAY_DELTA));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could also use
ThreadLocalRandom.current().nextLong(min, max)

.hasDescription(
"Indicates the number of routes started successfully since context start-up or the last reset operation.")
.hasUnit("{route}")
.hasDataPointsWithIntValues(value -> value.isEqualTo(numberOfTestRoutes))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is a bit strange that jmx metrics have their own custom assertions. I guess when the jmx tests were added it might have been easier to do it this way but now you could use https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/smoke-tests/src/main/java/io/opentelemetry/smoketest/SmokeTestInstrumentationExtension.java to pull data from the fake backend and assert it the same way as other test do. I think ideally we should try to move towards using https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/assertj/MetricAssert.java here too. Keeping the api used in jmx assertions the same as in sdk assertions could make moving back to sdk assertions easier.
Anyway this was just something to consider for future work, not relevant for this pr.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree, replacing those assertions with standard SDK test assertions or making them at least rely on the SDK assertions is something to consider as a future improvement, this was a shortcut taken when we migrated this code from the contrib repository.

@SylvainJuge
Copy link
Copy Markdown
Contributor

@laurit can we merge this so I can add value tests in #16355 ? As #16355 is a bugfix it would be great to be able to include it for next release.

@laurit laurit merged commit 8380d7b into open-telemetry:main Mar 9, 2026
93 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants