-
Notifications
You must be signed in to change notification settings - Fork 554
feat(integrations): Add tracing to DramatiqIntegration #4571
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: master
Are you sure you want to change the base?
Conversation
Adds tracing support to DramatiqIntegration getsentry#3454
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.
Hi @Igreh, I took a quick glance at your PR – at a high level this seems reasonable; however, before I do a full review, I would appreciate if you could please add some tests for these changes, since we need tests before we can consider merging this.
sentry_sdk/integrations/dramatiq.py
Outdated
@@ -85,20 +89,27 @@ class SentryMiddleware(Middleware): # type: ignore[misc] | |||
DramatiqIntegration. | |||
""" | |||
|
|||
# type: contextvars.ContextVar[Transaction] | |||
_transaction = contextvars.ContextVar("_transaction", default=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.
Why are we using a ContextVar
here, instead of just a simple instance variable on the SentryMiddleware
?
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.
To isolate variable between threads:
- Dramatiq can be started in multithreading mode:
dramatiq --processes 1 --threads 5 ...
- And I was inspired by official middleware:
CurrentMessageMiddleware
I will try recheck if it's actually needed
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #4571 +/- ##
==========================================
- Coverage 80.74% 80.72% -0.03%
==========================================
Files 156 156
Lines 16483 16491 +8
Branches 2801 2801
==========================================
+ Hits 13309 13312 +3
- Misses 2296 2297 +1
- Partials 878 882 +4
|
Hi! Edit: |
Hey @Igreh, it'd be good to have "end-to-end" (i.e., not unit tests) for the different tracing code paths. Stuff like:
...and/or whatever you consider important so that when the test suite is run and ends up green, you'd have a reasonable amount of confidence that tracing in the Dramatiq integration is working. You can have a look at some of these similar integrations and their test suites (e.g. arq, huey) for inspiration. Thanks for working on this! Let us know if you need any help. |
- add trace propagation - set dramatiq_task_id as tag instead of extra - new tests - fix mypy issues Issue: getsentry#3454
Hi @sentrivana ! |
Adds tracing support to DramatiqIntegration #3454