Repository navigation
fix: export synthetic OpenTelemetry roots - #767
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| failure); | ||
| UnrecoverableDurableExecutionException selected; | ||
| synchronized (activeThreads) { | ||
| if (closing && !(failure instanceof VirtualMachineError || failure instanceof ThreadDeath)) return; |
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.
| var outputPayload = config.getSerDes().serialize(result); | ||
| var output = | ||
| DurableExecutionOutput.success(handleLargePayload(executionManager, outputPayload)); | ||
| executionManager.closeCheckpointContinuationAdmission(); |
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.
There was a problem hiding this comment.
Great to see fallback-root parity landing; the three-level sampler ladder (DurableSampler, bridge, plain replacement) and the "resolve root sampling context before descendants" fix read well.
Before approve, three parity-blockers against JS #966 as merged and one scope question. I'd also split out the core SDK changes into a separate PR, about half the diff is sdk/ work (ExecutionManager, DurableExecutor, WaitForConditionOperation, StepOperation, plugin contract) that lives outside the otel-plugin fallback-root concern. Keeping Java the same OTel-only shape as JS #966 and Python #754 makes three-SDK parity review here (and in the sister SDKs) far cheaper. Just a suggestion, though, if it's easier keeping it in this pr also good.
I may have mistraced something and I'm happy to be corrected on any of the parity points :-)
|
|
||
| import com.amazonaws.services.lambda.runtime.Context; | ||
| import java.time.Instant; | ||
| import java.util.ArrayList; |
There was a problem hiding this comment.
(Anchoring at the top of ExecutionManager.java as representative of the sdk/** surface this PR touches.)
This PR mixes a sdk/** continuation-coordinator and WaitForCondition rework with the otel-plugin fallback-root fix. Could we please move the core SDK work (ExecutionManager, DurableExecutor, WaitForConditionOperation, StepOperation, the plugin contract Javadoc change) to its own PR?
Why: JS #966 and Python #754 are OTel-only. Keeping Java the same shape makes the three-SDK parity review tractable, and the core work has its own correctness surface — there's a Codex P2 open about StepOperation retries that deserves its own scope.
There was a problem hiding this comment.
I kept the existing core changes in this PR for this revision, following the option in your review summary to keep one PR. The new changes are confined to otel-plugin/; they do not dismiss the separate core correctness concerns. READY continuation handling, output-preparation End delivery and the remaining mandatory-large-result checkpoint concern are called out separately in the description, and that checkpoint concern remains open.
Commit: 466fc1a2.
There was a problem hiding this comment.
Thanks, the anchor fixes in this revision look good to me, and one PR is fine. You're also right about the replacement sampler: terminal-only creation wouldn't make it consistent either.
On the large-result race (Codex r4233196696, r4233420625), I think main has it too now that #771 has merged. Tracing current main:
- The handler's success is selected first (
DurableExecutor.javaL125-L129). - Main closes continuation admission only in
ExecutionManager.close()(L732-L742), which runs after output preparation. So a continuation can still fail during preparation and completeexecutionExceptionFuture(L627-L644). - The >6 MB
EXECUTION/SUCCEEDupdate then reachescheckpointBatch. It seesexecutionExceptionFuturedone, logs an error, and returns without sending the update (CheckpointManager.javaL216-L224). - The batcher still completes the caller's future normally (
ApiRequestDelayedBatcher.javaL184-L187), sohandleLargePayloadreturns""(L464-L475). - So the invocation returns SUCCEEDED with an empty result that was never checkpointed.
This PR narrows the window by closing admission before preparation. A fatal continuation, or a failure selected just before the close, can still publish afterwards.
This can land in a separate PR, since main is exposed as is. Could you please open an issue to track it? Codex's new worker-admission finding (r4262386190) could go in the same issue, since main has that code too. I think a fix would probably look something like this:
a) checkpointBatch completes skipped updates exceptionally instead of normally. A write that was never sent then can't be reported as persisted, and the worst case becomes a RETRYING invocation. I'd treat this as the minimum, because it covers every reason a batch is skipped, not only continuation failures.
b) On top of that, Codex's fix: decide between success and continuation failure under one lock before output preparation, and reject failures published after the close.
Plus Codex's regression test: hold oversized-result preparation while a continuation fails, then assert the invocation doesn't return SUCCEEDED with an empty result.
I'm approving now, but if #767 conflicts after #771, pls feel free to ping me and I'll re-review/approve.
I've only traced this, not run it, so I could easily be mistracing. Happy to be corrected :-)
There was a problem hiding this comment.
Opened #787 to track the skipped mandatory-result checkpoint and the related already-admitted worker boundary. It explicitly records this as static analysis, with runtime confirmation still outstanding, and separates persistence acknowledgment from any stronger success/control arbitration contract.
I am resolving the #771 merge conflicts now. The approved same-worker End lifecycle changes where legacy continuation cleanup can run; I am validating those boundaries against the public tests and will point out any material conflict-resolution differences for re-review, as you offered.
There was a problem hiding this comment.
The conflicts with #771 are resolved in f256f4f0 on main 1064d0ec. The immediate, decision-only root and the approved plain-replacement sampling behavior are retained. The integration uses main's handler-owned Start/End lifecycle and adds its handler scope restoration around the existing root cleanup/flush.
One boundary changed during conflict resolution and needs your re-review: the old test parked a worker inside Executor.execute, after SDK admission, and expected it never to start after End. Main's lifecycle permits that already-admitted worker to start. The revised test now proves this boundary explicitly (one late start for the unawaited/early-parallel variants, none for awaited/already-running variants), while the separate preparation controls still assert zero new registration/dispatch after admission closes. The README records the limitation. This does not fix the remaining worker-start or large-result acknowledgment issues tracked in #787.
Complete Java 17 verification passes: 2,915 passed, 31 conditional skips, zero failures/errors. Focused Java 25 validation passes 581 tests, including both OTel views and the handler-owned lifecycle controls. Please re-review this conflict resolution before we squash-merge; I am not treating the approval of 0df995cc as approval of this changed boundary.
| executionStartTime, | ||
| samplingIntent, | ||
| OtelPluginSupport.usesLocalDurableSampler(sdkTracerProvider), | ||
| OtelPluginSupport.usesDurableSamplingBridge(sdkTracerProvider)); |
There was a problem hiding this comment.
blocker — sampler attributes & tracestate on the anchor diverge from JS
The anchor currently carries the full sampler SamplingResult (attributes and updated tracestate). JS #966 as merged carries only the sampling decision; the anchor's attributes are durable.execution.arn + durable.execution.synthetic_root and nothing from the sampler.
Why this matters: the anchor is the only span re-exported under the same span ID across invocations (#756 explicitly calls this out as "stable across recovery exports"). A sampler whose attributes or tracestate change per call — a time-based ratio sampler, a sampler reading request-scoped baggage — will produce a different span body for the same span ID on each re-export.
Could we build the anchor from the SamplingDecision only, matching JS? README L113 already demands sampler stability for identical re-exports; narrowing the inputs is the enforceable version of that requirement.
There was a problem hiding this comment.
Updated the managed anchor to carry only the sampled decision and the two SDK identity attributes. Sampler-added attributes and updated tracestate remain on descendants, but no longer change a re-exported anchor. ExecutionRootParityTest.anchorReexportsExcludeChangingSamplerMetadataButChildrenKeepIt exercises a sampler returning different metadata on successive invocations in both views; the isolated-loader controls also verify the boundary. The delegate is not queried again for the anchor.
Commit: 466fc1a2.
| A visible plain replacement sampler supplies its root policy for execution-ancestor flags, then keeps its normal | ||
| per-span sampling behavior. It receives no SDK sampling carrier and does not provide the SDK decision-reuse | ||
| or full-result override guarantees above. Opaque providers cannot expose a later sampler replacement to the | ||
| application, so retain the wrapper through the documented customization setup. | ||
|
|
||
| For explicit execution-level control, keep `DurableSampler` as the final installed sampler and set `Sampled` upstream, | ||
| for example through X-Ray | ||
| active tracing. The SDK sampling wrapper preserves that decision. | ||
|
|
||
| Parentless fallback-root exports also use the provider's sampler. If the SDK wrapper is replaced by a plain | ||
| sampler, those anchors can follow its normal root policy independently of the flags used by their descendants; | ||
| full upstream sampling overrides require the installed SDK wrapper. |
There was a problem hiding this comment.
blocker — plain-replacement sampler lets the anchor diverge from its descendants
A visible plain replacement sampler supplies its root policy for execution-ancestor flags, then keeps its normal per-span sampling behavior… those anchors can follow its normal root policy independently of the flags used by their descendants.
JS considered and rejected this behaviour in the lead-up to #966 (ParidelPooya raised it as the "no second sampler query" case): a sampled anchor with an unsampled Invocation child, or vice versa, is a broken hierarchy. JS creates the root only at terminal completion when the SDK wrapper isn't installed, so anchor and descendants always share one decision.
Could we align Java here? Either keep the terminal-only root for the plain-replacement path, or document explicitly that we accept the hierarchy mismatch and name the test case that covers it.
There was a problem hiding this comment.
I kept the documented replacement-sampler boundary and added ExecutionRootParityTest.plainReplacementRetainsIndependentPerSpanSampling: both views explicitly cover an anchor-only policy and an Invocation-only policy. The README now names the mismatch and that test; retaining DurableSampler is required for a shared SDK decision. JS's opaque terminal-only fallback avoids an extra early sampler query, but terminal-only creation cannot force an arbitrary replacement sampler's later root and child decisions to agree. I have not claimed that stronger guarantee.
Commit: 466fc1a2.
| When the backend supplies no complete remote parent, both views export a `DurableExecutionRoot` anchor before the first | ||
| invocation returns, including when it suspends with `PENDING` or fails with `RETRYING`. | ||
| The anchor is started before descendants so their parent context includes its provider-resolved trace flags and | ||
| trace state. It ends at invocation end, before flushing, with the same fixed execution-start timestamp. For a |
There was a problem hiding this comment.
suggestion — the anchor ends at invocation end, not in the start hook
It ends at invocation end, before flushing, with the same fixed execution-start timestamp.
JS and Python end the anchor in the start hook (all three are zero-duration regardless). A Java invocation killed between start and onInvocationEnd — a Lambda timeout, a provider-side kill, an uncaught Error escaping before cleanup — never exports its copy, where JS/Python may already have exported theirs on the batch processor's timer. Ending the anchor immediately after start still satisfies Codex's "start before descendants" requirement because its SpanContext stays valid as a parent after end().
Would it be reasonable to end the anchor right after start in the start hook, like the other SDKs?
There was a problem hiding this comment.
Changed both views to end the zero-duration anchor during Start, before creating descendants. Its ended SpanContext remains their parent, and a batch processor can export the anchor while customer code runs. End still flushes completed telemetry and does not end/export the anchor a second time. The new Start-timing checks, partial-Start/reentrant-End controls and public-handler flushing tests cover this behavior.
Commit: 466fc1a2.
| var sampled = OtelPluginSupport.isSampled(decision); | ||
| var execCtx = ExecutionTraceContext.resolve(extracted, canonicalTraceId, arn(), idGenerator, () -> sampled); | ||
| executionStartTime = info.executionStartTime(); | ||
| executionRootSpan = OtelPluginSupport.startExecutionRoot( |
There was a problem hiding this comment.
suggestion — RECORD_ONLY produces a recording unsampled anchor
A RECORD_ONLY decision currently creates a recording-but-unsampled anchor. JS and Python emit no anchor in that case. For parity (and because an exporter configured to export unsampled spans would export this one), could we skip the anchor on RECORD_ONLY and treat it the same as DROP?
There was a problem hiding this comment.
Changed the SDK sampler path so RECORD_ONLY and DROP do not record an anchor. Descendants still retain the full decision and sampler metadata. The regression uses an exporter explicitly configured to export unsampled spans, so the absence of the anchor is verified independently of an exporter's usual filtering.
Commit: 466fc1a2.
| invocation returns, including when it suspends with `PENDING` or fails with `RETRYING`. | ||
| The anchor is started before descendants so their parent context includes its provider-resolved trace flags and | ||
| trace state. It ends at invocation end, before flushing, with the same fixed execution-start timestamp. For a | ||
| deferred sampler, the first actual fallback span is now `DurableExecutionRoot`; custom policies that depend on the |
There was a problem hiding this comment.
nit — README missing the zero-duration note
Could we add that the anchor is zero-duration and that its descendants end after it? JS merged this (ParidelPooya point 5): some trace viewers and validators flag "child ends after parent" patterns, and callers hit it in practice. One sentence covers it.
There was a problem hiding this comment.
Added the explicit zero-duration note: descendants can end after the anchor, and its ended context remains a valid parent. The README also notes that viewers enforcing time containment may flag this shape.
Commit: 466fc1a2.
| * Recovery re-exports retain identity and timestamps; Workflow carries duration and outcome. Remote parents are | ||
| * never owned. | ||
| */ | ||
| static Span startExecutionRoot( |
There was a problem hiding this comment.
nit — exportExecutionRoot lives in OtelPluginSupport
OtelPluginSupport's class Javadoc still scopes it to "ADOT SPI constructor support". Could the fallback-root export helpers move to a DurableExecutionRoot class (mirroring Python's ExecutionRoot and the TypeScript createSyntheticRoot grouping)? It makes the Java API shape match the sister SDKs, so the three READMEs can describe the same object.
There was a problem hiding this comment.
Moved fallback-root creation and ending into the package-private DurableExecutionRoot helper. OtelPluginSupport now documents its remaining provider setup, sampling resolution and invocation cleanup responsibilities.
Commit: 466fc1a2.
| * full-fidelity context key (same class loader) and falls back to the thread-scoped system property (cross class | ||
| * loader). | ||
| */ | ||
| static Intent get(Context context) { |
There was a problem hiding this comment.
nit — DurableSamplingDecision.get(Context) has no production caller
After the switch to consume(Context), the only remaining call sites of get(Context) are tests. Could it either be @VisibleForTesting or deleted?
There was a problem hiding this comment.
Removed DurableSamplingDecision.get(Context). The tests now exercise consume(Context) directly when checking that the one-shot carrier has been cleared; no testing-only production API or annotation dependency was added.
Commit: 466fc1a2.
| // Invocation and DurableExecutionRoot are exported. A still-open operation has no recording span (creation is | ||
| // deferred to | ||
| // onOperationEnd), so there is nothing to abandon, and the Workflow span is not exported on a non-terminal | ||
| // invocation. |
There was a problem hiding this comment.
nit — awkward wrap in the test comment
The comment wrap splits "(creation is / deferred to / onOperationEnd), so…" across three lines, with // deferred to on its own. Just a readability nit — rewrap at a word boundary so the parenthetical reads cleanly.
There was a problem hiding this comment.
Rewrapped the comment so the deferred-creation parenthetical and the non-terminal invocation explanation each read continuously.
Commit: 466fc1a2.
|
The functional workflows for The reference-only repair is #786. It uses the published squash commit |
| var outputPayload = config.getSerDes().serialize(result); | ||
| var output = | ||
| DurableExecutionOutput.success(handleLargePayload(executionManager, outputPayload)); | ||
| executionManager.closeCheckpointContinuationAdmission(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| CompletableFuture<Void> completion) { | ||
| try { | ||
| try { | ||
| if (!isClosing()) continuation.run(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| pollingFutures.entrySet().removeIf(entry -> { | ||
| entry.getValue().removeIf(CompletableFuture::isDone); | ||
| return entry.getValue().isEmpty(); | ||
| }); |
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.
Retain the approved root sampling behavior and worker-owned scope cleanup. Preserve the base boundary for already-admitted workers, with explicit regression coverage and documentation linking the remaining issue #787.
zhongkechen
changed the title
| /** Closes new checkpoint-continuation admission without signaling owners or joining their cleanup. */ | ||
| void closeCheckpointContinuationAdmission() { | ||
| synchronized (activeThreads) { | ||
| closing = true; |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_53bft2vg4vjxd3j53a3loalx2z
P2: This can select success after a continuation failure has already won under activeThreads but before signalContinuationFailure() publishes it to executionExceptionFuture. The invocation may return SUCCEEDED; an oversized result can also skip its mandatory checkpoint. Atomically arbitrate closure against selectedContinuationFailure under this lock, propagate the selected retry failure before serialization, and test the selection-to-publication interleaving with a large output.
|
|
||
| private DurableExecutionOutput finishInvocation(O value, Throwable failure) { | ||
| if (failure != null) return finishFailure(ExceptionHelper.unwrapCompletableFuture(failure)); | ||
| manager.closeCheckpointContinuationAdmission(); |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_k7k7xlxduyzbkvhhvq5fywjf2o
P2: Closing registration here is insufficient because an existing continuation can already have passed isClosing(), wait while retiring its previous worker, and submit a new step or predicate worker after serialization or invocation End begins. That permits user side effects and checkpoints after success was selected. Make user-worker admission atomic with the manager’s closing state, and test a continuation paused between prior-worker handoff and executor submission.
Codex AI reviewFound two P2 lifecycle races in the new successful-output admission boundary. Reviewed commit |
This branch was successfully deployed
2 participants
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Issue Link, if available
Fixes #756. Related first-invocation behavior: aws/aws-durable-execution-sdk-js#938 and merged aws/aws-durable-execution-sdk-js#966.
Description
Fallback Workflow and Invocation spans referenced an SDK-created parent that was never exported. Both OTel views now create
DurableExecutionRootbefore descendants and end it immediately in the invocation Start hook. Sampled PENDING and RETRYING invocations can export the anchor without waiting for terminal completion. Its existing ARN-derived identity and checkpointed execution-start timestamp remain stable; it is zero-duration, so descendants can end after it. Complete remote parents remain externally owned. Later invocations can re-export the same anchor after an interrupted or failed delivery; export and flush are not an exactly-once guarantee.With
DurableSamplerinstalled, the anchor uses only the sampled decision and SDK identity attributes. It excludes changing sampler attributes/tracestate and is not recorded for RECORD_ONLY or DROP. Descendants keep the full SamplingResult, including RECORD_ONLY, attributes and tracestate. The same-loader carrier, foreign-loader bridge and bounded execution-keyed decision cache retain their existing ownership and reuse behavior. Intent is consumed before processor callbacks. A provider that replaces the SDK sampler retains ordinary per-span sampling, which can select an anchor and its descendants independently; the README and both-view controls make this boundary explicit.The PR now includes main
1064d0ecand its handler-owned invocation lifecycle. Start, handler/finally, SDK output preparation and End run on the handler worker; outer manager cleanup follows End. The remaining core delta closes new checkpoint-continuation admission before successful-output serialization. It does not revoke workers already admitted to the configured executor: they can begin after End, and already-running unawaited work can finish afterward. Their final operation telemetry is not guaranteed to be included. This is a material change from the earlier queued-resumption assertion, now documented and covered explicitly. The worker-start boundary and mandatory large-result checkpoint acknowledgment concern remain tracked in #787; this PR does not fix them or add a global outcome arbiter.Thread-bound OTel scopes are closed by their owning user-function worker, including late UserFunctionEnd and worker/plugin reuse. Output preparation failures deliver RETRYING End before propagating the original failure. End flushes completed telemetry even after partial Start. An isolated root end processor Exception or LinkageError is logged without suppressing descendant setup or the End flush; other Errors retain the existing Start-boundary behavior. The merged core supplies its lifecycle capability check, reverse-order End dispatch, failure classification and MDC behavior. Complete remote parents and the documented visible replacement sampler policy remain unchanged.
Demo/Screenshots
Not applicable (SDK and exported trace behavior).
Checklist
Testing
The integrated head passes complete Java 17 verification: 2,915 tests passed, 31 conditional skips, no failures or errors. Focused Java 25 validation passes 581 tests with no skips, including 531 OTel controls and 50 core/integration lifecycle controls. Formatting and diff checks pass. The first Java 25 attempt was blocked by Mockito attachment inside the sandbox; the same tests pass on the host. Current-head cloud and artifact-compatibility results remain separate CI checks.
Unit Tests
Both views cover immediate anchor completion, stable identity with changing sampler metadata, sampled/RECORD_ONLY/DROP behavior, retained descendant metadata, plain replacement policies, isolated class loaders, processor sampling/ID isolation, partial Start, reentrant End and reuse. Status/type assertions select Invocation by name rather than export order.
Integration Tests
The OTel suite exercises real public DurableExecutor/LocalDurableTestRunner paths for first PENDING invocation and resume, skipped completed work, serialization/checkpoint failures, BatchSpanProcessor flushing and recovery. Existing readiness, continuation ownership, map threshold/replay and late worker Context regressions remain in the PR. The admission controls distinguish unchanged rejection of new registration during output preparation from an already-admitted worker parked inside executor dispatch; the latter can start after End under the merged base lifecycle.
Examples
Existing examples, cloud assertions, thresholds, scenario references and deployment locks are unchanged.