Do not open a spurious empty WorkflowRun span on no-work wakeups in the streaming run loop - #739
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts streaming run-loop telemetry so a workflow_invoke (WorkflowRun) span is only created when there is actual work (unprocessed messages) to execute, eliminating zero-superstep spans caused by idle/no-work wakeups and aligning behavior with the existing lockstep implementation.
Changes:
- Move
telemetry.StartWorkflowRunandEventWorkflowStartedinside theHasUnprocessedMessages()branch instreamingRunEventStream.runLoop(). - Guard run-span finalization (
EventWorkflowCompleted+End()) behind arunActivity != nilcheck. - Add a regression test ensuring a no-work wakeup does not emit any
workflow_invokespans.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| workflow/internal/execution/eventstream.go | Avoids creating WorkflowRun spans on no-work wakeups by starting/ending spans only when work is present. |
| workflow/internal/execution/eventstream_test.go | Adds a telemetry-focused regression test to assert no workflow_invoke spans are created on a no-work wakeup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
The streaming run loop opened a WorkflowRun span (StartWorkflowRun + EventWorkflowStarted) unconditionally on every wakeup, and emitted EventWorkflowCompleted + End() unconditionally, even when the iteration had no unprocessed messages to run. No-work wakeups are reachable: the loop is signaled when only HasUnservicedRequests() is true, and checkpoint restore always signals the run loop regardless of queued messages. Each such wakeup produced a zero-superstep workflow_invoke span. Move span creation inside the HasUnprocessedMessages branch, co-located with the StartedEvent emission that was already guarded there, and only finalize the span when one was actually started (guard on nil runActivity). This mirrors the lockstep implementation, which already uses a nil runActivity guard, keeping the two paths consistent.
9b95de4 to
8e0a576
Compare
Parity Review — PR #739Scope: Internal observability fix — removes a spurious empty Verdict: ✅ No parity issues.
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
| cycleCtx := ctx | ||
| var runActivity *observability.Activity |
There was a problem hiding this comment.
These declarations are not necessary if the runActivity.AddEvent and runActivity.End calls are moved inside the s.stepRunner.HasUnprocessedMessages block.
What
In
workflow/internal/execution/eventstream.go,streamingRunEventStream.runLoop()opened a WorkflowRun span on every wakeup: it calledtelemetry.StartWorkflowRun+runActivity.AddEvent(EventWorkflowStarted)unconditionally, andAddEvent(EventWorkflowCompleted)+End()unconditionally. Only the actual superstep work and theStartedEventwere guarded byif s.stepRunner.HasUnprocessedMessages(), with the existing comment noting that events should be emitted "only when there's actual work to process, to avoid spurious events on no-work loop iterations." The telemetry span was left outside that guard.No-work wakeups are reachable in normal operation:
HasUnservicedRequests()is true (no messages queued), andEach such wakeup therefore produced a zero-superstep
workflow_invokespan with noexecutor.processchildren.This change moves
StartWorkflowRun+EventWorkflowStartedinside theHasUnprocessedMessages()branch, co-located with theStartedEventemission, and finalizes the span (EventWorkflowCompleted+End()) only when a run activity was actually started, guarding on a nilrunActivity. No-work iterations now create no span.Why
This mirrors the sibling lockstep implementation (
lockstepRunEventStream) in the same file, which already opens the run span lazily viastartRunActivity()and guards finalization withif runActivity != nil. Aligning the streaming path removes an inconsistency between the two execution modes and keeps WorkflowRun span semantics matching the .NET/Python behavior, where a run/invoke activity corresponds to an actual unit of work rather than an idle wakeup. It also brings the telemetry in line with theStartedEventguard that already lived in this loop.Tests
Added
TestStreamingRunEventStream_NoWorkWakeupDoesNotOpenWorkflowRunSpanto the package's canonicaleventstream_test.go. It builds a telemetry-enabled workflow with an in-memory recording tracer, starts the streaming run loop, and triggers a no-work wakeup (SignalInputwhileHasUnprocessedMessages()reports false, as in a checkpoint restore whose only state is an unserviced request). It synchronizes on the loop's internal halt signal, then asserts zeroworkflow_invokespans. The test fails before the fix (count = 1) and passes after.go build ./...,go vet ./workflow/internal/execution/..., andgo test ./workflow/internal/execution/... -raceall pass.