Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix incorrect juggling of logging contexts in _PerHostRatelimiter #13554

Merged
merged 9 commits into from
Aug 18, 2022
1 change: 1 addition & 0 deletions changelog.d/13554.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Instrument `FederationStateIdsServlet` (`/state_ids`) for understandable traces in Jaeger.
28 changes: 18 additions & 10 deletions synapse/util/ratelimitutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,19 @@ def ratelimit(self, host: str) -> "Iterator[defer.Deferred[None]]":

request_id = object()
ret = self._on_enter(request_id)
ret = defer.ensureDeferred(self._trace_wait(ret))
try:
yield ret
finally:
self._on_exit(request_id)

def _on_enter(self, request_id: object) -> "defer.Deferred[None]":
"""
Returns:
A `Deferred` that resolves once the request is allowed by the rate limiter.
The returned `Deferred` does not follow the Synapse logging context rules
and must be wrapped with `make_deferred_yieldable` before awaiting it.
"""
time_now = self.clock.time_msec()

# remove any entries from request_times which aren't within the window
Expand Down Expand Up @@ -222,20 +229,21 @@ def on_both(r: object) -> object:
# Ensure that we've properly cleaned up.
self.sleeping_requests.discard(request_id)
self.ready_request_queue.pop(request_id, None)
wait_span_scope.__exit__(None, None, None)
wait_timer_cm.__exit__(None, None, None)
return r

# Tracing
wait_span_scope = start_active_span("ratelimit wait")
wait_span_scope.__enter__()
# Metrics
wait_timer_cm = queue_wait_timer.time()
wait_timer_cm.__enter__()

ret_defer.addCallbacks(on_start, on_err)
ret_defer.addBoth(on_both)
return make_deferred_yieldable(ret_defer)
return ret_defer

async def _trace_wait(self, d: "defer.Deferred[None]") -> None:
"""Waits for a given `Deferred` to resolve, while timing it for metrics.

Args:
d: The `Deferred` to wait for. Must not follow the Synapse logging context
rules.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finding this slightly hard to wrap my head around. Rather than passing in a deferred, I would pass in the request id and have it call _on_enter. And do some renaming of things.

with start_active_span("ratelimit wait"), queue_wait_timer.time():
await make_deferred_yieldable(d)

def _on_exit(self, request_id: object) -> None:
logger.debug("Ratelimit(%s) [%s]: Processed req", self.host, id(request_id))
Expand Down