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

Fix the trace function for async functions.#7872

Merged
clokep merged 6 commits intodevelopfrom
clokep/async-tracing
Jul 17, 2020
Merged

Fix the trace function for async functions.#7872
clokep merged 6 commits intodevelopfrom
clokep/async-tracing

Conversation

@clokep
Copy link
Member

@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 force-pushed the clokep/async-tracing branch from 0ffd4d2 to 988caa0 Compare July 16, 2020 14:48
clokep
clokep commented Jul 16, 2020
View reviewed changes
@clokep clokep requested a review from a team July 16, 2020 14:52
richvdh
richvdh suggested changes Jul 16, 2020
View reviewed changes
synapse/logging/opentracing.py Outdated
scope = start_active_span(_opname)
scope.__enter__()
scope = start_active_span(_opname)
scope.__enter__()
Copy link
Member

@richvdh richvdh Jul 16, 2020

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

@clokep clokep Jul 16, 2020

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

@clokep clokep Jul 16, 2020

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

@clokep clokep Jul 16, 2020

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

@clokep clokep requested review from richvdh and removed request for richvdh July 16, 2020 15:14
@clokep
Copy link
Member 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
richvdh
richvdh reviewed Jul 17, 2020
View reviewed changes
richvdh
richvdh approved these changes Jul 17, 2020
View reviewed changes
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

@clokep @richvdh
Clarify the changelog
b269f22 
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
@erikjohnston erikjohnston mentioned this pull request Jul 27, 2020
Merged
@clokep clokep mentioned this pull request Jul 30, 2020
Closed
48 tasks
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit 'de119063f' into anoa/dinsic_release_1_18_x
a567e76 
* 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.

Reviewers

1 more reviewer

@richvdh richvdh richvdh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@clokep @richvdh