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

feat(http-plugin): add options to disable new spans if no parent #931 #948

Merged
merged 7 commits into from
May 6, 2020

Conversation

vmarchaud
Copy link
Member

No description provided.

@vmarchaud vmarchaud self-assigned this Apr 8, 2020
@vmarchaud vmarchaud added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 8, 2020
@vmarchaud vmarchaud force-pushed the http-option-require-parent branch 3 times, most recently from 6efb8b0 to 8de7045 Compare April 12, 2020 12:00
@vmarchaud
Copy link
Member Author

As #930, lint is failing because of GTS versions

@codecov-io
Copy link

codecov-io commented Apr 12, 2020

Codecov Report

Merging #948 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #948      +/-   ##
==========================================
+ Coverage   95.01%   95.06%   +0.05%     
==========================================
  Files         212      212              
  Lines        8827     8898      +71     
  Branches      797      801       +4     
==========================================
+ Hits         8387     8459      +72     
+ Misses        440      439       -1     
Impacted Files Coverage Δ
packages/opentelemetry-plugin-http/src/http.ts 97.70% <100.00%> (+0.12%) ⬆️
...y-plugin-http/test/functionals/http-enable.test.ts 96.88% <100.00%> (+0.59%) ⬆️
...lemetry-plugin-http/test/utils/DummyPropagation.ts 100.00% <0.00%> (+6.66%) ⬆️

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.

look good

@dyladan
Copy link
Member

dyladan commented Apr 18, 2020

@open-telemetry/javascript-approvers we need more reviews on this

@vmarchaud vmarchaud force-pushed the http-option-require-parent branch 2 times, most recently from 77e9ee9 to e35ff55 Compare April 25, 2020 16:03
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.

LGTM thanks

@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Apr 29, 2020
@vmarchaud vmarchaud force-pushed the http-option-require-parent branch from 42e9c10 to cb1e933 Compare May 2, 2020 13:17
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, one comment

packages/opentelemetry-plugin-http/src/http.ts Outdated Show resolved Hide resolved
@vmarchaud vmarchaud force-pushed the http-option-require-parent branch from 6eed95e to 4cd48d1 Compare May 6, 2020 15:51
@dyladan
Copy link
Member

dyladan commented Aug 4, 2020

@vmarchaud can you help me understand why you decided to create a non recording span rather than just returning the NOOP span provided by the API?

You've introduced a hard dependency on our core/sdk, which means a third party implementation of an SDK will not be able to work with this plugin.

@vmarchaud
Copy link
Member Author

@dyladan Well if i remember correctly using a NoopSpan wasn't propagating the context, but if i'm mistaken i see no reason to not use a NoopSpan instead

@dyladan
Copy link
Member

dyladan commented Aug 4, 2020

I would like to remove the dependency on core for all plugins if possible. In the current state, plugins are only compatible with the default sdk, but won't be interoperable with third party sdks. The http plugin is one of the only ones that depends on something other than BasePlugin in core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants