-
Notifications
You must be signed in to change notification settings - Fork 129
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
feat: Integration test for End to End tracing #3691
Conversation
Looking through the details of the failure in Kokoro - Test: Integration with Directpath using the following target log Many of the tests seem to be facing "com.google.api.gax.rpc.PermissionDeniedException: io.grpc.StatusRuntimeException: PERMISSION_DENIED: The caller does not have permission" when on trace exporter. So the underlying issue regarding permission denied for trace export is not related to this change but is causing failure in the new test introduced. |
traceConfigurationBuilder.setProjectId(options.getProjectId()).build()); | ||
|
||
String serviceName = | ||
"java-spanner-jdbc-integration-tests-" + ThreadLocalRandom.current().nextInt(); |
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.
"java-spanner-jdbc-integration-tests-" + ThreadLocalRandom.current().nextInt(); | |
"java-spanner-integration-tests-" + ThreadLocalRandom.current().nextInt(); |
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.
Done
|
||
static { | ||
SpannerOptionsHelper.resetActiveTracingFramework(); | ||
SpannerOptions.enableOpenTelemetryMetrics(); |
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.
why do we need to enable metrics?
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.
removed.
</dependency> | ||
<dependency> | ||
<groupId>com.google.api.grpc</groupId> | ||
<artifactId>proto-google-cloud-trace-v1</artifactId> |
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.
why are we adding this?
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.
We query trace using trace client which requires this dependency.
" trace = client.getTrace(env.getTestHelper().getInstanceId().getProject(), traceId)"
|
||
@Before | ||
public void initSelectValueQuery() { | ||
selectValueQuery = "SELECT @p1 + @p1 "; |
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.
can you initialize selectValueQuery
as a static variable?
private static final String selectValueQuery = "SELECT @p1 + @p1";
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.
done.
.build(); | ||
try (TraceServiceClient client = TraceServiceClient.create(settings)) { | ||
// It can take a few seconds before the trace is visible. | ||
Thread.sleep(5000L); |
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.
Is there any better way to know that trace is exported? I just wanted to make sure this is not flakky
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.
Do a forceflush(), instead of adding a sleep.
sdkTracerProvider.forceFlush();
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.
we need to wait for sever side traces which are collected from request on Span FE and asynchronously exported later.
clientTrace.getSpansList().stream() | ||
.anyMatch( | ||
span -> | ||
"CloudSpannerOperation.ExecuteStreamingQuery".equals(span.getName()))); |
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.
We are only asserting on Client Trace and not on server trace?
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.
updated.
.build(); | ||
try (TraceServiceClient client = TraceServiceClient.create(settings)) { | ||
// It can take a few seconds before the trace is visible. | ||
Thread.sleep(5000L); |
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.
Do a forceflush(), instead of adding a sleep.
sdkTracerProvider.forceFlush();
OpenTelemetrySdk.builder() | ||
.setTracerProvider(sdkTracerProvider) | ||
.setPropagators(ContextPropagators.create(W3CTraceContextPropagator.getInstance())) | ||
.buildAndRegisterGlobal(); |
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.
Do not register the tracerProvider at global, instead inject OpenTelemerty object via spanner options.
If we register it globally, we might start seeing traces coming from other parallel integration tests as enableOpenTelemetryTraces
is a static method.
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.
updated.
google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java
Show resolved
Hide resolved
boolean foundTrace = false; | ||
for (int attempts = 0; attempts < 2; attempts++) { | ||
try { | ||
Trace clientTrace = |
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.
Create the traceClient in "before" block
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.
TraceClient is created before the block. This is actually the trace, not the client. Updated naming in recent commit to avoid confusion.
@manu2 I don't understand the need for this Integration test. Do we want to verify server traces as I don't see that in the assert? |
The test intents to verify the generation of server side traces when end to end tracing is enabled. Have updated the assertion to validate server side trace. |
try { | ||
Trace trace = client.getTrace(env.getTestHelper().getInstanceId().getProject(), traceId); | ||
// Assert Spanner Frontend Trace is present | ||
assertTrue( |
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.
assertTrue will result into error if the trace is not found and the test will be marked as failed, so the for loop will never actually work. Instead of this , we could try something like below and also remove the 10 second sleep before the loop.
Trace trace = client.getTrace(env.getTestHelper().getInstanceId().getProject(), traceId);
while (trace.getSpansList().stream()
.anyMatch(span -> "Spanner.ExecuteStreamingSql".equals(span.getName())
&& metricsPollingStopwatch.elapsed(TimeUnit.SECONDS) < 30) {
// Try every 5 seconds
Thread.sleep(5000);
}
assertTrue(trace.getSpansList().stream()
.anyMatch(span -> "Spanner.ExecuteStreamingSql".equals(span.getName());
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.
Makes sense. If the getTrace calls fails, it throws an APIException. Have kept the catch block to handle it but changed the logic to poll upto 30 sec and sleep for 5 sec if not found.
(cherry picked from commit f47f01a834c80fadf754ed1ceee5179c1bd00deb)
<dependency> | ||
<groupId>com.google.cloud.opentelemetry</groupId> | ||
<artifactId>exporter-trace</artifactId> | ||
<version>0.33.0</version> |
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.
Please remove the version from here. It should either fetch from bom or the latest version
| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [com.google.cloud:google-cloud-spanner](https://github.com/googleapis/java-spanner) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `6.89.0` -> `6.90.0` | --- ### Release Notes <details> <summary>googleapis/java-spanner (com.google.cloud:google-cloud-spanner)</summary> ### [`v6.90.0`](https://github.com/googleapis/java-spanner/blob/HEAD/CHANGELOG.md#6900-2025-03-31) ##### Features - Add default_isolation_level connection property ([#​3702](googleapis/java-spanner#3702)) ([9472d23](googleapis/java-spanner@9472d23)) - Adds support for Interval datatype in Java client ([#​3416](googleapis/java-spanner#3416)) ([8be8f5e](googleapis/java-spanner@8be8f5e)) - Integration test for End to End tracing ([#​3691](googleapis/java-spanner#3691)) ([bf1a07a](googleapis/java-spanner@bf1a07a)) - Specify isolation level per transaction ([#​3704](googleapis/java-spanner#3704)) ([868f30f](googleapis/java-spanner@868f30f)) - Support PostgreSQL isolation level statements ([#​3706](googleapis/java-spanner#3706)) ([dda2e1d](googleapis/java-spanner@dda2e1d)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Never, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: a63885015974acb6061bae5ca3b9d61136991376
With End to End tracing, customers are able to view server side traces. The new integration tests aims to verify this from a client side pov.
Note: Currently the InegrationTestEnv sets up default interceptor which in turns creates a default GlobalOpenTelementry sdk. This sdk is then injected to the gRPC connection builder and stud. So a new opentelemetry sdk has been created in IntegrationTestEnv that sets up the required config for End to End Testing and will be used by default for all integration tests.