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 10 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.
...ud-compute/src/test/java/com/google/cloud/compute/v1/integration/ITComputeGoldenSignals.java
Show resolved
Hide resolved
...ud-compute/src/test/java/com/google/cloud/compute/v1/integration/ITComputeGoldenSignals.java
Outdated
Show resolved
Hide resolved
…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?
| rootSpanName = "ComputeRootSpan-" + generateRandomHexString(8); | ||
|
|
||
| GoogleCredentials credentials = GoogleCredentials.getApplicationDefault(); | ||
| credentials.refreshIfExpired(); |
There was a problem hiding this comment.
Is refreshing the credentials needed?
| .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
| .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
| 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.
| } | ||
| } | ||
|
|
||
| @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?
2 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.