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

Add type annotations to trace decorator #13328

Merged
merged 11 commits into from
Jul 19, 2022
Merged

Add type annotations to trace decorator #13328

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c6914ee
Split trace into two functions.
clokep Jul 19, 2022
893cb52
Annotate trace and trace_with_opname.
clokep Jul 19, 2022
52318fb
Federation timeout is an Optional[int].
clokep Jul 19, 2022
fd0d962
The 'version' query parameter is required.
clokep Jul 19, 2022
245eb11
Generalize return type.
clokep Jul 19, 2022
725b44a
Fix simple end_to_end_keys errors.
clokep Jul 19, 2022
5e99e74
(Mostly) fix get_e2e_device_keys_and_signatures signature.
clokep Jul 19, 2022
244c719
Fix remaining errors.
clokep Jul 19, 2022
99f35bb
Newsfragment
clokep Jul 19, 2022
65a7218
Fix invariance.
clokep Jul 19, 2022
cde9700
Fix Literal import.
clokep Jul 19, 2022
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
50 changes: 28 additions & 22 deletions synapse/logging/opentracing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,14 +84,13 @@ def interesting_function(*args, **kwargs):
return something_usual_and_useful


Operation names can be explicitly set for a function by passing the
operation name to ``trace``
Operation names can be explicitly set for a function by using ``trace_with_opname``:

.. code-block:: python

from synapse.logging.opentracing import trace
from synapse.logging.opentracing import trace_with_opname

@trace(opname="a_better_operation_name")
@trace_with_opname("a_better_operation_name")
def interesting_badly_named_function(*args, **kwargs):
# Does all kinds of cool and expected things
return something_usual_and_useful
Expand Down Expand Up @@ -798,33 +797,31 @@ def extract_text_map(carrier: Dict[str, str]) -> Optional["opentracing.SpanConte
# Tracing decorators


def trace(func=None, opname: Optional[str] = None):
def trace_with_opname(opname: str) -> Callable[[Callable[P, R]], Callable[P, R]]:
"""
Decorator to trace a function.
Sets the operation name to that of the function's or that given
as operation_name. See the module's doc string for usage
examples.
Decorator to trace a function with a custom opname.

See the module's doc string for usage examples.

"""

def decorator(func):
def decorator(func: Callable[P, R]) -> Callable[P, R]:
if opentracing is None:
return func # type: ignore[unreachable]

_opname = opname if opname else func.__name__

if inspect.iscoroutinefunction(func):

@wraps(func)
async def _trace_inner(*args, **kwargs):
with start_active_span(_opname):
return await func(*args, **kwargs)
async def _trace_inner(*args: P.args, **kwargs: P.kwargs) -> R:
with start_active_span(opname):
return await func(*args, **kwargs) # type: ignore[misc]
Copy link
Member Author

@clokep clokep Jul 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For completeness this is ignoring:

error: Incompatible types in "await" (actual type "R", expected type "Awaitable[Any]")  [misc]

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I think this is fine: within this branch, func is a coroutine so should return an awaitable. It's a shame mypy can't deduce this for itself, given the use of TypeGuard in typeshed.

Oh, but that bit of typeshed I quoted only just landed this morning! So we might find that this ignore is unnecessary in a future mypy release.


else:
# The other case here handles both sync functions and those
# decorated with inlineDeferred.
@wraps(func)
def _trace_inner(*args, **kwargs):
scope = start_active_span(_opname)
def _trace_inner(*args: P.args, **kwargs: P.kwargs) -> R:
scope = start_active_span(opname)
scope.__enter__()

try:
Expand Down Expand Up @@ -858,12 +855,21 @@ def err_back(result: R) -> R:
scope.__exit__(type(e), None, e.__traceback__)
raise

return _trace_inner
return _trace_inner # type: ignore[return-value]
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the error here, out of interest?

Copy link
Member Author

Choose a reason for hiding this comment

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

synapse/logging/opentracing.py:858: error: Incompatible return value type (got "Callable[P, Coroutine[Any, Any, R]]", expected "Callable[P, R]")  [return-value]

Actually now that I split this up I might be able to do simple overloads with returning Awaitable[R] in a couple situations and remove this ignore?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that's possible actually. Any idea if we should do something about this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

As sad as I am to leave a typing problem lying around like this, I think I'd probably leave the ignore as is. The important thing is that we accurately annotate the decorator itself; its internals don't have an effect on the rest of the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

its internals don't have an effect on the rest of the function.

This was kind of my conclusion after messing with it for a bit.


if func:
return decorator(func)
else:
return decorator
return decorator


def trace(func: Callable[P, R]) -> Callable[P, R]:
"""
Decorator to trace a function.

Sets the operation name to that of the function's name.

See the module's doc string for usage examples.
"""

return trace_with_opname(func.__name__)(func)


def tag_args(func: Callable[P, R]) -> Callable[P, R]:
Expand Down
4 changes: 2 additions & 2 deletions synapse/replication/http/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from synapse.http.server import HttpServer, is_method_cancellable
from synapse.http.site import SynapseRequest
from synapse.logging import opentracing
from synapse.logging.opentracing import trace
from synapse.logging.opentracing import trace_with_opname
from synapse.types import JsonDict
from synapse.util.caches.response_cache import ResponseCache
from synapse.util.stringutils import random_string
Expand Down Expand Up @@ -196,7 +196,7 @@ def make_client(cls, hs: "HomeServer") -> Callable:
"ascii"
)

@trace(opname="outgoing_replication_request")
@trace_with_opname("outgoing_replication_request")
async def send_request(*, instance_name: str = "master", **kwargs: Any) -> Any:
with outgoing_gauge.track_inprogress():
if instance_name == local_instance_name:
Expand Down
4 changes: 2 additions & 2 deletions synapse/rest/client/keys.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
parse_string,
)
from synapse.http.site import SynapseRequest
from synapse.logging.opentracing import log_kv, set_tag, trace
from synapse.logging.opentracing import log_kv, set_tag, trace_with_opname
from synapse.types import JsonDict, StreamToken

from ._base import client_patterns, interactive_auth_handler
Expand Down Expand Up @@ -71,7 +71,7 @@ def __init__(self, hs: "HomeServer"):
self.e2e_keys_handler = hs.get_e2e_keys_handler()
self.device_handler = hs.get_device_handler()

@trace(opname="upload_keys")
@trace_with_opname("upload_keys")
async def on_POST(
self, request: SynapseRequest, device_id: Optional[str]
) -> Tuple[int, JsonDict]:
Expand Down
4 changes: 2 additions & 2 deletions synapse/rest/client/sendtodevice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from synapse.http.server import HttpServer
from synapse.http.servlet import assert_params_in_dict, parse_json_object_from_request
from synapse.http.site import SynapseRequest
from synapse.logging.opentracing import set_tag, trace
from synapse.logging.opentracing import set_tag, trace_with_opname
from synapse.rest.client.transactions import HttpTransactionCache
from synapse.types import JsonDict

Expand All @@ -43,7 +43,7 @@ def __init__(self, hs: "HomeServer"):
self.txns = HttpTransactionCache(hs)
self.device_message_handler = hs.get_device_message_handler()

@trace(opname="sendToDevice")
@trace_with_opname("sendToDevice")
def on_PUT(
self, request: SynapseRequest, message_type: str, txn_id: str
) -> Awaitable[Tuple[int, JsonDict]]:
Expand Down
12 changes: 6 additions & 6 deletions synapse/rest/client/sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
from synapse.http.server import HttpServer
from synapse.http.servlet import RestServlet, parse_boolean, parse_integer, parse_string
from synapse.http.site import SynapseRequest
from synapse.logging.opentracing import trace
from synapse.logging.opentracing import trace_with_opname
from synapse.types import JsonDict, StreamToken
from synapse.util import json_decoder

Expand Down Expand Up @@ -210,7 +210,7 @@ async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
logger.debug("Event formatting complete")
return 200, response_content

@trace(opname="sync.encode_response")
@trace_with_opname("sync.encode_response")
async def encode_response(
self,
time_now: int,
Expand Down Expand Up @@ -315,7 +315,7 @@ def encode_presence(events: List[UserPresenceState], time_now: int) -> JsonDict:
]
}

@trace(opname="sync.encode_joined")
@trace_with_opname("sync.encode_joined")
async def encode_joined(
self,
rooms: List[JoinedSyncResult],
Expand All @@ -340,7 +340,7 @@ async def encode_joined(

return joined

@trace(opname="sync.encode_invited")
@trace_with_opname("sync.encode_invited")
async def encode_invited(
self,
rooms: List[InvitedSyncResult],
Expand Down Expand Up @@ -371,7 +371,7 @@ async def encode_invited(

return invited

@trace(opname="sync.encode_knocked")
@trace_with_opname("sync.encode_knocked")
async def encode_knocked(
self,
rooms: List[KnockedSyncResult],
Expand Down Expand Up @@ -420,7 +420,7 @@ async def encode_knocked(

return knocked

@trace(opname="sync.encode_archived")
@trace_with_opname("sync.encode_archived")
async def encode_archived(
self,
rooms: List[ArchivedSyncResult],
Expand Down