Skip to content

Conversation

@SylvainJuge
Copy link
Contributor

@SylvainJuge SylvainJuge commented Mar 27, 2025

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.

  • the protobuf parsing logic can be reused from io.opentelemetry.javaagent.testing.common.AgentTestingExporterAccess but needs to be moved to a separate class to prevent classloading issues.
  • the metric type checks like isCounter and isGauge needs to be adapted to fit both long and double variants, for example with isLongCounter and isDoubleCounter

Overall I think it would be more beneficial if we extend the SDK MetricAssert with 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 an updowncounter requires to know that it is stored as a "non monotonic sum" and that a counter is 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 to MetricData.

import java.util.function.Consumer;
import java.util.stream.Collectors;

public class MetricsVerifier {
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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.

Comment on lines +46 to +50
strictCheck("description", /* expectedCheckStatus= */ true, descriptionChecked);
strictCheck("unit", /* expectedCheckStatus= */ true, unitChecked);
strictCheck("type", /* expectedCheckStatus= */ true, typeChecked);
strictCheck(
"data point attributes", /* expectedCheckStatus= */ true, dataPointAttributesChecked);
Copy link
Contributor Author

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.

@SylvainJuge SylvainJuge mentioned this pull request Mar 27, 2025
7 tasks
@SylvainJuge SylvainJuge requested a review from robsunday March 27, 2025 10:04
@SylvainJuge SylvainJuge force-pushed the yaml-jmx-metrics-test branch from 6977cc0 to d081c29 Compare March 27, 2025 10:06
@SylvainJuge SylvainJuge self-assigned this Apr 7, 2025
@SylvainJuge
Copy link
Contributor Author

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.

@SylvainJuge SylvainJuge marked this pull request as ready for review April 7, 2025 12:26
@SylvainJuge SylvainJuge requested a review from a team as a code owner April 7, 2025 12:26
@trask trask merged commit fd8fe90 into open-telemetry:main Apr 9, 2025
86 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.

3 participants