-
Notifications
You must be signed in to change notification settings - Fork 1k
yaml jmx metrics test infrastructure #13597
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
Conversation
| import java.util.function.Consumer; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class MetricsVerifier { |
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.
| import org.testcontainers.utility.MountableFile; | ||
|
|
||
| /** Base class for testing YAML metric definitions with a real target system */ | ||
| public class TargetSystemTest { |
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] using a very close test setup as in contrib: the target system is started with the instrumentation agent and a copy of yaml definitions to test them, a very simple metrics exporter endpoint is used on the test host to collect the metrics from the remote system. We could maybe reuse this in other tests but it would probably be better in a separate PR for simplicity.
| /** Dedicated Assertj extension to provide convenient fluent API for metrics testing */ | ||
| // TODO: we should contribute this back to sdk-testing | ||
| // This has been intentionally not named `*Assertions` to prevent checkstyle rule to be triggered | ||
| public class JmxAssertj extends org.assertj.core.api.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.
[for reviewer] this has been copied as-is from contrib with a small rename https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/jmx-scraper/src/integrationTest/java/io/opentelemetry/contrib/jmxscraper/assertions/Assertions.java, it would make sense to add those assertions to io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions but the latter is in the SDK, works with MetricData and not directly with the io.opentelemetry.proto.metrics.v1.Metric class.
| strictCheck("description", /* expectedCheckStatus= */ true, descriptionChecked); | ||
| strictCheck("unit", /* expectedCheckStatus= */ true, unitChecked); | ||
| strictCheck("type", /* expectedCheckStatus= */ true, typeChecked); | ||
| strictCheck( | ||
| "data point attributes", /* expectedCheckStatus= */ true, dataPointAttributesChecked); |
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] this ensures that the test explicitly tests all the common properties of JMX metrics, which could be a feature that would be removed if we used io.opentelemetry.sdk.testing.assertj.MetricAssert, however this also means some duplicated effort so it could make sense to reuse what is available.
6977cc0 to
d081c29
Compare
…nstrumentation into yaml-jmx-metrics-test
…nstrumentation into yaml-jmx-metrics-test
|
We now have two distinct PRs that depend on this test code, so we should be ready for review:
Once this is merged, it would help unblock those PRs and make each PR review easier, for example in the JVM metrics case ~75% of the PR is included in this PR. |
…nstrumentation into yaml-jmx-metrics-test
This PR adds test infrastructure coded needed to make testing YAML metrics easier.
This has been extracted from #13392 to allow reusing it and making review easier.
Related PR(s) that depend on it and provide use-cases:
The test assertions partially overlap the ones provided by SDK testing library but I think it's an acceptable compromise for now as it makes importing/reusing yaml files from contrib easier for the migration process.
For more technical details here is a copy of my conclusion on the topic (initially posted here as comment)
I tried to refactor the test assertions to only rely on the ones provided by the SDK (using
io.opentelemetry.sdk.testing.assertj.MetricAssert), but this requires quite significant work which is probably not worth the investment time for now.io.opentelemetry.javaagent.testing.common.AgentTestingExporterAccessbut needs to be moved to a separate class to prevent classloading issues.isCounterandisGaugeneeds to be adapted to fit bothlonganddoublevariants, for example withisLongCounterandisDoubleCounterOverall I think it would be more beneficial if we extend the SDK
MetricAssertwith similar "high level" assertions that are closer to the metric data model in order to help making test code more readable. For example, currently writing test code for anupdowncounterrequires to know that it is stored as a "non monotonic sum" and that acounteris translated to a "monotonic sum", both of which require to read the actual values as the monoticity is not preserved when parsing it from protobuf toMetricData.