-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix the trace function for async functions. #7872
Conversation
0ffd4d2
to
988caa0
Compare
synapse/logging/opentracing.py
Outdated
scope = start_active_span(_opname) | ||
scope.__enter__() | ||
scope = start_active_span(_opname) | ||
scope.__enter__() |
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.
You've made this harder than it needs to be :).
No need to call __enter__
/__exit__
manually. Just:
with scope:
return await func(*args, **kwargs)
possibly with an except
clause just to set_tag(tags.ERROR)
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.
Yeah, I was copying and pasting from below. I can update both to actually use a context manager instead.
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.
Ah, I see the Deferred
case can't use the context manager easily, that's why it was that way. 👍
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.
We could always assume that the non-coroutine case was an inlineDeferred, this seems to be what measure_func
does:
synapse/synapse/util/metrics.py
Lines 64 to 87 in 39230d2
def measure_func(name=None): | |
def wrapper(func): | |
block_name = func.__name__ if name is None else name | |
if inspect.iscoroutinefunction(func): | |
@wraps(func) | |
async def measured_func(self, *args, **kwargs): | |
with Measure(self.clock, block_name): | |
r = await func(self, *args, **kwargs) | |
return r | |
else: | |
@wraps(func) | |
@defer.inlineCallbacks | |
def measured_func(self, *args, **kwargs): | |
with Measure(self.clock, block_name): | |
r = yield func(self, *args, **kwargs) | |
return r | |
return measured_func | |
return wrapper |
@richvdh I had to add back to |
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.
lgtm
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
* commit 'de119063f': (31 commits) Convert room list handler to async/await. (#7912) Element CSS and logo in email templates (#7919) Lint the contrib/ directory in CI and linting scripts, add synctl to linting script (#7914) Remove unused code from synapse.logging.utils. (#7897) Fix a typo in the sample config. (#7890) Fix deprecation warning: import ABC from collections.abc (#7892) Change sample config's postgres user to synapse_user (#7889) Fix deprecation warning due to invalid escape sequences (#7895) Remove Ubuntu Eoan that is now EOL (#7888) Fix the trace function for async functions. (#7872) Add help for creating a user via docker (#7885) Switch to Debian:Slim from Alpine for the docker image (#7839) Stop using 'device_max_stream_id' (#7882) Fix TypeError in synapse.notifier (#7880) Add a default limit (of 100) to get/sync operations. (#7858) Change "unknown room ver" logging to warning. (#7881) Convert device handler to async/await (#7871) Convert synapse.app to async/await. (#7868) Convert _base, profile, and _receipts handlers to async/await (#7860) Add admin endpoint to get members in a room. (#7842) ...
I think this is the proper fix for the
@trace
decorator with async functions, but I'm not 100% sure.I think there might be an issue open for some things missing from tracing, which could be this?
The diff should be better without whitespace changes.