Skip to content

fix(otel): set explicit span status OK on successful operation and attempt spans - #583

Open
SilanHe wants to merge 4 commits into
mainfrom
feat/otel-setstatus-ok-on-success
Open

fix(otel): set explicit span status OK on successful operation and attempt spans#583
SilanHe wants to merge 4 commits into
mainfrom
feat/otel-setstatus-ok-on-success

Conversation

@SilanHe

@SilanHe SilanHe commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

set explicit span status OK on successful operation and attempt spans

Testing

  • otel-plugin module build — clean
  • otel-plugin tests — 168/168 (0 failures/errors), incl. 4 new OK-on-success tests (InvocationOtelPluginTest 37→39, ExecutionOtelPluginTest 23→25)

Coordination

Aligns Java with the operation-status/span-status model tracked by aws/aws-durable-execution-conformance-tests#52 and #53, matching Python #604 and the JS companion change.

Port the explicit success-status behavior from the Python OTel plugin
(PR #604) to the Java plugins. Operation spans (onOperationEnd terminal
path and the cross-invocation continuation-span path) and attempt spans
(onUserFunctionEnd) now call span.setStatus(StatusCode.OK) on success,
where previously they were left UNSET. Existing ERROR + recordException
on failure is unchanged.

Applied to both InvocationOtelPlugin and ExecutionOtelPlugin. The
Invocation-span (applyInvocationStatus) and Workflow-span OK/ERROR
mappings are untouched, and operation spans force-ended attribute-less
at onInvocationEnd (still-running/suspended) remain UNSET.

Adds OK-on-success tests for operation and attempt spans in both
plugin test classes.
@SilanHe
SilanHe requested a review from a team July 31, 2026 22:28
@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 22:28 — with GitHub Actions Inactive
@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 22:28 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 22:36 — with GitHub Actions Inactive
@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 22:36 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@SilanHe
SilanHe marked this pull request as draft July 31, 2026 22:48
@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 23:04 — with GitHub Actions Inactive
@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 23:04 — with GitHub Actions Inactive
@SilanHe

SilanHe commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Codex P1 / Claude finding in ad07ddb.

The else -> setStatus(StatusCode.OK) on the operation and continuation spans was gating on info.error() == null, which mislabeled non-success terminal states as OTel OK: onOperationEnd fires for every terminal status, and extractErrorFromOperation returns null for CANCELLED (always) and for FAILED/TIMED_OUT/STOPPED with no error object. All four operation/continuation sites in InvocationOtelPlugin.java and ExecutionOtelPlugin.java now gate on "SUCCEEDED".equals(info.status()) || info.status() == null. The || info.status() == null clause preserves OK for successful statusless virtual (FLAT CONTEXT) operations — verified against ChildContextOperation (success fires fireOnOperationEnd(null, null, false); failure passes a non-null error and hits the ERROR branch). Non-success statuses stay UNSET. Attempt-span else if (info.succeeded()) left untouched.

Added 6 regression tests (3 per plugin): non-SUCCEEDED terminal status with null error -> NOT OK (same-invocation + continuation paths), plus a null-status virtual-context -> OK case. otel-plugin module: 142/142 passing.

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown

Claude AI review

No actionable findings. This change is correct, symmetric across InvocationOtelPlugin and ExecutionOtelPlugin, and well-tested.

I verified the two guards against the SDK's actual behavior:

  • Terminal operation spans (onOperationEnd, both the this-invocation and continuation-span branches): else if ("SUCCEEDED".equals(info.status()) || info.status() == null) correctly stamps OK only on genuine success. The failure branch (info.error() != null) still wins first, and BaseDurableOperation.extractErrorFromOperation returns a non-null error only for FAILED/TIMED_OUT/STOPPED — so error-less non-success statuses (CANCELLED always, and error-less FAILED/TIMED_OUT/STOPPED) fall through and stay UNSET, matching the PR intent. I confirmed the "SUCCEEDED" literal matches operation.status().toString(), the same value the pre-existing DURABLE_OPERATION_STATUS attribute already writes.
  • Null-status path: I enumerated every fireOnOperationEnd caller. The only calls that pass a null Operation (→ null status) are MapOperation:124 (empty map, error=null, success), ChildContextOperation:165 (virtual success, error=null), and ChildContextOperation:216 (virtual FLAT failure, error=exception). The failure case carries a non-null error and is caught by the ERROR branch, so status==null && error==null is always genuine success — the OK stamp is safe.
  • Attempt spans (onUserFunctionEnd): else if (info.succeeded()) → OK is mutually exclusive with the existing ERROR branch and preserves the prior UNSET behavior for the (rare) !succeeded && error==null case.

Residual test risk (minor, non-blocking): The new tests assert OK on the this-invocation operation branch (operationSuccess_setsOkOnOperationSpan), the attempt span, and the null-status path, and assert UNSET on the continuation branch for a non-success status. There is no test that explicitly asserts StatusCode.OK on the continuation-span branch for a SUCCEEDED status (the new success line at ExecutionOtelPlugin.java:359 / InvocationOtelPlugin.java:482). That branch is exercised by pre-existing continuation tests but none assert the span status is OK, so a future regression there would go uncaught. Consider adding a continuation-branch OK-on-success assertion to mirror the existing UNSET negative test.

Reviewed commit ad07ddb883ecf8f2eeabea09543debb722ec6cc1. Workflow run

@SilanHe
SilanHe marked this pull request as ready for review July 31, 2026 23:12
@SilanHe
SilanHe had a problem deploying to ai-pr-review-runtime July 31, 2026 23:12 — with GitHub Actions Failure
@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 23:12 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@SilanHe
SilanHe had a problem deploying to ai-pr-review-runtime July 31, 2026 23:29 — with GitHub Actions Failure
@SilanHe
SilanHe temporarily deployed to ai-pr-review-runtime July 31, 2026 23:29 — with GitHub Actions Inactive
@github-actions

Copy link
Copy Markdown

Codex AI review

No actionable findings.

Residual risk: success status is covered through direct plugin-hook tests, but not through an end-to-end durable runner lifecycle.

Reviewed commit f77d30690f9a2580a97ee27f1ede7e7019b41168. 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.

3 participants