celery: allow using links instead of child spans for task execution #3779
Conversation
| SpanAttributes.MESSAGING_DESTINATION: "celery", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
This section of code is just duplicated from test_task, the remaining assertions below are the only meaningful changes
jlc-christie
force-pushed
the
celery-allow-using-links-instead-of-child-spans
branch
from
b87b7a0 to
541face
Compare
jlc-christie
force-pushed
the
celery-allow-using-links-instead-of-child-spans
branch
from
541face to
b87b7a0
Compare
jlc-christie
force-pushed
the
celery-allow-using-links-instead-of-child-spans
branch
from
99cc8be to
4f1eda6
Compare
|
@xrmx just rebased latest from main onto this branch, lmk if it's easier for your review process to just let it drift and rebase after you've reviewed |
| The ``CeleryInstrumentor().instrument()`` method accepts the following arguments: | ||
| * ``use_links`` (bool): When ``True``, Celery task execution spans will be linked to the |
There was a problem hiding this comment.
From #3002 (comment) I understand we should always add links but make the parent / child relationship optional instead?
There was a problem hiding this comment.
It seems a "one-or-the-other" situation to me...
There was a problem hiding this comment.
It seems a "one-or-the-other" situation to me...
Agreed, I went with a default of False since it seemed like this would be a pretty major change for consumers of this package that weren't expecting the update without warning, whilst still giving consumers who care about this an option to opt-out of the current behaviour.
Maybe the changing of the default from False to True could be packaged up in a future release and a warning logged mentioning the change in that future release so people are aware of the incoming change?
Personally no objection from my side for changing the default though so if this isn't as contentious as I'm imagining it would be then I'm happy to flip the value.
There was a problem hiding this comment.
@xrmx are you happy with this approach? As mentioned I don't feel strongly either way but don't want to leave this PR open too long and end up drifting from main.
| tracer_provider, | ||
| schema_url="https://opentelemetry.io/schemas/1.11.0", | ||
| ) | ||
| # pylint: disable=attribute-defined-outside-init |
There was a problem hiding this comment.
Not related to this PR, but this pylint rule seems useless in this project, can we disable it?
There was a problem hiding this comment.
True... I added it to keep it consistent with the rest of the file but I'll add an issue to the project for this
...on/opentelemetry-instrumentation-celery/src/opentelemetry/instrumentation/celery/__init__.py
Outdated
Show resolved
Hide resolved
jlc-christie
force-pushed
the
celery-allow-using-links-instead-of-child-spans
branch
from
d6f5fa1 to
3bd9d2e
Compare
4 participants
Description
As described in #3002, the current default behaviour results in all celery tasks that execute are child spans of the code that pushed it on to the broker, which conflicts with the semantic conventions.
This PR doesn't change the default behaviour since this may be undesirable for consumers expecting the behaviour to be consistent.
Fixes #3002 by providing an optional parameter to the
.instrument()method to allow for optionally using span links, e.g.Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
use_links=Falseand alsouse_links=Trueand observed the expected behaviour: when the flag isFalse(default) all task executions of a fan-out job are child spans of the code that enqueued it and whenTruethey appeared as linked spans.Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.