Repository navigation
fix(observability)!: write events inline so overlapping runs keep their events - #1741
Conversation
…r events Every top-level run started and shut down the one process-wide Observer, so the first run to finish stopped recording for every other run, and a run on another thread crashed in shutdown() awaiting a task from a different loop. - Writers are synchronous and called on the emitting thread, under a lock per writer and per inline listener; runs no longer start or stop the Observer. Writers start on the first event and shut down at exit. - configure_writers can be called at any time; replaced writers shut down. - JsonlWriter closes a session's file once session.completed is written. - QueuePolicy, ensure_started and publish_event_nowait are removed; publish_event and the Writer protocol are synchronous. Closes #1605
…ency # Conflicts: # packages/railtracks/tests/unit_tests/observability/test_jsonl_writer.py
claude said the fix is cheap: Raise a TypeError in _start_writers or configure_writers when inspect.iscoroutinefunction(writer.write) (or start/shutdown) is true. |
…ency # Conflicts: # packages/railtracks/tests/unit_tests/observability/test_jsonl_writer.py
Writers now run inline on the publishing thread, so an async start, write or shutdown would never be awaited and its events would be dropped silently. configure_writers raises a TypeError naming the class and method instead, and leaves the current writers in place.
|
Thanks Guan, all three were good catches. Here's what changed:
|
CoronRing
left a comment
There was a problem hiding this comment.
I love the changes! Thanks for the quick turnaround, this is good to go. All three points are handled, and the async writer check is exactly what I was hoping for: an old-style writer now fails loudly at configure_writers with the class and method in the message, and the writers already in place stay put instead of being half-replaced.
The shutdown() docstrings now make it clear that it stops recording for the whole process and can't be restarted, which is the part a user would trip on. Your reasoning on is_observing makes sense to me too: it only does what ensure_started() already did at the start of every run on main, RAILTRACKS_DISABLE_EVENTS still turns it off, and the follow-up that starts writers in configure_writers removes the side effect for good.
The new "Customize event writers" paragraph reads well. It says the writers must be sync, run on the emitting thread one call at a time, and should keep write quick, which is all someone writing a custom writer needs to know.
One small thing, fine as part of the follow-ups: publish loops over a snapshot of self._writers, so if configure_writers swaps writers mid-publish, that event still goes to the old writer after it has been shut down. For JsonlWriter this reopens the scope's file in append mode and leaves the handle open. A closed flag on _Sink that _shutdown_writer sets and __call__ checks under the lock would close the gap.
The branch is a couple of commits behind main (both unrelated), so just update it before merging. I'll close #1683 once this lands.
Ship it! ٩(◕‿◕)۶
Fixes #1605.
When top-level runs overlap, from concurrent
ainvokecalls on one loop orinvokecalls in a thread pool, the first run to finish shut down the shared Observer. The other runs lost the rest of their events, and a run on another thread crashed inshutdown().Changes
JsonlWritercloses a session's file when itssession.completedis written.configure_writerscan be called at any time and shuts down the writers it replaces.Breaking change
start(),write(event)andshutdown()methods.configure_writersraises aTypeErrorfor a writer with async methods, and the "Customize event writers" docs now say so.QueuePolicy,ensure_startedandpublish_event_nowaitare removed, andpublish_eventandshutdownare no longer async.Testing
test_concurrent_runs.pycovers both cases from the issue.Follow-ups
Each of these will be a separate PR:
emitandemit_nowaitinto one synchronousemit. They're identical now that publishing is synchronous, so this removes theawaitfrom everyemitcall site.configure_writersis called, and keep lazy creation only for the defaultJsonlWriter. This removes the Observer's "not started" state; decide there whetherconfigure_writers([])means no writers.