Fix custom headers propagation for protocol 1 hybrid messages #6374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request fixes 1 alert when merging 14e5a84 into ea37db1 - view on LGTM.com fixed alerts:
|
beezz
force-pushed
the
fix-custom-headers-hybrid-msg
branch
from
14e5a84
to
1227012
Compare
|
This pull request fixes 2 alerts when merging 1227012 into ea37db1 - view on LGTM.com fixed alerts:
|
beezz
force-pushed
the
fix-custom-headers-hybrid-msg
branch
from
1227012
to
70ad00e
Compare
|
This pull request fixes 2 alerts when merging 70ad00e into ea37db1 - view on LGTM.com fixed alerts:
|
auvipy
approved these changes
Sep 30, 2020
|
Hi @auvipy, thank you very much for taking your time with this PR. What do you think about backporting this fix to 4.x? |
|
@auvipy I've added it to the 4.5 milestone so you won't forget. |
|
@auvipy Did you backport this? |
luzfcb
added a commit
to labcodes/celery
that referenced
this pull request
Nov 23, 2021
… 1 hybrid messages
15 tasks
emmettbutler
added a commit
to DataDog/dd-trace-py
that referenced
this pull request
Mar 22, 2023
The issue referenced was [resolved](celery/celery#6374), but it actually does not apply to the bit of code it's pointing to. Fixes #4649 I tried removing the code referenced by the comment, and it causes the test `tests/contrib/celery/test_integration.py::CeleryDistributedTracingIntegrationTask::test_distributed_tracing_propagation_async` to fail. As far as I can tell from about an hour of investigation, the celery [fix](celery/celery#4356) only affects "hybrid messages" (those that blend protocol 1 and 2 syntax as part of a rolling Celery upgrade), which our test suite doesn't use.
IlyaMichlin
pushed a commit
to IlyaMichlin/dd-trace-py
that referenced
this pull request
Mar 30, 2023
The issue referenced was [resolved](celery/celery#6374), but it actually does not apply to the bit of code it's pointing to. Fixes DataDog#4649 I tried removing the code referenced by the comment, and it causes the test `tests/contrib/celery/test_integration.py::CeleryDistributedTracingIntegrationTask::test_distributed_tracing_propagation_async` to fail. As far as I can tell from about an hour of investigation, the celery [fix](celery/celery#4356) only affects "hybrid messages" (those that blend protocol 1 and 2 syntax as part of a rolling Celery upgrade), which our test suite doesn't use.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: Before submitting this pull request, please review our contributing
guidelines.
Description
This is an attempt to fix custom header propagation that broke for us after migration from Celery 3.X to 4.X.
I believe it is the same issue as described in #4875
For the hybrid messages of protocol 1, the
message.headersare ignored, this PR merges them with the rest of the headers extracted from the body.Requestclass - fix(tracing): Fix celery custom headers propagation getsentry/sentry#20996