feat(celery): Send queue name to Sentry#2984
Conversation
b02c1eb to
759bd53
Compare
Struggling to get this test to work due to complex test setup
|
Due to the current way we test Celery, it appears there is no straightforward way to test this change in our test suite. The problem is that our tests run with Celery's eager mode, which means the tasks are run synchronously in the same process, without any queueing. Since there is no queue, we never set We should improve our Celery tests, and add a test for this change, in a separate PR. |
| try: | ||
| return f(*args, **kwargs) | ||
| with sentry_sdk.start_span( | ||
| op=OP.QUEUE_TASK_CELERY, description=task.name |
There was a problem hiding this comment.
Is there a better op we could use here? queue.task.celery seemed like the most fitting op; however, since the parent transaction also has op set to queue.task.celery, this leads to the span having the same op as its parent, which could be somewhat confusing
There was a problem hiding this comment.
Yes, could be confusing. Lets see what @cleptric spec on develop docs will look like and then see if we should add a new op for this.
There was a problem hiding this comment.
Will be queue.process if I get the context here right.
We'll put celery on messaging.system.
There was a problem hiding this comment.
So this would be a new span op @cleptric? I do not see queue.process listed anywhere on our list of span ops.
There was a problem hiding this comment.
We'll use these values, prefixed with queue.
https://opentelemetry.io/docs/specs/semconv/attributes-registry/messaging/
Just a sidenote, but this is also basically the reason why we can't test #2377 properly at the moment. So as soon as we can run the tests in non-eager mode we also unblock at least one other feature. |
759bd53 to
0e3c823
Compare
antonpirker
left a comment
There was a problem hiding this comment.
The one assert in the tests I do not get.
And because of the "always-eager" there is no way we can tests for messaging.destination.name right? Do you have any other PR plan on how we could run tests with a queue?
| try: | ||
| return f(*args, **kwargs) | ||
| with sentry_sdk.start_span( | ||
| op=OP.QUEUE_TASK_CELERY, description=task.name |
There was a problem hiding this comment.
Yes, could be confusing. Lets see what @cleptric spec on develop docs will look like and then see if we should add a new op for this.
I could not find an obvious way to test |
8028917 to
5a99aad
Compare
Struggling to get this test to work due to complex test setup
|
@antonpirker Turns out that we can test for |
2fea931 to
3bf4c12
Compare
|
Failing tests should be fixed by this pr: #3030 |
f1671f5 to
67e2531
Compare
67e2531 to
e8ae74e
Compare
Send the queue name to Sentry for Celery tasks using the default exchange. The queue name is sent as span data with the key `messaging.destination.name` within a new span op named "queue.process". Also, add tests for the new behavior. Ref GH-2961
e8ae74e to
72ac630
Compare
Send the queue name to Sentry for Celery tasks using the default exchange. The queue name is sent as span data with the key
messaging.destination.name.Closes GH-2961