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
feat(celery): Send queue name to Sentry #2984
base: master
Are you sure you want to change the base?
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. |
def _inner(*args, **kwargs): | ||
# type: (*Any, **Any) -> Any | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this would be a new span op
@cleptric? I do not see queue.process
listed anywhere on our list of span op
s.
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.
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
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.
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?
def _inner(*args, **kwargs): | ||
# type: (*Any, **Any) -> Any | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
worked for me in testing
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`. Ref GH-2961
The previous test only checked that the queue was set on the span when the queue had the default name ("celery"). This test adds a check to ensure the queue also gets set on the span when it has a non-default value.
This test checks that `messaging.destination.name` is only set for the default exchange. Other exchanges should not set this value, since the routing key may be different from the queue name.
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
.Closes GH-2961