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 21 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.
| .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.
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
| Duration currentDelay = retrySettings.getInitialRetryDelayDuration(); | ||
| com.google.monitoring.v3.TimeSeries targetTs = null; | ||
|
|
||
| while (metricsPollingStopwatch.elapsed().compareTo(retrySettings.getTotalTimeoutDuration()) |
There was a problem hiding this comment.
It seems a little too aggressive to put this in a while loop. Usually how long does it take for the metrics to show up?
| validateMetricsInCloudMonitoring(expectError); | ||
| } | ||
|
|
||
| private void validateLogging(boolean expectError) { |
There was a problem hiding this comment.
Ideally we want to test with Cloud Logging, but I know it is hard to test without a proper exporter, so this is good enough for now.
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.