Adds loop delay monitor and logger #138
Conversation
Defaults to capturing any delays of more than a second
![modal-pr-review-automation[bot]](https://avatars.githubusercontent.com/in/335990?s=60&v=4){kind=small}
synchronicity/synchronizer.py
Outdated
| t0 = time.monotonic() | ||
| await asyncio.sleep(self.loop_delay_monitor_period) | ||
| duration = time.monotonic() - t0 | ||
| delay = duration - self.loop_delay_monitor_period |
There was a problem hiding this comment.
Could our estimate of the true time the event loop was blocked be off by up to loop_delay_monitor_period? In other words, is the right way to think about it that another task could start blocking immediately after we sleep, or immediately before we try to wake, and we wouldn't be able to tell the difference the way we're calculating / reporting the delay here?
(This is mostly a question for my own understanding — it's probably not critical to report metrics exactly here).
There was a problem hiding this comment.
Yes, technically the event loop could have been blocked for up to loop_delay_monitor_period while this coroutine is sleeping and we wouldn't detect it unless the blockage overlapped with our expected return time. So worst case we could miss event loop blockage by loop_delay_monitor_period + loop_delay_monitor_threshold seconds (since we don't report unless we pass the threshold either).
But for Modal's purposes the most critical event loop blockage would be anything in the 10+ second magnitude (where heartbeats start to time out), so this should be fine (and we can adjust the period and threshold)
There was a problem hiding this comment.
If we want to be pedantic we could say “by up to” and give the upper bound but agree a second margin of error isn’t a huge deal.
synchronicity/synchronizer.py
Outdated
| self.loop_delay_monitor_period = 1.0 # seconds | ||
| self.loop_delay_monitor_threshold = 1.0 # seconds |
There was a problem hiding this comment.
Should these be public attributes?
There was a problem hiding this comment.
That's the idea, so that whatever library that sets up the synchronicity loop can adjust it. But now that I think about it, it's probably better to add them to the synchronizer constructor as optional arguments and keep them private, since changing the period after the event loop thread has started won't have any effect...
There was a problem hiding this comment.
Got it, yeah I saw you mutating them in the client so that clarified the intention, but agree that constructor params might make more sense.
Adds a simple background task that checks every second if there has been an event loop delay of more than a second.