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

Type hints for the remaining two files in synapse.http. #11164

Merged
merged 9 commits into from
Oct 28, 2021
Merged

Type hints for the remaining two files in synapse.http. #11164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7f9fe6c
Teach MyPy that the sentinel context is False
Oct 22, 2021
48cf46c
Annotate RequestMetrics
Oct 22, 2021
df7bdbc
Make mypy pass for synapse.http.request_metrics
Oct 22, 2021
f5287d9
Make synapse.http.connectproxyclient pass mypy
Oct 22, 2021
2ae227e
synapse.http now passes mypy
Oct 22, 2021
4634368
Changelog
Oct 22, 2021
f362cd6
typo fix
Oct 22, 2021
8ee9d35
Try to express that counts are int, not float
Oct 22, 2021
af428f7
Fix sytest. Not sure about this.
Oct 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11164.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add type hints so that `synapse.http` passes `mypy` checks.
12 changes: 2 additions & 10 deletions mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ no_implicit_optional = True

files =
scripts-dev/sign_json,
synapse/__init__.py,
synapse/api,
synapse/appservice,
synapse/config,
Expand All @@ -31,16 +32,7 @@ files =
synapse/federation,
synapse/groups,
synapse/handlers,
synapse/http/additional_resource.py,
synapse/http/client.py,
synapse/http/federation/matrix_federation_agent.py,
synapse/http/federation/srv_resolver.py,
synapse/http/federation/well_known_resolver.py,
synapse/http/matrixfederationclient.py,
synapse/http/proxyagent.py,
synapse/http/servlet.py,
synapse/http/server.py,
synapse/http/site.py,
synapse/http,
synapse/logging,
synapse/metrics,
synapse/module_api,
Expand Down
12 changes: 9 additions & 3 deletions synapse/http/connectproxyclient.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@ def __init__(
def __repr__(self):
return "<HTTPConnectProxyEndpoint %s>" % (self._proxy_endpoint,)

def connect(self, protocolFactory: ClientFactory):
# Mypy encounters a false positive here: it complains that ClientFactory
# is incompatible with IProtocolFactory. But ClientFactory inherits from
# Factory, which implements IProtocolFactory. So I think this is a bug
# in mypy-zope.
def connect(self, protocolFactory: ClientFactory): # type: ignore[override]
f = HTTPProxiedClientFactory(
self._host, self._port, protocolFactory, self._proxy_creds
)
Expand Down Expand Up @@ -119,13 +123,15 @@ def __init__(
self.dst_port = dst_port
self.wrapped_factory = wrapped_factory
self.proxy_creds = proxy_creds
self.on_connection = defer.Deferred()
self.on_connection: "defer.Deferred[None]" = defer.Deferred()

def startedConnecting(self, connector):
return self.wrapped_factory.startedConnecting(connector)

def buildProtocol(self, addr):
wrapped_protocol = self.wrapped_factory.buildProtocol(addr)
if wrapped_protocol is None:
raise TypeError("buildProtocol produced None instead of a Protocol")

return HTTPConnectProtocol(
self.dst_host,
Expand Down Expand Up @@ -235,7 +241,7 @@ def __init__(
self.host = host
self.port = port
self.proxy_creds = proxy_creds
self.on_connected = defer.Deferred()
self.on_connected: "defer.Deferred[None]" = defer.Deferred()

def connectionMade(self):
logger.debug("Connected to proxy, sending CONNECT")
Expand Down
50 changes: 33 additions & 17 deletions synapse/http/request_metrics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

import logging
import threading
import traceback
from typing import Dict, Mapping, Set, Tuple

from prometheus_client.core import Counter, Histogram

Expand Down Expand Up @@ -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() -> Mapping[Tuple[str, ...], int]:
"""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:
Expand All @@ -127,8 +124,9 @@ 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 leave the length unspecified
# for compatability with LaterGauge's annotations.
counts: Dict[Tuple[str, ...], int] = {}
for rm in reqs:
key = (rm.method, rm.name)
counts[key] = counts.get(key, 0) + 1
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

reckon it's worth start_ts_sec whilst you're here? We for some reason love milliseconds as a time unit in Synapse and usually try to mark the ones that are in seconds so that it's obvious...

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Class.start(instance, ...), anyway)

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)
Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

Hesitant to change this to error.
It's certainly bad but it's not the server admin's fault. Unless it's because Sentry will only pick up on errors? I guess that makes sense and that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

Expand All @@ -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()
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions synapse/logging/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ def __init__(self) -> None:
self.scope = None
self.tag = None

def __str__(self):
def __str__(self) -> str:
return "sentinel"

def copy_to(self, record):
Expand All @@ -241,7 +241,7 @@ def add_database_scheduled(self, sched_sec):
def record_event_fetch(self, event_count):
pass

def __bool__(self):
def __bool__(self) -> Literal[False]:
return False


Expand Down
14 changes: 9 additions & 5 deletions synapse/metrics/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import platform
import threading
import time
from typing import Callable, Dict, Iterable, Optional, Tuple, Union
from typing import Callable, Dict, Iterable, Mapping, Optional, Tuple, Union

import attr
from prometheus_client import Counter, Gauge, Histogram
Expand Down Expand Up @@ -67,7 +67,11 @@ class LaterGauge:
labels = attr.ib(hash=False, type=Optional[Iterable[str]])
# callback: should either return a value (if there are no labels for this metric),
# or dict mapping from a label tuple to a value
caller = attr.ib(type=Callable[[], Union[Dict[Tuple[str, ...], float], float]])
caller = attr.ib(
type=Callable[
[], Union[Mapping[Tuple[str, ...], Union[int, float]], Union[int, float]]
]
)

def collect(self):

Expand All @@ -80,11 +84,11 @@ def collect(self):
yield g
return

if isinstance(calls, dict):
if isinstance(calls, (int, float)):
Copy link
Contributor

Choose a reason for hiding this comment

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

oh grumble, so int is a subtype of float except at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is a subtype of and is a subclass of are different relations. The latter implies the former, but the converse doesn't hold.

Consider a class P(Protocol): .... I might implement it twice and make two types A, B, both subtypes of P. If I implement them independently, then neither A nor B are subclasses of the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completeness:

$ python
Python 3.9.7 (default, Aug 30 2021, 00:00:00) 
[GCC 11.2.1 20210728 (Red Hat 11.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> issubclass(int, float)
False
>>> issubclass(float, int)
False

g.add_metric([], calls)
else:
for k, v in calls.items():
g.add_metric(k, v)
else:
g.add_metric([], calls)

yield g

Expand Down