Skip to content

fix(datastore): reduce flakiness of E2E tracing tests #13645

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.

Sign up for GitHub

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

Merged
lqiu96 merged 4 commits into from
Jul 6, 2026
Merged

fix(datastore): reduce flakiness of E2E tracing tests #13645

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e5a18c
fix(datastore): reduce flakiness of E2E tracing tests
lqiu96 Jul 2, 2026
9493964
fix(datastore): use openTelemetrySdk.close() in teardown
lqiu96 Jul 6, 2026
91b9e21
fix(datastore): make tracer and SDK fields non-static in E2E tests
lqiu96 Jul 6, 2026
6d01781
fix(datastore): address PR comments on E2E tracing tests
lqiu96 Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions .../google-cloud-datastore/src/test/java/com/google/cloud/datastore/it/ITE2ETracingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,14 +227,12 @@ private boolean dfsContainsCallStack(long spanId, List<String> expectedCallStack

private static final int NUM_SPAN_ID_BYTES = 16;

private static final int GET_TRACE_RETRY_COUNT = 60;
private static final int GET_TRACE_RETRY_COUNT = 120;

@blakeli0 blakeli0 Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we do Thread.sleep after each unsuccessful check, consider migrate the test to use Awaitility?

@lqiu96 lqiu96 Jul 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised a PR #13668


private static final int GET_TRACE_RETRY_BACKOFF_MILLIS = 1000;

private static final int TRACE_FORCE_FLUSH_MILLIS = 5000;

private static final int TRACE_PROVIDER_SHUTDOWN_MILLIS = 1000;

private static Key KEY1;

private static Key KEY2;
Expand Down Expand Up @@ -393,6 +391,9 @@ public void after() throws Exception {
tracer = null;
retrievedTrace = null;
customSpanContext = null;
if (openTelemetrySdk != null) {
openTelemetrySdk.close();
}
openTelemetrySdk = null;

@gemini-code-assist gemini-code-assist Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The traceExporter resource is created in before() but is not closed in after(), which can lead to resource leaks (such as gRPC channels and threads). Additionally, following the LIFO order for closing resources and ensuring exception safety is recommended.

    try {
      if (openTelemetrySdk != null) {
        openTelemetrySdk.close();
      }
    } finally {
      if (traceExporter != null) {
        traceExporter.close();
      }
    }
    openTelemetrySdk = null;
    traceExporter = null;
References
  1. When managing a collection of closeable resources (e.g., scopes), ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.

}

Expand Down Expand Up @@ -441,6 +442,9 @@ protected void waitForTracesToComplete() throws Exception {
CompletableResultCode completableResultCode =
openTelemetrySdk.getSdkTracerProvider().forceFlush();
completableResultCode.join(TRACE_FORCE_FLUSH_MILLIS, TimeUnit.MILLISECONDS);
if (!completableResultCode.isSuccess()) {
logger.warning("Force flush did not complete successfully");
}
Comment on lines 460 to +465

@gemini-code-assist gemini-code-assist Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If isUsingGlobalOpenTelemetrySDK() is false, openTelemetrySdk will be null, leading to a NullPointerException when calling openTelemetrySdk.getSdkTracerProvider(). Adding a null check and falling back to flushing traceExporter directly ensures robust behavior.

    if (openTelemetrySdk != null) {
      CompletableResultCode completableResultCode =
          openTelemetrySdk.getSdkTracerProvider().forceFlush();
      completableResultCode.join(TRACE_FORCE_FLUSH_MILLIS, TimeUnit.MILLISECONDS);
      if (!completableResultCode.isSuccess()) {
        logger.warning("Force flush did not complete successfully");
      }
    } else if (traceExporter != null) {
      CompletableResultCode completableResultCode = traceExporter.flush();
      completableResultCode.join(TRACE_FORCE_FLUSH_MILLIS, TimeUnit.MILLISECONDS);
      if (!completableResultCode.isSuccess()) {
        logger.warning("Trace exporter flush did not complete successfully");
      }
    }
References
  1. When using lazily initialized resources (such as ExecutorService), ensure that teardown or close methods perform explicit null checks before invoking methods on them to prevent NullPointerException.

}

// Validates `retrievedTrace`. Cloud Trace indexes traces w/ eventual consistency, even when
Expand Down
Loading