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

chore: requires user to pass context to propagation APIs #1734

Merged

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Dec 9, 2020

Which problem is this PR solving?

At least for extract() using the current active context is often a bad choice because usually a plugin wants to use an extracted span instead the current active.

Short description of the changes

Change propagation APIs to require context as first argument instead of using active context on default. This ensures that plugin developers have to be explicit instead of relying on a potential wrong default.

This PR is an enhanced but API breaking variant of #1714.

At least for extract() using the current active context is often a bad
choice because usually a plugin wants to use an extracted span instead
the current active.

Change propagation APIs to require context as first argument instead of
using active context on default. This ensures that plugin developers
have to be explicit instead of relying on a potential wrong default.
Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #1734 (0e2db5f) into master (0d06a52) will increase coverage by 0.20%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1734      +/-   ##
==========================================
+ Coverage   91.96%   92.17%   +0.20%     
==========================================
  Files         167      167              
  Lines        5599     5594       -5     
  Branches     1193     1191       -2     
==========================================
+ Hits         5149     5156       +7     
+ Misses        450      438      -12     
Impacted Files Coverage Δ
...telemetry-plugin-grpc-js/src/server/patchServer.ts 88.52% <ø> (ø)
packages/opentelemetry-api/src/api/propagation.ts 92.85% <100.00%> (+36.60%) ⬆️
...ges/opentelemetry-instrumentation-http/src/http.ts 95.49% <100.00%> (ø)
...emetry-instrumentation-xml-http-request/src/xhr.ts 96.77% <100.00%> (ø)
packages/opentelemetry-plugin-fetch/src/fetch.ts 96.52% <100.00%> (-0.03%) ⬇️
...s/opentelemetry-plugin-grpc-js/src/client/utils.ts 93.25% <100.00%> (ø)
packages/opentelemetry-plugin-grpc/src/grpc.ts 93.26% <100.00%> (ø)
packages/opentelemetry-plugin-http/src/http.ts 97.28% <100.00%> (ø)
...ackages/opentelemetry-shim-opentracing/src/shim.ts 87.70% <100.00%> (ø)

@obecny
Copy link
Member

obecny commented Dec 14, 2020

@Flarna pls allow edits or update the branch

@Flarna
Copy link
Member Author

Flarna commented Dec 14, 2020

@obecny I enabled it on my PR but as far as I know github doesn't support/allow edits for org repos like that one I have to use because of company rules.

@Flarna
Copy link
Member Author

Flarna commented Dec 14, 2020

Failed test seems to be unrelated.

   1) Packages
       get
         should create a span for GET requests and add propagation headers by using got package:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/__w/opentelemetry-js/opentelemetry-js/packages/opentelemetry-plugin-http/test/functionals/http-package.test.ts)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

@obecny
Copy link
Member

obecny commented Dec 14, 2020

Failed test seems to be unrelated.

   1) Packages
       get
         should create a span for GET requests and add propagation headers by using got package:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/__w/opentelemetry-js/opentelemetry-js/packages/opentelemetry-plugin-http/test/functionals/http-package.test.ts)
      at listOnTimeout (internal/timers.js:554:17)
      at processTimers (internal/timers.js:497:7)

It is failing in few other places. It is http plugin test, @vmarchaud you are the most familiar with this plugin, do you have maybe idea what can be the issue ? for example here too https://github.com/open-telemetry/opentelemetry-js/pull/1687/checks?check_run_id=1550385379

@vmarchaud
Copy link
Member

It is failing in few other places. It is http plugin test, @vmarchaud you are the most familiar with this plugin, do you have maybe idea what can be the issue ? for example here too https://github.com/open-telemetry/opentelemetry-js/pull/1687/checks?check_run_id=1550385379

I didn't wrote that part but from the comment its seems that got is cannot be mocked so we tries to reach http://info.cern.ch which is currenly down.
I think its safe to merge this and tries to fix the got problem in a seperate PR, WDYT ?

@obecny
Copy link
Member

obecny commented Dec 14, 2020

It is failing in few other places. It is http plugin test, @vmarchaud you are the most familiar with this plugin, do you have maybe idea what can be the issue ? for example here too https://github.com/open-telemetry/opentelemetry-js/pull/1687/checks?check_run_id=1550385379

I didn't wrote that part but from the comment its seems that got is cannot be mocked so we tries to reach http://info.cern.ch which is currenly down.
I think its safe to merge this and tries to fix the got problem in a seperate PR, WDYT ?

Yea it makes sense, now I see this external domain, thx @vmarchaud for explanation

@obecny obecny merged commit 5701473 into open-telemetry:master Dec 14, 2020
@Flarna Flarna deleted the explicit-propagator-api branch December 14, 2020 15:23
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
…try#1734)

* chore: requires user to pass context to propagation APIs

At least for extract() using the current active context is often a bad
choice because usually a plugin wants to use an extracted span instead
the current active.

Change propagation APIs to require context as first argument instead of
using active context on default. This ensures that plugin developers
have to be explicit instead of relying on a potential wrong default.

* (chore) fix lint

* add test

* Adapt http instrumentation

* fix: update integration testserver
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
…try#1734)

* chore: requires user to pass context to propagation APIs

At least for extract() using the current active context is often a bad
choice because usually a plugin wants to use an extracted span instead
the current active.

Change propagation APIs to require context as first argument instead of
using active context on default. This ensures that plugin developers
have to be explicit instead of relying on a potential wrong default.

* (chore) fix lint

* add test

* Adapt http instrumentation

* fix: update integration testserver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants