-
Notifications
You must be signed in to change notification settings - Fork 569
feat(django): Add span around Task.enqueue
#5209
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
Conversation
Task.enqueueTask.enqueue
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5209 +/- ##
==========================================
+ Coverage 82.87% 84.26% +1.39%
==========================================
Files 181 182 +1
Lines 18459 18489 +30
Branches 3287 3290 +3
==========================================
+ Hits 15297 15580 +283
+ Misses 2141 1895 -246
+ Partials 1021 1014 -7
|
| ) | ||
|
|
||
| with sentry_sdk.start_span( | ||
| op=OP.QUEUE_SUBMIT_DJANGO, name=name, origin=DjangoIntegration.origin |
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.
Bug: Queue span uses HTTP origin instead of queue origin
The queue.submit.django span uses DjangoIntegration.origin which is "auto.http.django", but queue operations in other integrations (arq, celery, huey) consistently use "auto.queue.{name}" as the origin. This causes the span to be incorrectly categorized as an HTTP operation rather than a queue operation, which is inconsistent with other queue integrations and may affect how Sentry analyzes and displays queue-related telemetry data.
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 think this is a valid point @sentrivana.
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.
IDK if origin is actually used anywhere on the server, but the OG idea behind the field is just to be able to determine which integration the span is coming from, nothing more.
When someone actually uses Celery/some other queuing library as a backend for django.tasks, those will have their own origin.
alexander-alderman-webb
left a comment
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.
LGTM, see the comments before merging 🙏
| return old_task_enqueue(self, *args, **kwargs) | ||
|
|
||
| name = ( | ||
| getattr(self.func, "__name__", repr(self.func)) or "<unknown Django task>" |
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.
nitpick: would it make more sense to use __qualname__ here?
| getattr(self.func, "__name__", repr(self.func)) or "<unknown Django task>" | |
| getattr(self.func, "__qualname__", repr(self.func)) or "<unknown Django task>" |
| ) | ||
|
|
||
| with sentry_sdk.start_span( | ||
| op=OP.QUEUE_SUBMIT_DJANGO, name=name, origin=DjangoIntegration.origin |
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 think this is a valid point @sentrivana.
| if integration is None: | ||
| return old_task_enqueue(self, *args, **kwargs) | ||
|
|
||
| name = qualname_from_function(self.func) or "<unknown Django task>" |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
It's not public, but it's def there and pretty integral to the whole tasks feature, so I don't expect it to disappear.
Description
Add a
queue.submit.djangospan when aTaskin enqueued via Django.Issues
Closes #5201
Closes PY-2006
Reminders
tox -e linters.feat:,fix:,ref:,meta:)