fix(celery): stop closing prerun_span too soon to account for Celery chains scenario #11498

wantsui · 2024-11-21T23:38:20Z

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

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
The change includes or references documentation updates if necessary
Backport labels are set (if applicable)

Reviewer Checklist

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 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

github-actions · 2024-11-21T23:38:53Z

CODEOWNERS have been resolved as:

releasenotes/notes/fix-celery-closed-spans-34ff43868c1e33b8.yaml        @DataDog/apm-python
tests/contrib/celery/run_tasks.py                                       @DataDog/apm-core-python @DataDog/apm-idm-python
tests/contrib/celery/tasks.py                                           @DataDog/apm-core-python @DataDog/apm-idm-python
tests/contrib/celery/test_chained_task.py                               @DataDog/apm-core-python @DataDog/apm-idm-python
ddtrace/contrib/internal/celery/app.py                                  @DataDog/apm-core-python @DataDog/apm-idm-python
ddtrace/contrib/internal/celery/signals.py                              @DataDog/apm-core-python @DataDog/apm-idm-python

pr-commenter · 2024-11-22T23:52:12Z

Benchmarks

Benchmark execution time: 2024-12-19 17:33:29

Comparing candidate commit 8d2128e in PR branch wantsui/celery-chain-task with baseline commit 315a48f in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 334 metrics, 2 unstable metrics.

tests/contrib/celery/test_integration.py

…with a test where the worker runs in a subprocess to better reflect actual worker behavior

datadog-dd-trace-py-rkomorn · 2024-12-09T16:59:02Z

Datadog Report

Branch report: wantsui/celery-chain-task
Commit report: 137f6e5
Test service: dd-trace-py

✅ 0 Failed, 30 Passed, 1438 Skipped, 1m 24.74s Total duration (35m 16.99s time saved)

tests/contrib/celery/test_chained_task.py

wantsui · 2024-12-18T23:20:12Z

/merge

dd-devflow · 2024-12-18T23:20:18Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-12-18 23:20:18 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 31m.

2024-12-19 01:20:39 UTC ❌ MergeQueue: The build pipeline has timeout

The merge request has been interrupted because the build took longer than expected. The current limit for the base branch 'main' is 120 minutes.

Possible reasons:

some mandatory checkruns are failing:
- require-checklist

wantsui · 2024-12-19T19:26:20Z

/merge

dd-devflow · 2024-12-19T19:26:29Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-12-19 19:26:28 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 31m.

2024-12-19 20:05:28 UTC ⚠️ MergeQueue: This merge request build was cancelled

[email protected] cancelled this merge request build

wantsui · 2024-12-19T20:05:18Z

/remove

dd-devflow · 2024-12-19T20:05:25Z

Devflow running: `/remove`

View all feedbacks in Devflow UI.

2024-12-19 20:05:24 UTC ℹ️ Devflow: /remove

…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)

…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]>

Remove postrun spans logic to account for chain tasks scenarios.

883e073

wantsui requested a review from tabgok November 21, 2024 23:38

Merge branch 'main' into wantsui/celery-chain-task

4c685b3

wantsui added 3 commits November 22, 2024 19:40

Fix spacing.

e860319

Add test with celery chains with the pipe operator.

4228d2d

Remove chain import due to using pipe operator.

bc68ed3

datadog-datadog-prod-us1 bot reviewed Nov 23, 2024

View reviewed changes

tests/contrib/celery/test_integration.py Outdated Show resolved Hide resolved

wantsui added 5 commits December 6, 2024 20:10

Replace test_integration.py test involving celery specific span tags …

1715c2c

…with a test where the worker runs in a subprocess to better reflect actual worker behavior

Remove unneeded import

416efa7

Update test for specific span tags.

102555c

Add release note.

d70f05b

Shorten regex.

e91ec2c

Merge branch 'main' into wantsui/celery-chain-task

4ca4e50

wantsui marked this pull request as ready for review December 9, 2024 17:44

wantsui requested review from a team as code owners December 9, 2024 17:44

wantsui requested a review from emmettbutler December 9, 2024 17:44

wantsui changed the title ~~fix(celery): account for a chains scenario~~ fix(celery): stop closing prerun_span too soon to account for Celery chains scenario Dec 9, 2024

tabgok reviewed Dec 9, 2024

View reviewed changes

tests/contrib/celery/test_chained_task.py Outdated Show resolved Hide resolved

tabgok reviewed Dec 9, 2024

View reviewed changes

tests/contrib/celery/test_chained_task.py Outdated Show resolved Hide resolved

wantsui added 5 commits December 9, 2024 13:08

Try without sleeps.

243e252

Remove time import.

451f851

Use whatever python the CI has for that run.

942178b

Add back one of the sleeps.

fd13bf9

Add slightly longer delay for different python versions.

6b14562

tabgok reviewed Dec 10, 2024

View reviewed changes

tests/contrib/celery/test_chained_task.py Outdated Show resolved Hide resolved

wantsui added backport 2.17 backport 2.18 labels Dec 18, 2024

dd-devflow bot added mergequeue-status: queued mergequeue-status: in_progress mergequeue-status: rejected and removed mergequeue-status: queued mergequeue-status: in_progress labels Dec 18, 2024

Merge branch 'main' into wantsui/celery-chain-task

8d2128e

dd-devflow bot added mergequeue-status: queued mergequeue-status: in_progress and removed mergequeue-status: rejected mergequeue-status: queued labels Dec 19, 2024

dd-devflow bot removed the mergequeue-status: in_progress label Dec 19, 2024

dd-devflow bot added the mergequeue-status: removed label Dec 19, 2024

wantsui enabled auto-merge (squash) December 19, 2024 20:06

wantsui merged commit e8aab65 into main Dec 19, 2024
567 of 568 checks passed

wantsui deleted the wantsui/celery-chain-task branch December 19, 2024 20:06

github-actions bot mentioned this pull request Dec 19, 2024

fix(celery): stop closing prerun_span too soon to account for Celery chains scenario [backport 2.16] #11804

Open

2 tasks

github-actions bot mentioned this pull request Dec 19, 2024

fix(celery): stop closing prerun_span too soon to account for Celery chains scenario [backport 2.17] #11805

Open

2 tasks

github-actions bot mentioned this pull request Dec 19, 2024

fix(celery): stop closing prerun_span too soon to account for Celery chains scenario [backport 2.18] #11806

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(celery): stop closing prerun_span too soon to account for Celery chains scenario #11498

fix(celery): stop closing prerun_span too soon to account for Celery chains scenario #11498

wantsui commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

pr-commenter bot commented Nov 22, 2024 •

edited

Loading

datadog-dd-trace-py-rkomorn bot commented Dec 9, 2024 •

edited

Loading

wantsui commented Dec 18, 2024

dd-devflow bot commented Dec 18, 2024 •

edited

Loading

wantsui commented Dec 19, 2024

dd-devflow bot commented Dec 19, 2024 •

edited

Loading

wantsui commented Dec 19, 2024

dd-devflow bot commented Dec 19, 2024

fix(celery): stop closing prerun_span too soon to account for Celery chains scenario #11498

fix(celery): stop closing prerun_span too soon to account for Celery chains scenario #11498

Conversation

wantsui commented Nov 21, 2024 • edited Loading

Checklist

Reviewer Checklist

github-actions bot commented Nov 21, 2024 • edited Loading

pr-commenter bot commented Nov 22, 2024 • edited Loading

Benchmarks

datadog-dd-trace-py-rkomorn bot commented Dec 9, 2024 • edited Loading

Datadog Report

wantsui commented Dec 18, 2024

dd-devflow bot commented Dec 18, 2024 • edited Loading

Devflow running: /merge

wantsui commented Dec 19, 2024

dd-devflow bot commented Dec 19, 2024 • edited Loading

Devflow running: /merge

wantsui commented Dec 19, 2024

dd-devflow bot commented Dec 19, 2024

Devflow running: /remove

wantsui commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

pr-commenter bot commented Nov 22, 2024 •

edited

Loading

datadog-dd-trace-py-rkomorn bot commented Dec 9, 2024 •

edited

Loading

dd-devflow bot commented Dec 18, 2024 •

edited

Loading

Devflow running: `/merge`

dd-devflow bot commented Dec 19, 2024 •

edited

Loading

Devflow running: `/merge`

Devflow running: `/remove`