From dfa70adc33b8f058acae8a4f4f61a83f9fab47a7 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Mon, 21 May 2018 16:03:39 +0100 Subject: [PATCH 1/3] Add in flight request metrics This tracks CPU and DB usage while requests are in flight, rather than when we write the response. --- synapse/http/request_metrics.py | 152 +++++++++++++++++++++++++++++++- synapse/http/site.py | 4 +- 2 files changed, 154 insertions(+), 2 deletions(-) diff --git a/synapse/http/request_metrics.py b/synapse/http/request_metrics.py index 8c850bf23..f1e9b41fd 100644 --- a/synapse/http/request_metrics.py +++ b/synapse/http/request_metrics.py @@ -98,14 +98,76 @@ response_size = metrics.register_counter( "response_size", labels=["method", "servlet", "tag"] ) +# In flight metrics are incremented while the requests are in flight, rather +# than when the response was written. + +in_flight_requests_ru_utime = metrics.register_counter( + "in_flight_requests_ru_utime_seconds", labels=["method", "servlet"], +) + +in_flight_requests_ru_stime = metrics.register_counter( + "in_flight_requests_ru_stime_seconds", labels=["method", "servlet"], +) + +in_flight_requests_db_txn_count = metrics.register_counter( + "in_flight_requests_db_txn_count", labels=["method", "servlet"], +) + +# seconds spent waiting for db txns, excluding scheduling time, when processing +# this request +in_flight_requests_db_txn_duration = metrics.register_counter( + "in_flight_requests_db_txn_duration_seconds", labels=["method", "servlet"], +) + +# seconds spent waiting for a db connection, when processing this request +in_flight_requests_db_sched_duration = metrics.register_counter( + "in_flight_requests_db_sched_duration_seconds", labels=["method", "servlet"] +) + +_in_flight_requests_count = metrics.register_gauge( + "in_flight_requests_count", labels=["method", "servlet"] +) + + +# The set of all in flight requests. +_in_flight_requests = set() + + +def _collect_in_flight(): + """Called just before metrics are collected, so we use it to update all + the in flight request metrics + """ + + # Map from (method, name) -> int, the number of in flight requests of that + # type + counts = {} + + for rm in _in_flight_requests: + rm.update_metrics() + key = (rm.method, rm.name,) + counts[key] = counts.get(key, 0) + 1 + + for (method, name), count in counts.iteritems(): + _in_flight_requests_count.set(count, method, name) + + +metrics.register_collector(_collect_in_flight) + class RequestMetrics(object): - def start(self, time_msec, name): + def start(self, time_msec, name, method): self.start = time_msec self.start_context = LoggingContext.current_context() self.name = name + self.method = method + + self._request_stats = _RequestStats.from_context(self.start_context) + + _in_flight_requests.add(self) def stop(self, time_msec, request): + _in_flight_requests.discard(self) + context = LoggingContext.current_context() tag = "" @@ -147,3 +209,91 @@ class RequestMetrics(object): ) response_size.inc_by(request.sentLength, request.method, self.name, tag) + + # We always call this at the end to ensure that we update the metrics + # regardless of whether a call to /metrics while the request was in + # flight. + self.update_metrics() + + def update_metrics(self): + """Updates the in flight metrics with values from this request. + """ + + diff = self._request_stats.update() + + in_flight_requests_ru_utime.inc_by( + diff.ru_utime, self.method, self.name, + ) + + in_flight_requests_ru_stime.inc_by( + diff.ru_stime, self.method, self.name, + ) + + in_flight_requests_db_txn_count.inc_by( + diff.db_txn_count, self.method, self.name, + ) + + in_flight_requests_db_txn_duration.inc_by( + diff.db_txn_duration_ms / 1000., self.method, self.name, + ) + + in_flight_requests_db_sched_duration.inc_by( + diff.db_sched_duration_ms / 1000., self.method, self.name, + ) + + +class _RequestStats(object): + """Keeps tracks of various metrics for an in flight request. + """ + + __slots__ = [ + "context", "ru_utime", "ru_stime", + "db_txn_count", "db_txn_duration_ms", "db_sched_duration_ms", + ] + + def __init__(self, context, ru_utime, ru_stime, db_txn_count, + db_txn_duration_ms, db_sched_duration_ms): + self.context = context + self.ru_utime = ru_utime + self.ru_stime = ru_stime + self.db_txn_count = db_txn_count + self.db_txn_duration_ms = db_txn_duration_ms + self.db_sched_duration_ms = db_sched_duration_ms + + @staticmethod + def from_context(context): + ru_utime, ru_stime = context.get_resource_usage() + + return _RequestStats( + context, + ru_utime, ru_stime, + context.db_txn_count, + context.db_txn_duration_ms, + context.db_sched_duration_ms, + ) + + def update(self): + """Updates the current values and returns the difference between the + old and new values. + + Returns: + _RequestStats: The difference between the old and new values + """ + new = _RequestStats.from_context(self.context) + + diff = _RequestStats( + self.context, + new.ru_utime - self.ru_utime, + new.ru_stime - self.ru_stime, + new.db_txn_count - self.db_txn_count, + new.db_txn_duration_ms - self.db_txn_duration_ms, + new.db_sched_duration_ms - self.db_sched_duration_ms, + ) + + self.ru_utime = new.ru_utime + self.ru_stime = new.ru_stime + self.db_txn_count = new.db_txn_count + self.db_txn_duration_ms = new.db_txn_duration_ms + self.db_sched_duration_ms = new.db_sched_duration_ms + + return diff diff --git a/synapse/http/site.py b/synapse/http/site.py index 202a99050..b60850422 100644 --- a/synapse/http/site.py +++ b/synapse/http/site.py @@ -85,7 +85,9 @@ class SynapseRequest(Request): def _started_processing(self, servlet_name): self.start_time = int(time.time() * 1000) self.request_metrics = RequestMetrics() - self.request_metrics.start(self.start_time, name=servlet_name) + self.request_metrics.start( + self.start_time, name=servlet_name, method=self.method, + ) self.site.access_logger.info( "%s - %s - Received request: %s %s", From fb2806b1868baaa24d0d6e21d10f2c1f6177717e Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 22 May 2018 09:31:53 +0100 Subject: [PATCH 2/3] Move in_flight_requests_count to be a callback metric --- synapse/http/request_metrics.py | 39 +++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/synapse/http/request_metrics.py b/synapse/http/request_metrics.py index f1e9b41fd..8cb23f4d6 100644 --- a/synapse/http/request_metrics.py +++ b/synapse/http/request_metrics.py @@ -124,12 +124,8 @@ in_flight_requests_db_sched_duration = metrics.register_counter( "in_flight_requests_db_sched_duration_seconds", labels=["method", "servlet"] ) -_in_flight_requests_count = metrics.register_gauge( - "in_flight_requests_count", labels=["method", "servlet"] -) - -# The set of all in flight requests. +# The set of all in flight requests, set[RequestMetrics] _in_flight_requests = set() @@ -138,22 +134,37 @@ def _collect_in_flight(): the in flight request metrics """ - # Map from (method, name) -> int, the number of in flight requests of that - # type - counts = {} - for rm in _in_flight_requests: rm.update_metrics() - key = (rm.method, rm.name,) - counts[key] = counts.get(key, 0) + 1 - - for (method, name), count in counts.iteritems(): - _in_flight_requests_count.set(count, method, name) metrics.register_collector(_collect_in_flight) +def _get_in_flight_counts(): + """Returns a count of all in flight requests by (method, server_name) + + Returns: + dict[tuple[str, str], int] + """ + + # Map from (method, name) -> int, the number of in flight requests of that + # type + counts = {} + for rm in _in_flight_requests: + key = (rm.method, rm.name,) + counts[key] = counts.get(key, 0) + 1 + + return counts + + +metrics.register_callback( + "in_flight_requests_count", + _get_in_flight_counts, + labels=["method", "servlet"] +) + + class RequestMetrics(object): def start(self, time_msec, name, method): self.start = time_msec From c435b0b44172efe485ae1401deb8adca2e774d3c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 22 May 2018 09:34:26 +0100 Subject: [PATCH 3/3] Don't store context --- synapse/http/request_metrics.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/synapse/http/request_metrics.py b/synapse/http/request_metrics.py index 8cb23f4d6..5917c8395 100644 --- a/synapse/http/request_metrics.py +++ b/synapse/http/request_metrics.py @@ -230,7 +230,7 @@ class RequestMetrics(object): """Updates the in flight metrics with values from this request. """ - diff = self._request_stats.update() + diff = self._request_stats.update(self.start_context) in_flight_requests_ru_utime.inc_by( diff.ru_utime, self.method, self.name, @@ -258,13 +258,12 @@ class _RequestStats(object): """ __slots__ = [ - "context", "ru_utime", "ru_stime", + "ru_utime", "ru_stime", "db_txn_count", "db_txn_duration_ms", "db_sched_duration_ms", ] - def __init__(self, context, ru_utime, ru_stime, db_txn_count, + def __init__(self, ru_utime, ru_stime, db_txn_count, db_txn_duration_ms, db_sched_duration_ms): - self.context = context self.ru_utime = ru_utime self.ru_stime = ru_stime self.db_txn_count = db_txn_count @@ -276,24 +275,22 @@ class _RequestStats(object): ru_utime, ru_stime = context.get_resource_usage() return _RequestStats( - context, ru_utime, ru_stime, context.db_txn_count, context.db_txn_duration_ms, context.db_sched_duration_ms, ) - def update(self): + def update(self, context): """Updates the current values and returns the difference between the old and new values. Returns: _RequestStats: The difference between the old and new values """ - new = _RequestStats.from_context(self.context) + new = _RequestStats.from_context(context) diff = _RequestStats( - self.context, new.ru_utime - self.ru_utime, new.ru_stime - self.ru_stime, new.db_txn_count - self.db_txn_count,