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

fix(http-plugin): don't modify user's headers object in plugin #1060

Merged
merged 4 commits into from
May 18, 2020
Merged

fix(http-plugin): don't modify user's headers object in plugin #1060

merged 4 commits into from
May 18, 2020

Conversation

blumamir
Copy link
Member

Which problem is this PR solving?

Fixes #1059
Zipkin exporter is constructing an headers object once and then use the same object in all http outgoing requests to Zipkin server. If the http plugin delete a key from the headers object, it affects following requests which are sent without OT header, thus being traced and create infinite loop of spans.

Short description of the changes

When http plugin receive outgoing request with OT header, it first clone the headers object, and then delete from the clone without modifying the original user's object.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks great! Can you also check if this issue affects the jaeger exporter?

@dyladan dyladan added bug Something isn't working Merge:express labels May 14, 2020
@blumamir
Copy link
Member Author

Looks great! Can you also check if this issue affects the jaeger exporter?

From the code it looks like Jaeger Exporter is using UDP, so it should not be affected.

@dyladan
Copy link
Member

dyladan commented May 14, 2020

Looks great! Can you also check if this issue affects the jaeger exporter?

From the code it looks like Jaeger Exporter is using UDP, so it should not be affected.

There was a PR to introduce http sending... possible it wasn't merged yet.

Ah yes #965 still open. Thanks for the fix.

@dyladan
Copy link
Member

dyladan commented May 14, 2020

@OlivierAlbertini as original plugin author would be nice to get your 👍 here

Copy link
Member

@OlivierAlbertini OlivierAlbertini left a comment

Choose a reason for hiding this comment

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

perhaps, we could add a test for ensuring that we will have no regression on this, WDYT ?

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm

@dyladan dyladan merged commit 637f982 into open-telemetry:master May 18, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zipkin exported http traffic is traced, causing infinite loop
6 participants