Fix #319: record event runtime for events acked via stream.take()#701
Open
wbarnha wants to merge 1 commit into
Open
Fix #319: record event runtime for events acked via stream.take()#701wbarnha wants to merge 1 commit into
wbarnha wants to merge 1 commit into
Conversation
Stream.take() disables acks and acks its buffered events manually after the main iteration loop has already yielded. The main loop only calls on_stream_event_out when do_ack is set, so for take() the out-event was delivered by Stream.ack() -- but without the sensor state returned by on_stream_event_in. on_stream_event_out needs that state to compute the runtime, so events_runtime was never populated and metrics such as events_runtime_ms_sum stayed at zero. Capture the per-sensor state on the event in SensorDelegate.on_stream_event_in (covers both the Python and Cython stream iterators, which share that delegate) and forward it from Stream.ack(), clearing it afterwards so a second ack cannot double-count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHPL4VFWQRQPpjR1gXSKyL
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #701 +/- ##
==========================================
+ Coverage 94.14% 94.15% +0.01%
==========================================
Files 104 104
Lines 11136 11141 +5
Branches 1201 1201
==========================================
+ Hits 10484 10490 +6
+ Misses 551 550 -1
Partials 101 101 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
When an agent consumes a stream with
stream.take(max_, within=...), the event runtime metric (events_runtime, surfaced e.g. as Prometheusevents_runtime_ms_sum) stayed at zero.Fixes #319.
Why
take()disables acking (enable_acks = False) and acks its buffered events manually in itsfinally, long after the main iteration loop yielded them. The main loop only callson_stream_event_outwhendo_ackis set, so for thetake()path the out-event is delivered byStream.ack()instead.But
Stream.ack()calledon_stream_event_out(tp, offset, self, event)without the sensor state.Monitor.on_stream_event_outneeds that state (which carriestime_in) to computetime_total; withstate=Noneit does nothing, so no runtime was ever recorded for taken events.How
SensorDelegate.on_stream_event_innow stashes the per-sensor state dict on the event itself (event.sensor_state). Both the Python (_py_aiter) and Cython (_cython/streams) iterators call this same delegate method, so the state is captured regardless of which iterator runs — no.pyxrecompile needed.Stream.ack()readsevent.sensor_state, forwards it toon_stream_event_out, then clears it so a second ack can't double-count.Event/EventTgain asensor_stateattribute (initialised toNone, added toEventT.__slots__).The normal (acks-enabled) path is unchanged: the main loop still fires
on_stream_event_outwith its local state and never routes throughStream.ack().Test
Added
test_take__records_event_runtimeintests/functional/test_streams.py: attaches aMonitor, sends a value, consumes one buffered batch viatake(), and assertsmonitor.events_runtimeis non-empty. Verified it fails on master (assert deque([])) and passes with the fix, under both the Cython andNO_CYTHONstream iterators. Updated theStream.ackunit test for the new forwarded/clearedsensor_stateargument.🤖 Generated with Claude Code
Generated by Claude Code