Repository navigation
fix(otel): preserve replay completion telemetry - #758
zhongkechen wants to merge 2 commits into
Conversation
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.
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.
| with self._operation_spans_lock: | ||
| if info.operation_id in self._ended_operation_ids: | ||
| 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.
| for operation in pending: | ||
| self._render_operation_end(operation) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in a5114c4. Both views now isolate completion failures during invocation End, allow one bounded ordinary retry, and finish other completions and root spans before reporting errors. Control exceptions are not retried or swallowed. Public-handler, nested-parent and permanent-failure regressions pass.
This comment has been minimized.
This comment has been minimized.
| for pending_end in pending_ends.values(): | ||
| self._render_operation_end(pending_end) |
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.
| if parent_context is not None: | ||
| self._operation_spans[parent_id] = DurableParentSpan( | ||
| parent_context, start_time=parent.start_time | ||
| ) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| for pending_end in pending_ends.values(): | ||
| self._render_operation_end(pending_end) |
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.
| parent = self._input_parent_refs.get(parent_id) | ||
| if parent is None or parent_id in self._operation_spans: | ||
| continue | ||
| parent_context = self._operation_link_context(parent_id) | ||
| if parent_context is not None: | ||
| self._operation_spans[parent_id] = DurableParentSpan( | ||
| parent_context, start_time=parent.start_time |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| # Registration and dequeue share the lock, so an arriving completion | ||
| # either sees its parent or is drained here. Do not hold it in callbacks. | ||
| for pending_end in pending_ends.values(): | ||
| self._render_operation_end(pending_end) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| except BaseException as error: | ||
| if finalization is None: | ||
| raise | ||
| with self._lock: | ||
| finalization.operation_failed(info.operation_id, error) |
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.
| self._operation_spans[parent_id] = DurableParentSpan( | ||
| parent_context, start_time=parent.start_time | ||
| ) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| for pending_end in pending_ends.values(): | ||
| self._render_operation_end(pending_end) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| except BaseException as error: | ||
| if finalization is None: | ||
| raise | ||
| with self._lock: | ||
| finalization.operation_failed(info.operation_id, error) |
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.
Keep completion snapshots and export deduplication in each OTel invocation. Complete reentered replay segments, preserve missing-parent completions, and keep failed span starts retryable. Isolate plugin error snapshots and flush attempts by provider. Restore main-only CI filters; retain testing-wheel support downstream. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| parent_context = self._operation_link_context(parent_id) | ||
| if parent_context is not None: | ||
| self._operation_spans[parent_id] = DurableParentSpan( | ||
| parent_context, start_time=parent.start_time | ||
| ) |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_tkwzf5lq7oyzoym3afwwuxplhb
[P2] Materialize pending parents before installing fallback references
When the full input snapshot contains updated terminal context P, terminal child C, and an unavailable ancestor of P, this loop installs a DurableParentSpan for P before processing P's deferred completion. _finish_operation_end then treats that placeholder as an existing span: its end() does nothing, but P enters _ended_operation_ids without being exported. Process pending ancestor completions before installing fallback-only references. Extend the nested-fallback regression with the full operations snapshot populated; its current empty snapshot bypasses this path.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| for pending_end in pending_ends.values(): | ||
| self._render_operation_end(pending_end) |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_opkwyjxdk23t3ijca65noht5av
[P2] Isolate deferred export failures from parent scope setup
This drain runs inside on_user_function_start, before _attach_context. If a callback completed during suspension and its processor raises when replay registers the parent, _render_operation_end re-raises because _finalization is still None. Core contains the exception and executes the parent's body without attaching its OTel context, so user-created spans and correlated logs use the enclosing context. Later completion retries cannot repair that parentage. Contain ordinary failures per pending completion, retain their retry state, and allow parent setup to finish; add a replay regression with a processor that fails once.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
| except BaseException as error: | ||
| if finalization is None: | ||
| raise | ||
| with self._lock: | ||
| finalization.operation_failed(info.operation_id, error) |
There was a problem hiding this comment.
Codex AI review · Finding arf_v1_wh6we4ebru4v7r5faoxepdlqa5
[P2] Preserve non-retryable control failures through cleanup
If a processor raises SystemExit or KeyboardInterrupt during a virtual child's normal terminal hook, this branch re-raises while leaving the completion pending. Core then invokes invocation End during exception cleanup. Its fresh InvocationFinalization has no record of the control failure, so it recreates the span and calls the processor again, potentially repeating partial delivery despite the control signal. Invocation view has the same path. Track non-retryable failures for the whole invocation and exclude them from subsequent drains; add a public virtual-child regression asserting one processor attempt and propagation of the original exception.
To suppress repeats, reply /ai verdict deferred, /ai verdict false-positive, or /ai verdict out-of-scope on the first nonblank line. Add rationale on later lines. A new /ai verdict accepted allows future repeats again.
Codex AI reviewFour P2 findings remain:
Static review only; tests were not executed under the review restrictions. Suppressed repeats
Reviewed commit |
A callback can complete while a durable function is suspended, and replay may never visit it. This change preserves telemetry for visible terminal operations using the existing plugin snapshots and hooks.
Changes
OperationInfo.from_operationconstructs the requested info subclass and copies mutable error stack traces. Terminal hooks use this conversion, so plugin mutations cannot alter checkpointed errors.Stack and scope
main → #758 → #795 → #796
The testing-library implementation is unchanged here, and installed-wheel checks use the published testing library. #795 carries replay visibility and successful-input acknowledgment fixes, local testing-wheel installation, and the CI filters needed by the remaining stack. All five workflow PR filters in #758 target only
main. #796 adds the conformance handlers and qualification coverage.Validation and limits
Validation on Python 3.13.17 passes:
The stack retains linear history. The additional #758 fix is carried into #795/#796 by rebasing their unchanged layer contents; independent tree checks verify those boundaries. Commit-message validation passes, with the accepted documentation suggestion and its coauthor attribution preserved. GitHub CI runs separately on the new pushed heads.
A processor can deliver a span and then raise, so retrying partial delivery can export it again. Deduplication is invocation-scoped; failed delivery can still be exported again after redelivery because durable state has no exporter acknowledgment. Shared conformance #131 remains a merge prerequisite. The separate virtual-FLAT replay persistence issue JS #997 remains outside this change.