fix(otel): scope deterministic ID generation - #627
Conversation
| GLOBAL, | ||
| /** Auto-configured OTLP/HTTP provider; plugin-owned. */ | ||
| AUTO_OTLP | ||
| GLOBAL |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Claude AI reviewNo blocking issues found. The change to scope deterministic trace/span ID generation is correct and well-tested. What I verified
Residual test risk (non-blocking)
Reviewed commit |
| 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.
This comment was marked as outdated.
Sorry, something went wrong.
| private final ThreadLocal<String> extractedTraceId = new ThreadLocal<>(); | ||
| private final ThreadLocal<String> arnDerivedTraceId = new ThreadLocal<>(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
| * = "Workflow"}, {@code instrumentationName = "aws-durable-execution-sdk-java"}. A {@code null} passed to any builder | ||
| * setter falls back to the corresponding default. |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
| // Prefer the active Java-agent span, then fall back to explicitly extracted upstream context. | ||
| var invocationParent = extractCurrentSpanContext(); | ||
| if (invocationParent == null) { | ||
| invocationParent = contextExtractor.extract(); |
There was a problem hiding this comment.
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.
Codex AI reviewThree 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 |
Summary
AUTO_OTLPpipeline and the redundantProviderSource; config/no-arg constructors use the global provider and builder constructors use caller-owned providersAUTO_OTLPTesting
mvn spotless:applymvn -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