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.

Sign up for GitHub

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

Jump to bottom

Added context propagation support to celery instrumentation #1135

Merged
merged 2 commits into from
Sep 29, 2020
Merged

Added context propagation support to celery instrumentation #1135

merged 2 commits into from
Sep 29, 2020

Conversation

owais
Copy link
Contributor

@owais owais commented Sep 17, 2020 •
edited
Loading

Description

  • Adds context propagation to celery instrumentation.
  • Updated celery span operation names to include task type and task name.

fixes #862
fixes #876

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

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

  • Manual tests
  • Added unit test
  • Updated docker tests

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@owais owais force-pushed the celery-propagation branch 14 times, most recently from f909ff7 to 61b95f4 Compare September 17, 2020 20:09
@owais owais marked this pull request as ready for review September 17, 2020 20:09
@owais owais requested a review from a team September 17, 2020 20:09
@owais owais force-pushed the celery-propagation branch from 61b95f4 to 1f72b1e Compare September 17, 2020 20:24
codeboten
codeboten approved these changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this change looks pretty good to me. @owais is this addressing any of the open issues around celery integration? https://github.com/open-telemetry/opentelemetry-python/issues?q=is%3Aissue+is%3Aopen+celery

@owais
Copy link
Contributor Author

owais commented Sep 23, 2020 •
edited
Loading

I think it should fix #876 and probably #862 as well. This PR does not fix #970 but it updates the docs to ackowledge the issue and recommends a solution (delayed instantiate tracing in celery workers so each worker gets it's own processor/exporter instance) See "setting up tracing" section and updated code examples in celery docs. @codeboten

@codeboten
Copy link
Contributor

@owais Thanks for updating the description, this is great! Need one more approval. @open-telemetry/python-approvers approvers please take a look

lzchen
lzchen reviewed Sep 25, 2020
View reviewed changes
instrumentation/opentelemetry-instrumentation-celery/CHANGELOG.md Outdated
@@ -2,6 +2,9 @@

## Unreleased

- Span operation names now include the task type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add PR link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@lzchen
Copy link
Contributor

lzchen commented Sep 25, 2020

Curious does this need to be implemented for other instrumentations or is this specific to celery (explicit context propagation)?

@owais
Copy link
Contributor Author

owais commented Sep 25, 2020

This is very specific to celery.

@owais owais force-pushed the celery-propagation branch from 6f6ee05 to 0e6df16 Compare September 25, 2020 18:16
@lzchen
Copy link
Contributor

lzchen commented Sep 25, 2020

@owais

This is very specific to celery.

Can you explain why?

@owais
Copy link
Contributor Author

owais commented Sep 25, 2020

Curious does this need to be implemented for other instrumentations or is this specific to celery (explicit context propagation)?

Sorry, I was answering this and confirming that it is specific to celery (celery context propagation) and does not apply to other instrumentations.

@owais owais requested a review from lzchen September 25, 2020 23:07
@codeboten codeboten merged commit 9be899e into open-telemetry:master Sep 29, 2020
@codeboten codeboten mentioned this pull request Sep 29, 2020
Closed
7 tasks
@owais owais deleted the celery-propagation branch September 29, 2020 18:18
@MrMegaMango
Copy link

celery context propagation still doesn't work on apply_sync

@emdneto
Copy link
Member

emdneto commented Oct 18, 2024

@MrMegaMango Please open a new issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzchen lzchen lzchen approved these changes

@codeboten codeboten codeboten approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@owais @codeboten @lzchen @MrMegaMango @emdneto