-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Type hints for the remaining two files in synapse.http
.
#11164
Changes from 6 commits
7f9fe6c
48cf46c
df7bdbc
f5287d9
2ae227e
4634368
f362cd6
8ee9d35
af428f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add type hints so that `synapse.http` passes `mypy` checks. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,8 @@ | |
|
||
import logging | ||
import threading | ||
import traceback | ||
from typing import Dict, Set, Tuple | ||
|
||
from prometheus_client.core import Counter, Histogram | ||
|
||
|
@@ -105,19 +107,14 @@ | |
["method", "servlet"], | ||
) | ||
|
||
# The set of all in flight requests, set[RequestMetrics] | ||
_in_flight_requests = set() | ||
_in_flight_requests: Set["RequestMetrics"] = set() | ||
|
||
# Protects the _in_flight_requests set from concurrent access | ||
_in_flight_requests_lock = threading.Lock() | ||
|
||
|
||
def _get_in_flight_counts(): | ||
"""Returns a count of all in flight requests by (method, server_name) | ||
|
||
Returns: | ||
dict[tuple[str, str], int] | ||
""" | ||
def _get_in_flight_counts() -> Dict[Tuple[str, ...], float]: | ||
"""Returns a count of all in flight requests by (method, server_name)""" | ||
# Cast to a list to prevent it changing while the Prometheus | ||
# thread is collecting metrics | ||
with _in_flight_requests_lock: | ||
|
@@ -127,11 +124,12 @@ def _get_in_flight_counts(): | |
rm.update_metrics() | ||
|
||
# Map from (method, name) -> int, the number of in flight requests of that | ||
# type | ||
counts = {} | ||
# type. The key type is Tuple[str, str], but we leavethe length unspecified | ||
DMRobertson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# for compatability with LaterGauge's annotations. | ||
counts: Dict[Tuple[str, ...], float] = {} | ||
for rm in reqs: | ||
key = (rm.method, rm.name) | ||
counts[key] = counts.get(key, 0) + 1 | ||
counts[key] = counts.get(key, 0.0) + 1.0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW I don't think you need to have changed this line because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this may have been a desperate attempt to solve the type mismatch (the one that (I resisted the temptation to just use a collections.Counter here) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't work OOTB because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Had a go at this in 8ee9d35. What do you reckon? |
||
|
||
return counts | ||
|
||
|
@@ -145,15 +143,21 @@ def _get_in_flight_counts(): | |
|
||
|
||
class RequestMetrics: | ||
def start(self, time_sec, name, method): | ||
self.start = time_sec | ||
def start(self, time_sec: float, name: str, method: str) -> None: | ||
self.start_ts = time_sec | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reckon it's worth There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh bahaha, it was a name clash with the method? That's pretty funny, but it's single-use presumably so you never normally notice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. If you call start() you'll be unable to call it a second time on that instance. (Without doing |
||
self.start_context = current_context() | ||
self.name = name | ||
self.method = method | ||
|
||
# _request_stats records resource usage that we have already added | ||
# to the "in flight" metrics. | ||
self._request_stats = self.start_context.get_resource_usage() | ||
if self.start_context: | ||
# _request_stats records resource usage that we have already added | ||
# to the "in flight" metrics. | ||
self._request_stats = self.start_context.get_resource_usage() | ||
else: | ||
logger.error( | ||
"Tried to start a RequestMetric from the sentinel context.\n%s", | ||
"".join(traceback.format_stack()), | ||
) | ||
|
||
with _in_flight_requests_lock: | ||
_in_flight_requests.add(self) | ||
|
@@ -169,12 +173,18 @@ def stop(self, time_sec, response_code, sent_bytes): | |
tag = context.tag | ||
|
||
if context != self.start_context: | ||
logger.warning( | ||
logger.error( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hesitant to change this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, @squahtx's advice was to get these in Sentry and then we can fix them up. |
||
"Context have unexpectedly changed %r, %r", | ||
context, | ||
self.start_context, | ||
) | ||
return | ||
else: | ||
logger.error( | ||
"Trying to stop RequestMetrics in the sentinel context.\n%s", | ||
"".join(traceback.format_stack()), | ||
) | ||
return | ||
|
||
response_code = str(response_code) | ||
|
||
|
@@ -183,7 +193,7 @@ def stop(self, time_sec, response_code, sent_bytes): | |
response_count.labels(self.method, self.name, tag).inc() | ||
|
||
response_timer.labels(self.method, self.name, tag, response_code).observe( | ||
time_sec - self.start | ||
time_sec - self.start_ts | ||
) | ||
|
||
resource_usage = context.get_resource_usage() | ||
|
@@ -213,6 +223,12 @@ def stop(self, time_sec, response_code, sent_bytes): | |
|
||
def update_metrics(self): | ||
"""Updates the in flight metrics with values from this request.""" | ||
if not self.start_context: | ||
logger.error( | ||
"Tried to update a RequestMetric from the sentinel context.\n%s", | ||
"".join(traceback.format_stack()), | ||
) | ||
return | ||
new_stats = self.start_context.get_resource_usage() | ||
|
||
diff = new_stats - self._request_stats | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really really really want to say:
But I guess it's OK (I can see why you had to do this) and it's only the metrics.