feat(blocks): add instrumentWorkflowRun for Cloudflare Workflow tracing - #414
feat(blocks): add instrumentWorkflowRun for Cloudflare Workflow tracing#414hugo-ccabral wants to merge 1 commit into
Conversation
instrumentWorker assumes a {fetch}/ctx.waitUntil lifecycle that Workflow
run(event, step) entrypoints don't have. Exports bootObservability and
extracts flushObservability from instrumentWorker's finally block so
Workflow entrypoints can boot the same observability stack and flush
explicitly (a plain await, no ctx.waitUntil) via the new
instrumentWorkflowRun helper.
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/blocks/src/sdk/otel.test.ts">
<violation number="1" location="packages/blocks/src/sdk/otel.test.ts:554">
P3: The test name promises "and still flushes", but the body only asserts the rejection and configures no OTLP adapter, so the flush-on-error behavior it claims to cover is never actually verified. Either rename it to reflect that it only checks error propagation, or configure a metrics fetch impl and assert the POST fires after rejection as the name implies.</violation>
</file>
<file name="packages/blocks/src/sdk/otel.ts">
<violation number="1" location="packages/blocks/src/sdk/otel.ts:517">
P2: A synchronous exception from `fn` skips the flush and escapes synchronously, so buffered telemetry from earlier workflow work can be lost. Start the callback in a promise chain so both synchronous throws and rejected promises reach the same `finally`.</violation>
<violation number="2" location="packages/blocks/src/sdk/otel.ts:517">
P2: Workflow runs get one explicit flush on the way out, but the OTLP meter (and by extension the same per-isolate pattern) enforces a 5s flush cooldown in `flush()`. When a workflow replays legs within that window, the final `await flushObservability()` is a silent no-op, so spans/metrics logged in the last leg can be lost when the isolate is torn down — exactly the durability this `finally` flush is meant to provide. Consider exposing a force/ignore-cooldown flush path (or raising the cap) for the Workflow entrypoint so the on-exit flush actually ships buffered data.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ): Promise<T> { | ||
| configureTracer(buildOtelApiTracer()); | ||
| bootObservability(options ?? {}, env); | ||
| return fn().finally(() => flushObservability()); |
There was a problem hiding this comment.
P2: A synchronous exception from fn skips the flush and escapes synchronously, so buffered telemetry from earlier workflow work can be lost. Start the callback in a promise chain so both synchronous throws and rejected promises reach the same finally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/blocks/src/sdk/otel.ts, line 517:
<comment>A synchronous exception from `fn` skips the flush and escapes synchronously, so buffered telemetry from earlier workflow work can be lost. Start the callback in a promise chain so both synchronous throws and rejected promises reach the same `finally`.</comment>
<file context>
@@ -446,34 +446,77 @@ export function instrumentWorker(
+): Promise<T> {
+ configureTracer(buildOtelApiTracer());
+ bootObservability(options ?? {}, env);
+ return fn().finally(() => flushObservability());
+}
+
</file context>
| return fn().finally(() => flushObservability()); | |
| return Promise.resolve().then(fn).finally(() => flushObservability()); |
| ): Promise<T> { | ||
| configureTracer(buildOtelApiTracer()); | ||
| bootObservability(options ?? {}, env); | ||
| return fn().finally(() => flushObservability()); |
There was a problem hiding this comment.
P2: Workflow runs get one explicit flush on the way out, but the OTLP meter (and by extension the same per-isolate pattern) enforces a 5s flush cooldown in flush(). When a workflow replays legs within that window, the final await flushObservability() is a silent no-op, so spans/metrics logged in the last leg can be lost when the isolate is torn down — exactly the durability this finally flush is meant to provide. Consider exposing a force/ignore-cooldown flush path (or raising the cap) for the Workflow entrypoint so the on-exit flush actually ships buffered data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/blocks/src/sdk/otel.ts, line 517:
<comment>Workflow runs get one explicit flush on the way out, but the OTLP meter (and by extension the same per-isolate pattern) enforces a 5s flush cooldown in `flush()`. When a workflow replays legs within that window, the final `await flushObservability()` is a silent no-op, so spans/metrics logged in the last leg can be lost when the isolate is torn down — exactly the durability this `finally` flush is meant to provide. Consider exposing a force/ignore-cooldown flush path (or raising the cap) for the Workflow entrypoint so the on-exit flush actually ships buffered data.</comment>
<file context>
@@ -446,34 +446,77 @@ export function instrumentWorker(
+): Promise<T> {
+ configureTracer(buildOtelApiTracer());
+ bootObservability(options ?? {}, env);
+ return fn().finally(() => flushObservability());
+}
+
</file context>
| expect(fn).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| it("rethrows fn's error and still flushes", async () => { |
There was a problem hiding this comment.
P3: The test name promises "and still flushes", but the body only asserts the rejection and configures no OTLP adapter, so the flush-on-error behavior it claims to cover is never actually verified. Either rename it to reflect that it only checks error propagation, or configure a metrics fetch impl and assert the POST fires after rejection as the name implies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/blocks/src/sdk/otel.test.ts, line 554:
<comment>The test name promises "and still flushes", but the body only asserts the rejection and configures no OTLP adapter, so the flush-on-error behavior it claims to cover is never actually verified. Either rename it to reflect that it only checks error propagation, or configure a metrics fetch impl and assert the POST fires after rejection as the name implies.</comment>
<file context>
@@ -524,3 +529,60 @@ describe("instrumentWorker — OTLP/HTTP error-log channel wiring", () => {
+ expect(fn).toHaveBeenCalledOnce();
+ });
+
+ it("rethrows fn's error and still flushes", async () => {
+ const fn = vi.fn().mockRejectedValue(new Error("workflow step failed"));
+
</file context>
Summary
instrumentWorkerassumes a{fetch}/ctx.waitUntilrequest lifecycle that Cloudflare Workflowrun(event, step)entrypoints don't have, so Workflow-heavy consumers currently have no way to reuse the observability stack for background jobs.bootObservability(already transport/lifecycle-agnostic — it never itself callsctx.waitUntil) and extractsflushObservability()out ofinstrumentWorker'sfinallyblock into its own function, preserving the existing "only callctx.waitUntilwhen an adapter is actually configured" behavior.instrumentWorkflowRun<T>(env, options, fn): boots observability and runsfn, flushing with a plainawaiton the way out (noctx.waitUntil— Workflows have no such hook and aren't latency-sensitive). Deliberately does not open a root span aroundfn(), since code outsidestep.do/step.sleepinrun()re-executes on every hibernate/wake replay — a span opened there would get a new random trace ID per replay leg instead of one coherent trace. Consumers should span individualstep.do(...)callbacks with the existingwithTracing, tagged withevent.instanceId.Test plan
bun run --filter='./packages/blocks' typecheckpassespackages/blocks/src/sdk/otel.test.ts— 28/28 passing, including 4 new tests coveringflushObservability's no-op case,instrumentWorkflowRun's return/rethrow/flush behavior, and OTLP metrics wiring through the new entrypointpackages/blockssuite run — no new failures (23 pre-existing failures incms/loader.test.ts/layoutCacheRace.test.tsare unrelatedURLPatternWeb API gaps in this local Bun/Node runtime, not caused by this change)🤖 Generated with Claude Code
Summary by cubic
Adds
instrumentWorkflowRunto enable OTLP metrics/logs/traces in Cloudflare Workflowrun(event, step)withoutctx.waitUntil. Also exportsbootObservabilityand a newflushObservabilityfor reuse across non-Worker entrypoints.New Features
instrumentWorkflowRun<T>(env, options, fn): boots observability, runsfn, and flushes withawaiton exit. No root span is opened; span individualstep.do(...)withwithTracingand tagevent.instanceId.bootObservabilityandflushObservability()for direct control in Workflow and other non-fetchcontexts.Refactors
flushObservability()and updatedinstrumentWorkerto call it viactx.waitUntilonly when any adapter is configured (meter/log/tracer).Written for commit 949ae42. Summary will update on new commits.