Skip to content

fix(otel): scope deterministic ID generation - #627

Open
zhongkechen wants to merge 3 commits into
mainfrom
fix/otel-scoped-id-generation
Open

fix(otel): scope deterministic ID generation#627
zhongkechen wants to merge 3 commits into
mainfrom
fix/otel-scoped-id-generation

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • scope deterministic trace and span ID overrides to durable-plugin span creation while preserving the provider's configured ID generator for unrelated instrumentation
  • separate the deterministic Workflow trace from ambient Invocation traces and remove fabricated replay/continuation links
  • late-bind the ADOT/global tracer provider at invocation start; disable telemetry for the entire invocation when it is unavailable and retry on the next invocation without installing OpenTelemetry's no-op global
  • keep explicit builder-based providers eagerly bound and caller-owned
  • remove the unreleased plugin-owned AUTO_OTLP pipeline and the redundant ProviderSource; config/no-arg constructors use the global provider and builder constructors use caller-owned providers
  • remove OTLP exporter and semantic-conventions dependencies that were only needed by AUTO_OTLP

Testing

  • mvn spotless:apply
  • mvn -pl otel-plugin -am -Djacoco.skip=true -DargLine=... test (1,145 SDK, 13 testing-utility, and 160 OTel tests passed)
  • mvn -pl examples -am -DskipTests test (example and cloud-test sources compiled)

Cloud tests were not run because they require deployed AWS resources.

Closes #625

@zhongkechen
zhongkechen requested a review from a team August 15, 2026 18:30
@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 15, 2026 18:30 — with GitHub Actions Inactive
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime August 15, 2026 18:30 — with GitHub Actions Active
GLOBAL,
/** Auto-configured OTLP/HTTP provider; plugin-owned. */
AUTO_OTLP
GLOBAL

This comment was marked as outdated.

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown

Claude AI review

No blocking issues found. The change to scope deterministic trace/span ID generation is correct and well-tested.

What I verified

  • DeterministicIdGenerator: the ThreadLocal<IdOverride> + thread-keyed system-property bridge is scoped tightly around each startSpan() via useIds/IdScope (try-with-resources, nesting-safe, restores prior state on close). Overrides no longer persist past a single span start, and consumeScopedSpanId prevents span-ID reuse. generateTraceId/generateSpanId/generatesRandomTraceIds fall through to the wrapped fallback generator outside scope. installOn is idempotent (returns an existing DeterministicIdGenerator unchanged).
  • Both plugins now use only the arn-explicit overloads and no longer rely on the generator's internal ARN/extracted-trace state; the Workflow trace is deterministic/X-Ray-compatible while Invocation spans inherit ambient/X-Ray context or a fresh provider root. Removal of the fabricated replay/continuation links and the InvocationOtelPlugin prior-invocation placeholder is consistent with the new per-invocation trace model; continuations now link to the Workflow span.
  • AUTO_OTLP, OTLP endpoint/headers config, and the opentelemetry-exporter-otlp/semconv dependencies are fully removed with no remaining references in main/example sources; all test usages are removed in the same diff. OtelPluginAutoConfigurationCustomizerProvider.order() = Integer.MAX_VALUE correctly makes it wrap the agent's configured generator last.

Residual test risk (non-blocking)

  • DeterministicIdGenerator.configuredIdGenerator reads SdkTracerProviderBuilder's private ID-generator field by reflection (necessary because the builder exposes only a setter). It works against the pinned OpenTelemetry 1.65.0 and is unit-tested, but is fragile to future OTel SDK internal field changes or stronger module encapsulation, in which case plugin construction would throw IllegalStateException. Worth an integration guard when bumping the OTel version.
  • generateTraceIdForExecution falls back to Instant.now() when executionStartTime is null, which would make the Workflow trace ID non-deterministic across invocations. InvocationInfo.executionStartTime is documented as stable and sourced from the initial EXECUTION operation, so this should not occur in practice; no test exercises the null-start-time path.

Reviewed commit 6ff3ca8876ffc0dc38152b305da27a1c341af36c. Workflow run

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime August 15, 2026 20:43 — with GitHub Actions Active
Comment on lines +124 to +126
var timestamp = executionStartTime != null ? executionStartTime : Instant.now();
var timestampHex = String.format("%08x", timestamp.getEpochSecond() & 0xffffffffL);
return timestampHex + sha256(arn != null ? arn : "").substring(0, 24);

This comment was marked as outdated.

Comment on lines +32 to +33
private final ThreadLocal<String> extractedTraceId = new ThreadLocal<>();
private final ThreadLocal<String> arnDerivedTraceId = new ThreadLocal<>();

This comment was marked as outdated.

@github-actions

This comment has been minimized.

@zhongkechen
zhongkechen deployed to ai-pr-review-runtime August 15, 2026 21:36 — with GitHub Actions Active
Comment on lines +27 to +28
* = "Workflow"}, {@code instrumentationName = "aws-durable-execution-sdk-java"}. A {@code null} passed to any builder
* setter falls back to the corresponding default.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex AI review

[P1] Preserve the existing public provider configuration API. This removes ProviderSource, the config getters/builders (providerSource, otlpEndpoint, otlpHeaders), and both plugins' public providerSource() method. Existing clients will fail to compile or link after a minor-version upgrade, contrary to the repository's API-compatibility requirement. Retain and deprecate these APIs and their behavior until a major release, or introduce this removal only with an explicit breaking-version transition.

}

String generateTraceIdForExecution(String arn, Instant executionStartTime) {
var timestamp = executionStartTime != null ? executionStartTime : Instant.now();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex AI review

[P2] Keep the null-timestamp fallback deterministic. LocalDurableTestRunner constructs its EXECUTION operation without a startTimestamp, so resumed invocations can call this with null. Using Instant.now() changes the workflow trace ID whenever invocations cross an epoch-second boundary, splitting one durable execution across multiple Workflow traces. Derive a stable fallback from the execution ARN (or supply and persist a stable start time) and add a multi-invocation null-timestamp test.

Comment on lines +201 to +204
// Prefer the active Java-agent span, then fall back to explicitly extracted upstream context.
var invocationParent = extractCurrentSpanContext();
if (invocationParent == null) {
invocationParent = contextExtractor.extract();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Codex AI review

[P2] Honor the configured ContextExtractor before ambient context. With an active Java-agent span, this ordering never calls a user-supplied extractor, despite ContextExtractor's contract that it establishes invocation parent context. Custom W3C or other runtime propagation therefore attaches to the wrong trace. Restore extractor-first precedence with ambient context as fallback, and make the equivalent change in ExecutionOtelPlugin; cover the case where both contexts are present.

@github-actions

Copy link
Copy Markdown

Codex AI review

Three regressions need resolution: public API breakage, unstable fallback workflow IDs, and ignored custom context extractors. Review was read-only; tests were not run.

Reviewed commit 6efa92c042b8abdc8476b47c3afe481ab1490598. Workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[otel] Scope deterministic trace IDs without replacing provider-wide generation

1 participant