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.

Sign up for GitHub

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

Jump to bottom

feat: use context-based tracing #816

Merged
merged 17 commits into from
Mar 2, 2020
Merged

feat: use context-based tracing #816

merged 17 commits into from
Mar 2, 2020

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Feb 26, 2020

Which problem is this PR solving?

Short description of the changes

  • Use context and propagation APIs for all span creation

@dyladan dyladan mentioned this pull request Feb 26, 2020
Closed
46 tasks
@dyladan dyladan added this to the Beta milestone Feb 26, 2020
@dyladan dyladan added the enhancement New feature or request label Feb 26, 2020
vmarchaud
vmarchaud approved these changes Feb 26, 2020
View reviewed changes
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, looking forward to discuss about the simplification of the initialisation of the global APIs

@dyladan
Copy link
Member Author

dyladan commented Feb 26, 2020

lgtm, looking forward to discuss about the simplification of the initialisation of the global APIs

@vmarchaud me too. it is becoming quite cumbersome and I would like to have something simpler before we launch beta. I was thinking we would have some function on the tracer provider that would initialize sane defaults (e.g. NodeTracerProvider.initGlobals() would init AsyncHooksScopeManager and HttpTraceContext with the global API). This is just a rough idea though and I need to think more about the various cases we need to solve.

@codecov-io
Copy link

codecov-io commented Feb 26, 2020 •
edited
Loading

Codecov Report

Merging #816 into master will increase coverage by 0.34%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #816      +/-   ##
=========================================
+ Coverage   94.05%   94.4%   +0.34%     
=========================================
  Files         245     245              
  Lines       10567   10787     +220     
  Branches     1029    1024       -5     
=========================================
+ Hits         9939   10183     +244     
+ Misses        628     604      -24
Impacted Files Coverage Δ
packages/opentelemetry-tracing/src/config.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-tracing/src/Tracer.ts 97.91% <ø> (+19.48%) ⬆️
packages/opentelemetry-api/src/trace/NoopTracer.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-api/test/api/api.test.ts 96.66% <ø> (ø) ⬆️
...-api/test/noop-implementations/noop-tracer.test.ts 100% <ø> (+46.42%) ⬆️
...telemetry-tracing/test/BasicTracerRegistry.test.ts 100% <100%> (ø) ⬆️
...opentelemetry-node/test/NodeTracerProvider.test.ts 100% <100%> (ø) ⬆️
...kages/opentelemetry-plugin-dns/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...ages/opentelemetry-plugin-http/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...kages/opentelemetry-metrics/test/mocks/Exporter.ts 33.33% <0%> (-16.67%) ⬇️
... and 41 more

OlivierAlbertini
OlivierAlbertini approved these changes Feb 27, 2020
View reviewed changes
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.

LGTM

@codecov-io
Copy link

Codecov Report

Merging #816 into master will increase coverage by 0.34%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #816      +/-   ##
=========================================
+ Coverage   94.05%   94.4%   +0.34%     
=========================================
  Files         245     245              
  Lines       10567   10787     +220     
  Branches     1029    1024       -5     
=========================================
+ Hits         9939   10183     +244     
+ Misses        628     604      -24
Impacted Files Coverage Δ
packages/opentelemetry-tracing/src/config.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-tracing/src/Tracer.ts 97.91% <ø> (+19.48%) ⬆️
packages/opentelemetry-api/src/trace/NoopTracer.ts 100% <ø> (ø) ⬆️
packages/opentelemetry-api/test/api/api.test.ts 96.66% <ø> (ø) ⬆️
...-api/test/noop-implementations/noop-tracer.test.ts 100% <ø> (+46.42%) ⬆️
...telemetry-tracing/test/BasicTracerRegistry.test.ts 100% <100%> (ø) ⬆️
...opentelemetry-node/test/NodeTracerProvider.test.ts 100% <100%> (ø) ⬆️
...kages/opentelemetry-plugin-dns/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...ages/opentelemetry-plugin-http/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...kages/opentelemetry-metrics/test/mocks/Exporter.ts 33.33% <0%> (-16.67%) ⬇️
... and 41 more

@codecov-io
Copy link

codecov-io commented Feb 27, 2020 •
edited
Loading

Codecov Report

Merging #816 into master will increase coverage by 1.62%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##           master     #816      +/-   ##
==========================================
+ Coverage   92.69%   94.32%   +1.62%     
==========================================
  Files         241      248       +7     
  Lines       10779    10667     -112     
  Branches     1048     1031      -17     
==========================================
+ Hits         9992    10062      +70     
+ Misses        787      605     -182
Impacted Files Coverage Δ
packages/opentelemetry-resources/src/version.ts 0% <0%> (ø)
packages/opentelemetry-resources/src/constants.ts 100% <100%> (ø)
...lemetry-resources/test/util/resource-assertions.ts 100% <100%> (ø)
...ages/opentelemetry-resources/test/Resource.test.ts 100% <100%> (ø)
...lemetry-resources/test/resource-assertions.test.ts 100% <100%> (ø)
packages/opentelemetry-resources/src/Resource.ts 100% <100%> (ø)
...kages/opentelemetry-plugin-dns/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...ages/opentelemetry-plugin-http/test/utils/utils.ts 33.33% <0%> (-26.67%) ⬇️
...kages/opentelemetry-metrics/test/mocks/Exporter.ts 33.33% <0%> (-16.67%) ⬇️
...es/opentelemetry-node/src/instrumentation/utils.ts 90.47% <0%> (-9.53%) ⬇️
... and 65 more

@dyladan
Copy link
Member Author

dyladan commented Feb 27, 2020

@open-telemetry/javascript-approvers i'd like to get some more eyeballs on this if possible. It's a big change and there will be a lot of follow up work to be done before beta so I don't want it to sit for too long.

@dyladan
Copy link
Member Author

dyladan commented Feb 28, 2020

@mayurkale22 I believe I've addressed all your concerns now

mayurkale22
mayurkale22 approved these changes Feb 29, 2020
View reviewed changes
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

lgtm

@mayurkale22
Copy link
Member

I think this is good to merge.

@dyladan dyladan merged commit 3c60157 into open-telemetry:master Mar 2, 2020
@dyladan dyladan deleted the context-tracing branch March 2, 2020 19:45
@mayurkale22 mayurkale22 mentioned this pull request Mar 4, 2020
Closed
@mayurkale22 mayurkale22 mentioned this pull request Mar 11, 2020
Merged
@xirzec xirzec mentioned this pull request Mar 25, 2020
Merged
@obecny obecny mentioned this pull request Mar 26, 2020
Closed
dyladan added a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
@dyladan
feat: use context-based tracing (open-telemetry#816)
339974c 
* feat: use context-based tracing

* chore: use withSpan where possible

* chore: propagate context in tests

* chore: allow spanoptions to override parent from context
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
@rauno56
fix: use localhost for services in CI (open-telemetry#816)
f497313
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vmarchaud vmarchaud vmarchaud approved these changes

@OlivierAlbertini OlivierAlbertini OlivierAlbertini approved these changes

@mayurkale22 mayurkale22 mayurkale22 approved these changes

@bg451 bg451 Awaiting requested review from bg451

@markwolff markwolff Awaiting requested review from markwolff

@obecny obecny Awaiting requested review from obecny

@rochdev rochdev Awaiting requested review from rochdev

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
Beta
Development

Successfully merging this pull request may close these issues.

Update propagators API for OTEP 66
5 participants
@dyladan @codecov-io @mayurkale22 @vmarchaud @OlivierAlbertini