Skip to content

Parent durable OTel spans to shared execution trace - #685

Draft
ayushiahjolia wants to merge 1 commit into
mainfrom
otel-shared-execution-trace
Draft

Parent durable OTel spans to shared execution trace#685
ayushiahjolia wants to merge 1 commit into
mainfrom
otel-shared-execution-trace

Conversation

@ayushiahjolia

Copy link
Copy Markdown
Contributor

Issue #, if available: #674

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@github-actions

This comment has been minimized.

@github-actions

Copy link
Copy Markdown
Contributor

Claude AI review

This PR reparents durable OTel spans (Workflow, Invocation, operations, attempts) onto one shared execution trace anchored to a propagated backend parent or a deterministic synthetic root, adds a DurableSampler/sampling-intent mechanism to resolve sampling once per invocation, and restructures context extractors to return a structured ExtractedContext. The trace-context resolution (canonical_trace_id, ExecutionTraceContext.resolve), sampling precedence, id namespacing (derive_execution_root_span_id), and the store_sampling_intent/DurableSampler wiring are internally consistent and well covered by the updated tests.

One correctness issue: both plugins now return early when execution_arn is missing without clearing _tracing_enabled, which _bind_sdk_tracer() set to True a few lines earlier. Because subsequent operation/user-function hooks only gate on _tracing_enabled, this leaves the plugin in a half-enabled state for the rest of the invocation, contradicting the "telemetry is disabled for this invocation" intent. See the two inline comments:

  • packages/aws-durable-execution-sdk-python-otel/src/aws_durable_execution_sdk_python_otel/invocation_plugin.py:463
  • packages/aws-durable-execution-sdk-python-otel/src/aws_durable_execution_sdk_python_otel/execution_plugin.py:340

Residual test risk: the new test_invocation_start_without_execution_arn_disables_tracing tests (in both plugins) only drive on_invocation_start/on_invocation_end, so they don't exercise the operation hooks and don't catch the stale-_tracing_enabled behavior. Adding a case that fires on_operation_start/on_user_function_start after a no-ARN start would guard this path.

Reviewed commit 9ff9f5561b298221fbb08b9620043fb7a2cbb7a7. Workflow run

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-execution-trace branch from 9ff9f55 to b645aea Compare August 27, 2026 05:06
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 27, 2026 05:08 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-execution-trace branch from b645aea to 337e5f5 Compare August 27, 2026 18:00
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 27, 2026 18:04 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-execution-trace branch from 337e5f5 to c21cf27 Compare August 27, 2026 18:38
@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-execution-trace branch from c21cf27 to 2b6ff70 Compare August 27, 2026 20:26
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 27, 2026 20:30 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-execution-trace branch from 2b6ff70 to 3ddb25e Compare August 27, 2026 21:19
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 27, 2026 21:20 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-execution-trace branch from 3ddb25e to bff3b67 Compare August 27, 2026 21:44
@ayushiahjolia
ayushiahjolia temporarily deployed to ai-pr-review-runtime August 27, 2026 22:44 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@ayushiahjolia
ayushiahjolia force-pushed the otel-shared-execution-trace branch from bff3b67 to e6f3f01 Compare August 27, 2026 23:46
@ayushiahjolia
ayushiahjolia deployed to ai-pr-review-runtime August 27, 2026 23:55 — with GitHub Actions Active
return self.has_valid_trace_id and self.has_valid_parent_span_id


ContextExtractor = Callable[["InvocationStartInfo"], ExtractedContext | None]

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.

Codex AI review

[P1] Preserve the existing ContextExtractor contract. This exported API previously returned an OpenTelemetry Context; _ensure_extracted_context now rejects every existing custom extractor, causing telemetry to be disabled after the plugin executor swallows the TypeError. Accept and adapt legacy Context results, including their trace state and sampling flags, while introducing ExtractedContext through a compatible migration path and test that behavior.

after_resume = next(span for span in spans if span.name == "otel-after-resume")

assert len(invocations) >= 2
assert len(waits) >= 2

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.

Codex AI review

[P1] This assertion fails for the parameterized ExecutionOtelPlugin. That plugin deliberately drops unfinished operation spans on a PENDING invocation, so only the resumed wait span is exported and len(waits) is 1; two spans are produced only by InvocationOtelPlugin. Split the expected count by plugin type, or change the execution-view lifecycle if exporting suspended segments is intended.

@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

Found two blocking issues: a public context-extractor compatibility regression and a failing parameterized end-to-end assertion.

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

1 participant