test(o11y): add tests for tracing, metrics and logging in java-compute#12730
test(o11y): add tests for tracing, metrics and logging in java-compute#12730diegomarquezp wants to merge 20 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces integration tests for observability "golden signals" (traces, metrics, and logs) in the Compute service. It adds necessary OpenTelemetry and logging dependencies to the POM, implements the ITComputeGoldenSignals test class, and updates CompositeTracer to delegate requestUrlResolved calls. Review feedback identifies an opportunity to prevent test flakiness by clearing the TestAppender state between tests and suggests removing a redundant call to getMDCPropertyMap in the appender's logic.
…mpute-integration-test
| Arrays.asList( | ||
| new OpenTelemetryTracingFactory(openTelemetrySdk), | ||
| new OpenTelemetryMetricsFactory(openTelemetrySdk), | ||
| new LoggingTracerFactory()); |
There was a problem hiding this comment.
Can we try to enable logging using environment variables?
There was a problem hiding this comment.
Omitted the logging factory here and added the env var in the pom.
| rootSpanName = "ComputeRootSpan-" + generateRandomHexString(8); | ||
|
|
||
| GoogleCredentials credentials = GoogleCredentials.getApplicationDefault(); | ||
| credentials.refreshIfExpired(); |
There was a problem hiding this comment.
Is refreshing the credentials needed?
There was a problem hiding this comment.
I thought a credentials object could be passed directly to the otel exporter, but it only accepts tokens.
In order to get the credentials we need to trigger a fetch by refreshing them.
Added a comment to clarify it.
| .isEqualTo("compute/v1/projects/{project=*}/zones/{zone=*}/instances"); | ||
| String expectedDestinationResource; | ||
| if (expectError) { | ||
| expectedDestinationResource = "//compute.googleapis.com/projects/invalid-project-"; |
There was a problem hiding this comment.
It actually took me some time to figure out why expectedDestinationResource is different even if it errors out. Which is because we are testing that the actual resource startsWith the expected resource. Can we try to use a fixed invalid project id instead of randomUUID? So that we can make sure that the resource name is in the same format for both success and error scenarios.
| assertThat(span.getLabelsMap().get(ObservabilityAttributes.DESTINATION_RESOURCE_ID_ATTRIBUTE)) | ||
| .startsWith(expectedDestinationResource); | ||
|
|
||
| // These might fail if not supported in HTTP/REST yet |
There was a problem hiding this comment.
This comment is not needed since compute only supports HTTP/REST
There was a problem hiding this comment.
Good point. Done.
| .startsWith(expectedDestinationResource); | ||
|
|
||
| // These might fail if not supported in HTTP/REST yet | ||
| assertThat(span.getLabelsMap()).containsKey(ObservabilityAttributes.HTTP_URL_FULL_ATTRIBUTE); |
There was a problem hiding this comment.
Can we verify the value as well? Same comments for the error attributes below as well. Status message might be hard but I think we should be able to verify all other attributes
There was a problem hiding this comment.
Done. Added a contains() assertion for status.message.
| Resource.create( | ||
| Attributes.of(AttributeKey.stringKey("gcp.project_id"), DEFAULT_PROJECT))); | ||
|
|
||
| metricReader = InMemoryMetricReader.create(); |
There was a problem hiding this comment.
Can we use Otel exporter to export it and use Cloud Monitoring client to verify it? To be consistent with how we verify traces.
There was a problem hiding this comment.
Added the exporter and verification against cloud monitoring.
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Let's break this up to a separate PR. This needs to be released but the integration tests does not.
|
|
||
| io.opentelemetry.api.common.Attributes attributes = | ||
| durationMetric.getHistogramData().getPoints().iterator().next().getAttributes(); | ||
| assertThat( |
There was a problem hiding this comment.
Can we verify as much attributes as possible?
There was a problem hiding this comment.
Added a few more for metrics. Most of them in the Cloud Monitoring verification and added a bit more attrs in the in-memory check.
Addressed multiple PR comments for naming, constants, and method extraction.
…/github.com/googleapis/google-cloud-java into observability/test/compute-integration-test
…mpute-integration-test
…/github.com/googleapis/google-cloud-java into observability/test/compute-integration-test
3 participants
This PR adds tests for java-compute to confirm behavior of recently added o11y features.
Key changes:
sdk-platform-java/gax-java: OverroderequestUrlResolvedinCompositeTracerto ensureurl.fullis recorded in HTTP/REST transport. This was a fix found during testing.java-compute: IntroducedITComputeGoldenSignals.javato validate tracing, metrics, and logging.java-compute: AddedGOOGLE_SDK_JAVA_LOGGING=trueenvironment variable tomaven-surefire-plugininpom.xmlto enable logging for verification in tests.