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

Fix the trace function for async functions. #7872

Merged
merged 6 commits into from
Jul 17, 2020
Merged

Fix the trace function for async functions. #7872

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
988caa0
Fix the trace function for async functions.
clokep Jul 16, 2020
9d623e8
Remove duplicate checks for if opentracing is enabled.
clokep Jul 16, 2020
c5c3ef4
Fix lint.
clokep Jul 16, 2020
f58a80e
Use a context manager.
clokep Jul 16, 2020
cb83512
Add back the runtime guards.
clokep Jul 16, 2020
b269f22
Clarify the changelog
clokep Jul 17, 2020
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/7872.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long standing bug where the tracing of async functions was broken.
clokep marked this conversation as resolved.
Show resolved Hide resolved
68 changes: 45 additions & 23 deletions synapse/logging/opentracing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,37 +733,59 @@ def decorator(func):

_opname = opname if opname else func.__name__

@wraps(func)
def _trace_inner(*args, **kwargs):
if opentracing is None:
return func(*args, **kwargs)
if inspect.iscoroutinefunction(func):
@wraps(func)
async def _trace_inner(*args, **kwargs):
if opentracing is None:
return await func(*args, **kwargs)
richvdh marked this conversation as resolved.
Show resolved Hide resolved

scope = start_active_span(_opname)
scope.__enter__()
scope = start_active_span(_opname)
scope.__enter__()
Copy link
Member

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)

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member Author

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


try:
result = func(*args, **kwargs)
if isinstance(result, defer.Deferred):
try:
result = await func(*args, **kwargs)
scope.__exit__(None, None, None)
return result

def call_back(result):
scope.__exit__(None, None, None)
return result
except Exception as e:
scope.span.set_tag(tags.ERROR, True)
scope.__exit__(type(e), None, e.__traceback__)
raise

def err_back(result):
scope.span.set_tag(tags.ERROR, True)
scope.__exit__(None, None, None)
return result
else:
# The other case here handles both sync functions and those
# decorated with inlineDeferred.
@wraps(func)
def _trace_inner(*args, **kwargs):
if opentracing is None:
return func(*args, **kwargs)

result.addCallbacks(call_back, err_back)
scope = start_active_span(_opname)
scope.__enter__()

else:
scope.__exit__(None, None, None)
try:
result = func(*args, **kwargs)
if isinstance(result, defer.Deferred):

def call_back(result):
scope.__exit__(None, None, None)
return result

def err_back(result):
scope.span.set_tag(tags.ERROR, True)
scope.__exit__(None, None, None)
return result

result.addCallbacks(call_back, err_back)

else:
scope.__exit__(None, None, None)

return result
return result

except Exception as e:
scope.__exit__(type(e), None, e.__traceback__)
raise
except Exception as e:
scope.__exit__(type(e), None, e.__traceback__)
raise

return _trace_inner

Expand Down