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

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Jul 16, 2020

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.

@clokep clokep requested a review from a team July 16, 2020 14:52
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
Contributor 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
Contributor 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
Contributor 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:

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

@clokep clokep requested review from richvdh and removed request for richvdh July 16, 2020 15:14
@clokep
Copy link
Contributor Author

clokep commented Jul 16, 2020

@richvdh I had to add back to opentracing checks, see #7872 (comment)

@clokep clokep requested a review from richvdh July 16, 2020 16:01
changelog.d/7872.bugfix Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a 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>
@clokep clokep merged commit d1d5fa6 into develop Jul 17, 2020
@clokep clokep deleted the clokep/async-tracing branch July 17, 2020 17:32
@clokep clokep mentioned this pull request Jul 30, 2020
48 tasks
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* 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)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants