Skip to content
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

Handling of Celery root spans is broken #11624

Open
iherasymenko opened this issue Dec 5, 2024 · 7 comments
Open

Handling of Celery root spans is broken #11624

iherasymenko opened this issue Dec 5, 2024 · 7 comments
Assignees

Comments

@iherasymenko
Copy link

iherasymenko commented Dec 5, 2024

Given:

pyproject.toml

[tool.poetry]
package-mode = false

[tool.poetry.dependencies]
python = "^3.12"
celery = { version = "5.4.0" }
#ddtrace = { git = "https://github.com/DataDog/dd-trace-py", rev = "3a859123249b5ecb00540a96243df2465bceb7c2" } # the commit that introduced the regression
#ddtrace = { git = "https://github.com/DataDog/dd-trace-py", rev = "758fb7dabcdd4db3e8aed7db6a74d639c217b1b1" } # last known working commit
ddtrace = "2.17.2" # The latest to date version; still broken
#ddtrace = "2.12.3" # The version the regression was introduced
#ddtrace = "2.12.2" # The last known working version

main.py

from celery import Celery
from ddtrace import tracer

app = Celery(
    'tasks',
    broker='amqp://localhost',
    broker_connection_retry_on_startup=True,
)

app.conf.accept_content = ['application/json', 'application/x-python-serialize']


@app.task(name='Task1')
def task1():
    print("Task 1 finished")


@app.task(name='Task2')
def task2():
    span1 = tracer.current_root_span()
    if span1 is not None:
        print("Writing my.tag to root span")
        span1.set_tag("my.tag", "my_value")
    else:
        print("Writing failed: No root span found")
    task1.delay()

    span2 = tracer.current_root_span()
    if span2 is not None:
        print(f"Reading my.tag: The value is {span2.get_tags().get('my.tag')}")
    else:
        print("Reading failed: No current span found")

    print("Task 2 finished")


if __name__ == '__main__':
    task2.delay()

Run:

poetry lock && poetry install --sync

and run:

poetry run ddtrace-run celery -A main worker

and then run:

poetry run ddtrace-run python main.py 

Version 2.12.2 and older:

[2024-12-04 19:05:17,051: WARNING/ForkPoolWorker-6] Writing my.tag to root span
[2024-12-04 19:05:17,060: WARNING/ForkPoolWorker-6] Reading my.tag: The value is my_value
[2024-12-04 19:05:17,060: WARNING/ForkPoolWorker-6] Task 2 finished
[2024-12-04 19:05:17,061: WARNING/ForkPoolWorker-7] Task 1 finished

Version 2.12.3 and newer:

[2024-12-04 19:12:27,720: WARNING/ForkPoolWorker-6] Writing my.tag to root span
[2024-12-04 19:12:27,738: WARNING/ForkPoolWorker-6] Reading failed: No current span found
[2024-12-04 19:12:27,738: WARNING/ForkPoolWorker-6] Task 2 finished
[2024-12-04 19:12:27,741: WARNING/ForkPoolWorker-7] Task 1 finished
@oranav
Copy link
Contributor

oranav commented Dec 5, 2024

I think this is essentially the same issue as #11479. In the other issue we trigger another task using a chain, here we just do it manually -- but it leads to the same consequence.

@wantsui
Copy link
Collaborator

wantsui commented Dec 5, 2024

@iherasymenko - Sorry for the change in behavior! Thanks for the reproduction app, and for flagging this with @oranav ! I just wrote this comment in #11479, but I'm taking a look at your example too to make sure we can capture this correctly in our tests without accidentally keeping the spans open indefinitely if an error occurs.

For now, I also recommend pinning your application to a different tracer, and I'll report back to both issues once I have an update.

@iherasymenko
Copy link
Author

@wantsui thank you for looking into this. Unfortunately pinning the last known working version is not an option for us as it does not include a fix for #10714 .

@oranav
Copy link
Contributor

oranav commented Dec 17, 2024

@iherasymenko You can try my workaround here.

@wantsui
Copy link
Collaborator

wantsui commented Dec 18, 2024

As an update, I got the approvals I needed to get this merged. I'm also working to backport this fix to 2.18.0, 2.17.0, and 2.16.0. Would any of those versions work for you @iherasymenko ?

I know there was mention of targeting 2.14.0 for a pymongo issue, so just want to check in.

@iherasymenko
Copy link
Author

iherasymenko commented Dec 18, 2024

I think 2.17 should work. Passing the torch to @Nanared1.

wantsui added a commit that referenced this issue Dec 19, 2024
…chains scenario (#11498)

We've made a few changes to handle celery context recently, including:
#10676

In particular the goal of
#10676 was to handle a
scenario where a long running task may run into an exception, preventing
it from closing.

Unfortunately, this scenario did not account for cases where tasks are
chained and may not close until later.

See: #11479 and
#11624

With this PR, the sample app in
#11479 would attach the
celery specific span back to the root span.

I also need to add tests for the chains scenario.

Related to AIDM-494

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
github-actions bot pushed a commit that referenced this issue Dec 19, 2024
…chains scenario (#11498)

We've made a few changes to handle celery context recently, including:
#10676

In particular the goal of
#10676 was to handle a
scenario where a long running task may run into an exception, preventing
it from closing.

Unfortunately, this scenario did not account for cases where tasks are
chained and may not close until later.

See: #11479 and
#11624

With this PR, the sample app in
#11479 would attach the
celery specific span back to the root span.

I also need to add tests for the chains scenario.

Related to AIDM-494

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit e8aab65)
github-actions bot pushed a commit that referenced this issue Dec 19, 2024
…chains scenario (#11498)

We've made a few changes to handle celery context recently, including:
#10676

In particular the goal of
#10676 was to handle a
scenario where a long running task may run into an exception, preventing
it from closing.

Unfortunately, this scenario did not account for cases where tasks are
chained and may not close until later.

See: #11479 and
#11624

With this PR, the sample app in
#11479 would attach the
celery specific span back to the root span.

I also need to add tests for the chains scenario.

Related to AIDM-494

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit e8aab65)
github-actions bot pushed a commit that referenced this issue Dec 19, 2024
…chains scenario (#11498)

We've made a few changes to handle celery context recently, including:
#10676

In particular the goal of
#10676 was to handle a
scenario where a long running task may run into an exception, preventing
it from closing.

Unfortunately, this scenario did not account for cases where tasks are
chained and may not close until later.

See: #11479 and
#11624

With this PR, the sample app in
#11479 would attach the
celery specific span back to the root span.

I also need to add tests for the chains scenario.

Related to AIDM-494

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

(cherry picked from commit e8aab65)
wantsui added a commit that referenced this issue Dec 19, 2024
…chains scenario [backport 2.18] (#11806)

Backport e8aab65 from #11498 to 2.18.

We've made a few changes to handle celery context recently, including:
#10676

In particular the goal of
#10676 was to handle a
scenario where a long running task may run into an exception, preventing
it from closing.

Unfortunately, this scenario did not account for cases where tasks are
chained and may not close until later.

See: #11479 and
#11624

With this PR, the sample app in
#11479 would attach the
celery specific span back to the root span.

I also need to add tests for the chains scenario.

Related to AIDM-494

## Checklist
- [x] PR author has checked that all the criteria below are met
- The PR description includes an overview of the change
- The PR description articulates the motivation for the change
- The change includes tests OR the PR description describes a testing
strategy
- The PR description notes risks associated with the change, if any
- Newly-added code is easy to change
- The change follows the [library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] Reviewer has checked that all the criteria below are met 
- Title is accurate
- All changes are related to the pull request's stated goal
- Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- Testing strategy adequately addresses listed risks
- Newly-added code is easy to change
- Release note makes sense to a user of the library
- If necessary, author has acknowledged and discussed the performance
implications of this PR as reported in the benchmarks PR comment
- Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: wantsui <[email protected]>
@wantsui
Copy link
Collaborator

wantsui commented Dec 20, 2024

As an update, we got 2.18.1 out with the celery update: v2.18.1 (release) . Can you give that a try?

Unfortunately, we can't release patches for 2.17.x or 2.16.x due to the holidays and code freezes, so that'll be out in January instead. I'll update this thread once we have those other versions out in January. Apologies for the inconvenience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants