-
Notifications
You must be signed in to change notification settings - Fork 554
Add async transport #4614
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
base: srothh/async-task-worker
Are you sure you want to change the base?
Add async transport #4614
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## srothh/async-task-worker #4614 +/- ##
============================================================
- Coverage 84.51% 83.86% -0.65%
============================================================
Files 158 158
Lines 15519 15657 +138
Branches 2449 2473 +24
============================================================
+ Hits 13116 13131 +15
- Misses 1649 1769 +120
- Partials 754 757 +3
|
1a129f7
to
97c5e3d
Compare
373942e
to
f01b00d
Compare
97c5e3d
to
b5eda0e
Compare
c541bd7
to
f5ef707
Compare
fca8740
to
328d8ad
Compare
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.
nice work. some nitpicking, a question and a change suggestion
sentry_sdk/transport.py
Outdated
}, | ||
) | ||
|
||
def _flush_client_reports(self: Self, force: bool = False) -> None: |
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.
are there plans to make this async in the future?
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.
I left it sync currently because it itself does not have an async component. But making it async would allow the entire request path to be async without changing functionality (and might also make it so the worker only needs to handle async callbacks), so it makes sense I think to make it an async function
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.
Changed this to be an async coroutine; this also meant that I could go back to the worker and simplify the callbacks to only handle async without breaking it !
Add an implementation of Transport to work with the async background worker and HTTPCore async. GH-4582
Async Transport now properly checks for the presence of the event loop in capture_envelop, and drops items in case the event loop is no longer running for some reason. GH-4582
Implement a kill method that properly shuts down the async transport. The httpcore async connection pool needs to be explicitly shutdown at the end of its usage. GH-4582
Fix type errors resulting from async override and missing type definition in the async transport. GH-4582
Add a try/catch to ensure silent fail on kill in case the event loop shuts down. GH-4582
Fix the event loop check in make_transport so that it does not throw a runtime error but rather falls back correctly. GH-4582
Make kill optionally return a task for async transport. This allows for a blocking kill operation if the caller is in an async context. GH-4582
295a0e9
to
6cb72ad
Compare
The lint error is in an unrelated file, not sure why it is happening |
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.
Looks good now! (the linting just fails because openfeature released something yesterday that breaks our linting. ignore this)
Add async implementation of the abstract Transport class. This transport utilizes the async task worker as well as the httpcore async functionality.
Thread Safety: As capture_envelope is registered by the client as a callback for several background threads in the sdk, which are not running the event loop, capture_envelope in the transport is made to be thread safe and allow for execution on the event loop from other threads. The same is currently not the case for flush, as there does not seem to be a usage from background threads, however if necessary, it can also be added.
HTTP2 support: Currently not activated, but from the looks of the httpcore docs it should be as simple as setting the http2 in the init of the pool to true. This likely makes sense to support, as HTTP2 shows great performance improvements with concurrent requests.
Kill: The kill method is sync, but the pool needs to be closed asynchronously. Currently, this is done by launching a task. However, the task cannot be awaited in sync code without deadlocking, therefore kill followed by an immediate loop shutdown could technically lead to resource leakage. Therefore, I decided to make kill optionally return the async task, so it can be awaited if called from an async context.
Note also that parts of the code are very similar to the HTTP2 integration, as they both use the httpcore library. Maybe in a later PR there could be a shared superclass to avoid code duplication?
GH-4582