feat(workflow): emit OpenTelemetry spans for runs and nodes by kwakayama · Pull Request #3888 · veryfront/veryfront-code

kwakayama · 2026-08-20T19:09:12Z

Closes #3871.

Problem

The workflow executor opened no OpenTelemetry spans. The agent runtime instruments itself
(agent.generate, agent.execution_loop, agent.tool_execute), so with
@veryfront/ext-observability-opentelemetry registered you got a flat scatter of
agent-internal spans with no workflow structure above them — no way to see which node a tool
call belonged to, or how long a run took.

The only hook available was onStepStart(nodeId, input) / onStepComplete(nodeId, output),
and it is not enough to build tracing on: onStepStart fires inside the step's async
execution scope while onStepComplete fires after the awaited body resolves, so a caller
cannot hold a span across the pair through normal context propagation. The reporter had to
reach into the OTel context manager's private _asyncLocalStorage and call enterWith()
directly.

The first commit here is a failing test that reproduces exactly that: one workflow, one
agent-style span opened inside the step, and the captured trace contains
["agent.generate"] — a single orphan, no parent.

What changed

workflow.run span wraps each execution, in WorkflowExecutor.executeRun — the single
point both start() and resume() funnel through.
workflow.node <id> span wraps each node, in DAGExecutor.executeNode. Dispatch moved
into an extracted dispatchNode so the span brackets the strategy call.
Retry span events in both retry loops (StepExecutor and executeCompositeNodeWithPolicy),
plus a final workflow.node.attempts attribute.
New addActiveSpanEvent export in otlp-setup.ts, mirroring the existing
setActiveSpanAttributes.

No public API change. onStepStart / onStepComplete keep their exact signatures and
firing order. They turned out not to need changing: withSpan activates its span through the
context manager, so anything opened inside the node body — every agent span — nests
automatically. The enterWith() workaround should now be deletable.

Design decisions worth knowing

The span opens at the DAG dispatch layer, not the step executor. The issue named
step-executor, but that only handles step nodes; a map or parallel node would have
produced children with no parent. executeNode is the one place all seven node types pass
through.

One trace per execution attempt, not per run. A run that pauses at a wait node or a
pending approval resumes later, possibly in another process; a span cannot stay open across
that. Resumed executions start a new trace and correlate through the workflow.run_id
attribute, which is on every span. Persisting trace context on the run and joining executions
with OTel span links is the intended follow-up — additive, and deliberately not here.

workflow.run_id is always the root, backend-persisted run id. Parallel, branch, and
sub-workflow nodes each synthesize a run record with a generated id that is never persisted
and cannot be looked up. Those ids would silently break correlation, so rootRunId is
threaded explicitly rather than read from the local run. It is a parameter and not an
instance field because DAGExecutor is shared across concurrent runs, where a field would
race. A sub-workflow's synthetic identity is recorded as workflow.sub_run_id /
workflow.sub_workflow_id instead.

Retries produce one span plus events, not a span per attempt. Both retry loops sit inside
the node span, so without events a node that failed for four minutes and then succeeded in
two seconds would render as one uneventful green span. The event carries attempt number,
backoff delay, and error message.

Span attributes carry identifiers only — never resolved step input or output. The issue
asked for input/output on spans. Agent spans in this codebase carry agent.id, agent.model
and timestamps and never message content; workflow inputs routinely carry customer data and
spans are exported to third-party backends. A test asserts no attribute contains a step
payload.

No new configuration. withSpan already no-ops when the extension is absent, and tracing
is already gated by the existing OTel environment variables.

Node ids are used verbatim in span names. A map over N items therefore produces N
distinct span names. This is a deliberate trade-off for a trace that reads at a glance in
Jaeger, which is the reporter's stated target; backends that aggregate by span name will see
high series counts from map fan-out. Documented in the extension README along with the span
volume hazard, since no framework-level cap is applied — sampling is the operator's call.

Reading the diff

workflow-executor.ts reports ~114 changed lines, but only four are real — the rest is
reindentation from wrapping the existing call in withSpan. Reviewing with
git diff -w on that file shows the actual change.

Tests

New src/workflow/executor/workflow-tracing.test.ts, 7 cases. The harness mirrors
wireTracingShim in src/server/bootstrap.ts exactly — real @opentelemetry/api, real
AsyncLocalStorageContextManager, real tracer provider. That matters: the one pre-existing
span test stubs context propagation with with: (_ctx, fn) => fn(), which can never observe
parentage. A stub here would have been a false green.

Covered: agent-span-nests-under-node and node-under-run with matching trace ids; composite
node types each emitting a span and every span carrying the root run id; retry event and
attempt count; skipped node flagged and replayed node emitting nothing; no payload leakage;
unchanged behaviour with no extension wired; and an import-parity guard.

That last one is worth calling out. Two different withSpan functions are exported from the
observability layer — one from otlp-setup, one from the tracing-manager index. The agent
runtime uses the former. If workflow code drifts to the other, spans still appear and span
counts still pass, but agent work silently stops nesting — the exact bug this PR fixes. The
guard pins that both sides import the same one.

Not in this PR

Persisting trace context on the run record and joining resumed executions with span links.
workflow.node.index / workflow.node.iteration attributes for map and loop children. The
index is already in the span name given the naming decision above, so these were dropped
rather than added speculatively.
Any cap, truncation, or sampling of span volume.
Metrics and logs; traces only.

Verification

deno task lint:ci — full gate.
deno test src/workflow/ — 74 files pass. src/workflow/blob/veryfront-cloud-storage.test.ts
fails, but it fails identically on a clean origin/main checkout; it is pre-existing and
unrelated to this change.

The workflow executor opened no spans, so agent spans arrived orphaned with no workflow structure above them. Callers could not tell which node produced a tool call, and the only hook available fired its start and complete halves in different async scopes -- so a span could not be held across the pair without reaching into the OTel context manager's private storage. Open a workflow.run span per execution and a workflow.node <id> span per node, using the same withSpan the agent runtime uses so agent work nests under its node through ordinary context propagation. The node span sits at the DAG dispatch layer rather than the step executor, so all seven node types are covered. workflow.run_id is always the root, backend-persisted run id: parallel, branch and sub-workflow nodes synthesise run records whose ids are never persisted, so the root id is threaded explicitly rather than read locally. It is a parameter and not a field because the DAG executor is shared across concurrent runs. Retries stay inside one node span, so each retry emits a span event with the attempt, delay and error -- otherwise a node that failed for minutes and then succeeded renders as one uneventful span. Span attributes carry identifiers only, never resolved step input or output. onStepStart and onStepComplete are unchanged.

coderabbitai · 2026-08-20T19:09:22Z

Important

🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fc698b6d-223b-43b3-b344-d6734f9ad3df

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-20T19:09:36Z

📦 Client bundle boundary

Entrypoint	Modules	Source size	Server leaks
`src/index.client.ts`	326	1948 KiB	✅ 0

A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in scripts/lint/client-bundle-baseline.json to burn down.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5468058261

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Review found that spans were always UNSET. Workflow failures are returned as a failed node state rather than thrown, so withSpan's catch never ran, and the async withSpan -- unlike withSpanSync -- never sets OK either. A failed run was indistinguishable from a successful one, leaving "which node failed" unanswerable from a trace. Node spans now carry workflow.node.status and set ERROR with the node's error text; the run span sets ERROR from the run control's onError callback. Also from review: - The composite retry path drew calculateRetryDelay twice, and it applies random jitter, so the recorded delay was never the delay slept. Draw once. - Composite retry telemetry had no coverage: deleting both call sites left every test green. Covered by a map node with retry. - workflow.run is not a trace root when the caller is traced -- it joins the caller's trace. The comment, README and a test all claimed otherwise. Behaviour kept, since a run belongs under the request that started it; claims corrected and the real behaviour pinned by a test. - README documents loop iteration cardinality alongside map. - The payload test scanned attributes but not event attributes, where the one free-form error string lives.

kwakayama · 2026-08-20T19:21:43Z

Updated after an independent review pass. ccc5f6a59 on top of 546805826.

The blocker it found: every span was UNSET, always. Workflow failures are returned as a failed node state rather than thrown, so withSpan's catch never ran — and the async withSpan, unlike withSpanSync, never sets OK either. A failed run was byte-identical to a successful one in the trace, and "which node failed" — the first question anyone takes to a trace — was unanswerable. That undercut the point of the feature. Node spans now carry workflow.node.status and set ERROR with the node's error text; the run span sets ERROR from the run control's onError.

Two more real defects:

The composite retry path called calculateRetryDelay twice, and it applies ±10% Math.random() jitter — so the recorded delay was never the delay actually slept. Now drawn once. The step path was already correct.
Composite retry telemetry had zero coverage: deleting both call sites left every test green. The retry test only exercised step. Now covered by a map node with retry.

One claim of mine was simply wrong. I documented workflow.run as a trace root. It isn't, when the caller is traced — start() dispatches inside the caller's async context, so a run started from an instrumented handler joins that request's trace. The behaviour is kept, because a run belongs under the request that triggered it, but the comment, the README and a test assertion all claimed otherwise. Claims corrected, and the real behaviour is now pinned by a test.

Also: README covers loop-iteration cardinality alongside map, and the payload test now scans event attributes too — where the one free-form error string lives.

Left open deliberately: a retried composite node emits N identically-named sibling child spans with no attempt marker. Fixing that means encoding attempt into generated child identity, which reopens the span-naming decision rather than settling it in a review.

Tests 7 → 10. deno task lint:ci green. src/workflow/ is 74 passed / 1 failed, and that failure is src/workflow/blob/veryfront-cloud-storage.test.ts, which fails identically on a clean origin/main — pre-existing and unrelated.

Retry spans need enough signal to explain retry behavior without exporting user-provided failure messages. The retry event now carries a bounded error classification shared by step and composite execution while preserving the recorded delay and attempt metadata. Constraint: Workflow telemetry must preserve the no-payload rule and avoid raw error messages.\nRejected: Redact and keep workflow.node.error | sanitized free text can still expose customer-specific failure detail.\nConfidence: high\nScope-risk: narrow\nDirective: Do not add free-form workflow retry error fields without a no-payload regression test.\nTested: deno test --preload=src/testing/preload.ts --no-check --allow-all src/workflow/executor/workflow-tracing.test.ts\nTested: deno test --preload=src/testing/preload.ts --no-check --allow-all src/workflow/executor/step-executor.test.ts src/workflow/executor/dag/index.test.ts src/workflow/executor/dag-executor.test.ts src/workflow/executor/workflow-executor.test.ts src/workflow/executor/workflow-tracing.test.ts\nTested: deno lint src/workflow/executor/retry-policy.ts src/workflow/executor/step-executor.ts src/workflow/executor/dag/composite-node-execution.ts src/workflow/executor/workflow-tracing.test.ts\nTested: deno check src/workflow/executor/retry-policy.ts src/workflow/executor/step-executor.ts src/workflow/executor/dag/composite-node-execution.ts src/workflow/executor/workflow-tracing.test.ts\nTested: deno fmt --check src/workflow/executor/retry-policy.ts src/workflow/executor/step-executor.ts src/workflow/executor/dag/composite-node-execution.ts src/workflow/executor/workflow-tracing.test.ts\nTested: deno test --preload=src/testing/preload.ts --no-check --allow-all tests/docs/guide-contracts.test.ts tests/docs/guide-content.test.ts\nTested: git diff --check\nNot-tested: Full repository test suite

Re-review found the jitter fix had no guard: reverting it to draw calculateRetryDelay twice left every test green, because the retry test read the attempt number but never the recorded delay, and at initialDelay 1 the jitter band is too narrow for any value assertion to discriminate. Stub Math.random with alternating draws at initialDelay 100 so a second draw is forced to differ: the slept value is 90, a fresh draw would report 110. Verified by mutation -- the test fails with the double-draw restored. Also document in the extension README that a cancelled run is not a failure (the in-flight node span ends ERROR with the reason, workflow.run stays unset), and that composite retry attempts appear as sibling spans told apart by status.

The generated reference embeds source line numbers, so adding the retry telemetry import to step-executor shifted getWorkflowTenant by one line and left docs/api-reference/veryfront/workflow.md stale. docs:api-reference:check gates ci (lint), so this would have failed CI. Regenerated with deno task docs.

kwakayama · 2026-08-20T19:33:07Z

Rebased onto @kojiwakayama's ef5fd5bdb rather than force-pushing over it. Two notes on that commit, both meant as useful rather than critical.

It would have failed CI as pushed. The generated API reference embeds source line numbers, so the added import shifted getWorkflowTenant by one line and left docs/api-reference/veryfront/workflow.md stale. docs:api-reference:check gates ci (lint). Regenerated in 047009198. The commit trailer lists lint, check, fmt and targeted tests — the docs gate isn't reachable from any of those, so there was no way to see it from there.

The delay guard is weaker than it looks. Math.random = () => 1 is a constant, so two draws return the same value and a double-draw is undetectable. I mutation-tested it: reintroducing recordCompositeRetry(node.id, attempt, calculateRetryDelay(attempt, retry), error) leaves "records the exact composite retry delay that was slept" passing, while the alternating-draw test fails. Both tests are kept — they prove different things. Yours proves the recorded delay was really slept (elapsed >= 22); mine proves it's the same draw that was slept.

On the substance — your change supersedes mine and I've kept it. I had put the raw error message on retry events. Review flagged that redaction is key-name driven, so free prose like rejected for bob@example.com passes straight through. A bounded retryTelemetryErrorType classification is the right answer, and "redact and keep" deserved the rejection it got.

Branch now:

047009198  docs(workflow): regenerate stale api reference
c762944fc  test(workflow): pin the retry delay that was actually slept
ef5fd5bdb  Keep workflow retry telemetry free of customer error text
ccc5f6a59  fix(workflow): surface node and run failures on their spans
546805826  feat(workflow): emit OpenTelemetry spans for runs and nodes

14 tracing tests pass. deno task lint:ci green end to end. src/workflow/ is 74 passed / 1 failed, that one being veryfront-cloud-storage.test.ts, which fails identically on clean origin/main.

Still open for a human: whether workflow.run should force a trace root with a link back to the caller, or keep joining the caller's trace as it does now.

kwakayama · 2026-08-20T19:33:22Z

Rebased onto ef5fd5bdb rather than force-pushing over it. Two notes on that commit, both meant as useful rather than critical.

It would have failed CI as pushed. The generated API reference embeds source line numbers, so the added import shifted getWorkflowTenant by one line and left docs/api-reference/veryfront/workflow.md stale. docs:api-reference:check gates ci (lint). Regenerated in 047009198. The commit trailer lists lint, check, fmt and targeted tests — the docs gate isn't reachable from any of those, so there was no way to see it from there.

The delay guard is weaker than it looks. Math.random = () => 1 is a constant, so two draws return the same value and a double-draw is undetectable. Mutation-tested: reintroducing recordCompositeRetry(node.id, attempt, calculateRetryDelay(attempt, retry), error) leaves "records the exact composite retry delay that was slept" passing, while the alternating-draw test fails. Both tests are kept — they prove different things. The constant-stub test proves the recorded delay was really slept (elapsed >= 22); the alternating one proves it is the same draw that was slept.

On the substance — that change supersedes mine and I kept it. I had put the raw error message on retry events. Review flagged that redaction is key-name driven, so free prose like rejected for bob@example.com passes straight through. A bounded retryTelemetryErrorType classification is the right answer, and "redact and keep" deserved the rejection it got.

Branch now:

047009198  docs(workflow): regenerate stale api reference
c762944fc  test(workflow): pin the retry delay that was actually slept
ef5fd5bdb  Keep workflow retry telemetry free of customer error text
ccc5f6a59  fix(workflow): surface node and run failures on their spans
546805826  feat(workflow): emit OpenTelemetry spans for runs and nodes

14 tracing tests pass. deno task lint:ci green end to end. src/workflow/ is 74 passed / 1 failed, that one being veryfront-cloud-storage.test.ts, which fails identically on clean origin/main.

Still open for a human: whether workflow.run should force a trace root with a link back to the caller, or keep joining the caller's trace as it does now.

kwakayama requested a review from kojiwakayama as a code owner August 20, 2026 19:09

chatgpt-codex-connector Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread src/workflow/executor/step-executor.ts Outdated

Comment thread src/workflow/executor/workflow-executor.ts

Comment thread src/workflow/executor/dag/composite-node-execution.ts Outdated

kojiwakayama and others added 3 commits August 20, 2026 21:24

kojiwakayama added this pull request to the merge queue Aug 20, 2026

Merged via the queue into main with commit 1f6b657 Aug 20, 2026
34 checks passed

kojiwakayama deleted the feat/workflow-otel-spans branch August 20, 2026 20:20

This was referenced Aug 20, 2026

fix(workflow): keep thrown error text off node and run spans #3895

Merged

docs(workflow): pin run identity in the domain glossary #3898

Merged

fix(observability): keep raw error text and stack traces off agent spans #3946

Merged

Navigation Menu

feat(workflow): emit OpenTelemetry spans for runs and nodes - #3888

feat(workflow): emit OpenTelemetry spans for runs and nodes#3888
kojiwakayama merged 5 commits into
mainfrom
feat/workflow-otel-spans

kwakayama commented Aug 20, 2026

Uh oh!

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 20, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwakayama commented Aug 20, 2026

Uh oh!

kwakayama commented Aug 20, 2026

Uh oh!

kwakayama commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kwakayama commented Aug 20, 2026

Problem

What changed

Design decisions worth knowing

Reading the diff

Tests

Not in this PR

Verification

Uh oh!

coderabbitai Bot commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 20, 2026

📦 Client bundle boundary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kwakayama commented Aug 20, 2026

Uh oh!

kwakayama commented Aug 20, 2026

Uh oh!

kwakayama commented Aug 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Aug 20, 2026 •

edited

Loading