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): strip otel custom http header #983 #984

Merged
merged 1 commit into from
May 11, 2020

Conversation

vmarchaud
Copy link
Member

see #983 and #965 (comment) for context

@vmarchaud vmarchaud self-assigned this Apr 26, 2020
@vmarchaud vmarchaud added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 26, 2020
@dyladan dyladan added the bug Something isn't working label Apr 29, 2020
@Flarna
Copy link
Member

Flarna commented Apr 29, 2020

Who removes the header in case http plugin is disabled?

@dyladan
Copy link
Member

dyladan commented Apr 29, 2020

Who removes the header in case http plugin is disabled?

Nobody. If this is important, maybe we should switch to use a hidden property on the request then? Or we could just wait for open-telemetry/opentelemetry-specification#530

I don't really like that we use header values as a signal between plugins. It is an observable side-effect, and quite an intrusive one at that. I much prefer the idea to use context to stop tracing.

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.

I'm going to approve this for now, but I think we should move to a context based system as soon as possible as it will be more generally applicable to e.g. dns

@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #984 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #984   +/-   ##
=======================================
  Coverage   95.13%   95.13%           
=======================================
  Files         212      212           
  Lines        8895     8899    +4     
  Branches      801      800    -1     
=======================================
+ Hits         8462     8466    +4     
  Misses        433      433           
Impacted Files Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts 97.70% <100.00%> (+0.01%) ⬆️
packages/opentelemetry-plugin-http/src/utils.ts 97.42% <100.00%> (+0.01%) ⬆️
...y-plugin-http/test/functionals/http-enable.test.ts 96.89% <100.00%> (+0.01%) ⬆️

@mayurkale22 mayurkale22 merged commit 35cda77 into open-telemetry:master May 11, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
* feat: use Otel SDK 1.2/0.28

* fixup! chore(deps): update dependency @types/react-dom to v18 (open-telemetry#974)

* host-metrics back to 0.27

* fix: correctly depen on contrib-test-utils

* fixup! fix: correctly depen on contrib-test-utils

* fixup! feat: use Otel SDK 1.2/0.28
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 13, 2024
* feat: use Otel SDK 1.2/0.28

* fixup! chore(deps): update dependency @types/react-dom to v18 (open-telemetry#974)

* host-metrics back to 0.27

* fix: correctly depen on contrib-test-utils

* fixup! fix: correctly depen on contrib-test-utils

* fixup! feat: use Otel SDK 1.2/0.28
martinkuba pushed a commit to martinkuba/opentelemetry-js that referenced this pull request Mar 16, 2024
* feat: use Otel SDK 1.2/0.28

* fixup! chore(deps): update dependency @types/react-dom to v18 (open-telemetry#974)

* host-metrics back to 0.27

* fix: correctly depen on contrib-test-utils

* fixup! fix: correctly depen on contrib-test-utils

* fixup! feat: use Otel SDK 1.2/0.28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(http-plugin): x-opentelemetry-outgoing-request isn't stripped before sending the request
6 participants