Skip to content

fix(otel): require execution start time for trace IDs - #660

Open
zhongkechen wants to merge 1 commit into
mainfrom
codex/require-otel-execution-start-time
Open

fix(otel): require execution start time for trace IDs#660
zhongkechen wants to merge 1 commit into
mainfrom
codex/require-otel-execution-start-time

Conversation

@zhongkechen

Copy link
Copy Markdown
Contributor

Summary

  • require an execution start timestamp when deriving the durable OpenTelemetry trace ID
  • remove the wall-clock fallback that could generate a different trace ID on replay
  • disable telemetry for an invocation when InvocationStartInfo.execution_start_time is missing
  • add coverage for the trace-ID helper and both OTel plugin implementations

Why

The durable trace ID embeds the execution start time in its first 32 bits. Falling back to datetime.now() when the service does not provide that timestamp makes the supposedly deterministic trace ID invocation-dependent and can split one durable execution across multiple traces.

The core plugin contract remains optional because other instrumentation plugins may not require this field. The OTel plugins enforce it at invocation start. Since plugin exceptions are intentionally swallowed by the core executor, they fail closed by leaving tracing disabled and logging an actionable warning rather than raising into a partially initialized plugin lifecycle.

Testing

  • 149 OpenTelemetry tests passed
  • mypy passed across 23 source files
  • Ruff and formatting checks passed

@zhongkechen
zhongkechen temporarily deployed to ai-pr-review-runtime August 19, 2026 21:44 — with GitHub Actions Inactive
@zhongkechen
zhongkechen deployed to ai-pr-review-runtime August 19, 2026 21:44 — with GitHub Actions Active
@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

No actionable findings. Residual risk: tests were not executed due to the read-only review constraint.

Reviewed commit e10dc12690cbaa188fbfc8d32e076742ff7b94fe. Workflow run

@github-actions

Copy link
Copy Markdown
Contributor

Claude AI review

No actionable findings.

This is a focused, correct fix. Requiring execution_start_time when deriving the durable OTel trace ID and removing the datetime.now() fallback eliminates the real determinism bug: the trace ID's 32-bit time prefix is now taken from the replay-stable checkpointed EXECUTION start_timestamp instead of a per-invocation wall clock, so one durable execution no longer splits across multiple traces.

Verification performed:

  • Both callers of _to_otel_trace_id (execution_plugin.py:287, invocation_plugin.py:409) are reached only after the new info.execution_start_time is None guard, so no path passes None; the retained runtime ValueError is consistent defensive validation for the tightened non-optional signature.
  • The early return in on_invocation_start leaves _tracing_enabled False (set by the preceding _reset_state()), so every downstream hook and on_invocation_end short-circuits cleanly and no OTel context is attached — the _assert_otel_context_balanced fixture invariant holds.
  • import datetime remains used in both plugins; in deterministic_id_generator.py the now-unused UTC import is correctly dropped while datetime stays referenced by the annotation — no dangling or unused imports.
  • The three added tests exercise the ValueError path and both plugins' fail-closed disable path and match the actual message/warning strings.

Residual test/behavior risk (not a defect): the fix is deliberately fail-closed, so when the service omits StartTimestamp or the EXECUTION operation is absent from the first page of results (state.py get_execution_operation can return None for large/paged executions), telemetry is disabled for that invocation and a WARNING is logged on each affected invocation. This is an intentional, documented tradeoff (a dropped span is preferable to a non-deterministic trace ID), but for long-running executions whose EXECUTION operation is routinely paged out it can mean repeated per-invocation warnings and no emitted spans. No test covers that specific "start time consistently unavailable across a multi-invocation execution" scenario.

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