Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(crons): Make monitor async friendly #2912

Merged
merged 12 commits into from
Mar 28, 2024

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Mar 27, 2024

Our monitor decorator/context manager is not async friendly. If you wrap an async function in it, monitor will only wrap the creation of the coroutine, rather than its execution, which means it'll exit early. It won't capture the actual duration of the execution of the function, and the cron run will always be reported as success.

To better illustrate, try this out:

import asyncio
import sentry_sdk

sentry_sdk.init(...)

@sentry_sdk.monitor(monitor_slug='my-slug')
async def my_cron():
    print('running...')
    await asyncio.sleep(1)
    raise ValueError

asyncio.run(my_cron())

If you execute this, you'll see multiple fishy things:

  • the cron will be green even though it throws an exception
  • even if it takes >= 1s to execute, the time reported will be ~0ms
  • the "running..." message will be printed after the final checkin (you can see this with debug=True)

To fix this, we need special handling for the async case where we actually execute the coroutine and wait until it's finished before reporting.

Implementation Notes

This is annoying to implement cleanly because of Python 2 compatibility. Python 2 will throw SyntaxErrors when it encounters any async defs. We could use an exec for the async part, but this is ugly.

Instead, I'm implementing the Python2+3-friendly part of the decorator in the main monitor class and outsourcing the problematic __call__ definition to version-specific mixins, where the Python 3-only mixin will never get imported on Python 2.

In any case, we can get rid of this compat hack with SDK 2.0.

@sentrivana sentrivana changed the title fix(crons): Make @monitor work with async functions fix(crons): Make monitor work with async functions Mar 27, 2024
@sentrivana sentrivana marked this pull request as ready for review March 27, 2024 12:26
@sentrivana sentrivana self-assigned this Mar 27, 2024
@sentrivana sentrivana changed the title fix(crons): Make monitor work with async functions fix(crons): Make monitor async friendly Mar 27, 2024
sentry_sdk/crons/_decorator.py Outdated Show resolved Hide resolved
sentry_sdk/crons/_decorator.py Outdated Show resolved Hide resolved
@sentrivana sentrivana added this to the Better async support milestone Mar 28, 2024
@sentrivana sentrivana merged commit b742c45 into master Mar 28, 2024
123 of 124 checks passed
@sentrivana sentrivana deleted the ivana/make-monitor-work-with-async-funs branch March 28, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants