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

Add type annotations to synapse.metrics #10847

Merged
merged 19 commits into from
Nov 17, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
17 changes: 10 additions & 7 deletions synapse/metrics/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@
Any,
Callable,
Dict,
Generic,
Iterable,
Optional,
Sequence,
Set,
Tuple,
Type,
TypeVar,
Union,
cast,
Expand Down Expand Up @@ -113,11 +115,12 @@ def _register(self) -> None:
all_gauges[self.name] = self


# Placeholder for `InFlightGauge._metrics_class`, which is created dynamically
_MetricsEntry = Any
# `MetricsEntry` only makes sense when it is a `Protocol`,
# but `Protocol` can't be used as a `TypeVar` bound.
MetricsEntry = TypeVar("MetricsEntry")


class InFlightGauge:
class InFlightGauge(Generic[MetricsEntry]):
reivilibre marked this conversation as resolved.
Show resolved Hide resolved
"""Tracks number of things (e.g. requests, Measure blocks, etc) in flight
at any given time.

Expand Down Expand Up @@ -147,13 +150,13 @@ def __init__(

# Create a class which have the sub_metrics values as attributes, which
# default to 0 on initialization. Used to pass to registered callbacks.
self._metrics_class = attr.make_class(
self._metrics_class: Type[MetricsEntry] = attr.make_class(
"_MetricsEntry", attrs={x: attr.ib(0) for x in sub_metrics}, slots=True
)

# Counts number of in flight blocks for a given set of label values
self._registrations: Dict[
Tuple[str, ...], Set[Callable[[_MetricsEntry], None]]
Tuple[str, ...], Set[Callable[[MetricsEntry], None]]
] = {}

# Protects access to _registrations
Expand All @@ -164,7 +167,7 @@ def __init__(
def register(
self,
key: Tuple[str, ...],
callback: Callable[[_MetricsEntry], None],
callback: Callable[[MetricsEntry], None],
) -> None:
"""Registers that we've entered a new block with labels `key`.

Expand All @@ -184,7 +187,7 @@ def register(
def unregister(
self,
key: Tuple[str, ...],
callback: Callable[[_MetricsEntry], None],
callback: Callable[[MetricsEntry], None],
) -> None:
"""Registers that we've exited a block with labels `key`."""

Expand Down
12 changes: 4 additions & 8 deletions synapse/metrics/_exposition.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import threading
from http.server import BaseHTTPRequestHandler, HTTPServer
from socketserver import ThreadingMixIn
from typing import Any, Dict, List, Type
from typing import Any, Dict, List, Type, Union
from urllib.parse import parse_qs, urlparse

from prometheus_client import REGISTRY, CollectorRegistry
Expand All @@ -39,15 +39,11 @@
CONTENT_TYPE_LATEST = "text/plain; version=0.0.4; charset=utf-8"


INF = float("inf")
MINUS_INF = float("-inf")


def floatToGoString(d: Any) -> str:
def floatToGoString(d: Union[int, float]) -> str:
reivilibre marked this conversation as resolved.
Show resolved Hide resolved
d = float(d)
if d == INF:
if d == math.inf:
return "+Inf"
elif d == MINUS_INF:
elif d == -math.inf:
return "-Inf"
elif math.isnan(d):
return "NaN"
Expand Down
9 changes: 6 additions & 3 deletions synapse/metrics/background_process_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ def run_as_background_process( # type: ignore[misc]
...
# The `type: ignore[misc]` above suppresses
# "error: Overloaded function signatures 1 and 2 overlap with incompatible return types [misc]"
#
# Overloads are used instead of a `Union` here because mypy fails to infer the type of `R`
# and complains about almost every call to this function when `func` is a `Union`.


@overload
Expand Down Expand Up @@ -287,19 +290,19 @@ async def run() -> Optional[R]:
F = TypeVar("F", bound=Callable[..., Any])


# NB: Return type is incorrect and should be a callable returning an F with a
# Deferred[Optional[R]] return, which we can't express correctly until Python 3.10.
def wrap_as_background_process(desc: str) -> Callable[[F], F]:
"""Decorator that wraps a function that gets called as a background
process.

Equivalent to calling the function with `run_as_background_process`.

Note that `run_as_background_process` changes the return type into
Note that the annotated return type of this function is incorrect.
The return type of the function, once wrapped, is actually a
`Deferred[Optional[T]]`.
"""

# NB: Return type is incorrect and should be F with a Deferred[Optional[R]] return
Copy link
Member

Choose a reason for hiding this comment

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

I think this should work, but maybe not? This makes me a bit nervous that we're not mentioning this returns a Deferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what to do about this. I updated the docstring above to call the change of return type out.

We can either annotate the return type correctly and lose the types of the arguments for the decorated function,
or annotate the return type incorrectly and preserve the types of the arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel kind of bad about lying but appreciate the dilemma. :/

I'm not really sure what's best, though. Is lying going to cause type errors for users of the function?

I would probably make the docstring louder about the return type being a lie, though.

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 tried to improve the docstring further

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine as - is.

question though, what happens if you bound F to return an Awaitable?

F = TypeVar("F", bound=Callable[..., Awaitable])

(or something similar?)

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine as - is.

to be clear: I'm saying I'm happy with the PR as it stands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

question though, what happens if you bound F to return an Awaitable?
F = TypeVar("F", bound=Callable[..., Awaitable])

Ooh. mypy throws up 3 errors which all look trivial to fix:

synapse/util/caches/expiringcache.py:86: error: Argument 2 to "run_as_background_process" has incompatible type "Callable[[], None]"; expected "Callable[..., Awaitable[Any]]"  [arg-type]
synapse/app/_base.py:404: error: Value of type variable "F" of function cannot be "Callable[[VarArg(Any), KwArg(Any)], None]"  [type-var]
synapse/handlers/typing.py:92: error: Value of type variable "F" of function cannot be "Callable[[FollowerTypingHandler], None]"  [type-var]

So it sounds like the cleanest thing to do is to restrict run_as_background_process to only accept Awaitable-returning functions and drop the whole maybe_awaitable thing. I'll make the change when I find some time.

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've gotten rid of the maybe_awaitable.

# We can't expression the return types correctly until Python 3.10.
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
def wrap_as_background_process_inner(func: F) -> F:
@wraps(func)
def wrap_as_background_process_inner_2(
Expand Down
11 changes: 9 additions & 2 deletions synapse/util/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from typing import Any, Callable, Optional, TypeVar, cast

from prometheus_client import Counter
from typing_extensions import Protocol

from synapse.logging.context import (
ContextResourceUsage,
Expand Down Expand Up @@ -53,8 +54,14 @@
"synapse_util_metrics_block_db_sched_duration_seconds", "", ["block_name"]
)


class InFlightBlockMetrics(Protocol):
real_time_max: float
real_time_sum: float


# Tracks the number of blocks currently active
in_flight = InFlightGauge(
in_flight = InFlightGauge[InFlightBlockMetrics](
"synapse_util_metrics_block_in_flight",
"",
labels=["block_name"],
Expand Down Expand Up @@ -168,7 +175,7 @@ def get_resource_usage(self) -> ContextResourceUsage:
"""
return self._logging_context.get_resource_usage()

def _update_in_flight(self, metrics):
def _update_in_flight(self, metrics: InFlightBlockMetrics):
"""Gets called when processing in flight metrics"""
duration = self.clock.time() - self.start

Expand Down