Parent durable OTel spans to shared execution trace - #685
Conversation
GitHub Actions
Error
77c426e to
9ff9f55
Compare
GitHub Actions
Inactive
![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.
Claude AI reviewThis 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 One correctness issue: both plugins now
Residual test risk: the new Reviewed commit |
9ff9f55 to
b645aea
Compare
GitHub Actions
Error
| if isinstance(extracted, Context): | ||
| span_context = trace.get_current_span(extracted).get_span_context() | ||
| if not span_context.is_valid: | ||
| return None | ||
| sampling = ( | ||
| Sampling.SAMPLED | ||
| if bool(span_context.trace_flags & trace.TraceFlags.SAMPLED) | ||
| else Sampling.NOT_SAMPLED | ||
| ) | ||
| return ExtractedContext( | ||
| trace_id=span_context.trace_id, | ||
| parent_span_id=span_context.span_id, | ||
| sampling=sampling, | ||
| ) |
There was a problem hiding this comment.
Codex AI review
P2: This compatibility path discards everything in the legacy Context except IDs and sampled flags. Baggage, custom context keys, and trace_state previously reached parent span creation and samplers; a recording-but-unsampled span is also converted into authoritative NOT_SAMPLED, causing durable spans to be dropped. Preserve the original context metadata and recording decision through parent construction and sampling resolution, or treat legacy contexts as a versioned breaking change instead of silently normalizing them.
| self._execution_trace_id = canonical_trace_id( | ||
| extracted=self._extracted_context, | ||
| execution_arn=self._execution_arn, | ||
| execution_start_time=info.execution_start_time, | ||
| ) |
There was a problem hiding this comment.
Codex AI review
P2: Making the backend X-Ray root authoritative is cross-component, cross-invocation behavior, but the PR adds only in-process tests with fabricated headers and exporters. It does not verify that real durable wait/resume invocations and ADOT propagation produce one stable trace. Repository policy requires an e2e test for this public workflow change; add one under tests/e2e/ covering at least two invocations and asserting Workflow, Invocation, and operation trace IDs and parentage. Coordinate any separate conformance requirement rather than modifying conformance tests here.
Codex AI reviewFound two medium-severity issues affecting context compatibility and required end-to-end coverage. Reviewed commit |
1 participant
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.