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: start a root span with spanOptions.parent = null #889

Merged
merged 5 commits into from Mar 26, 2020

Conversation

@dyladan
Copy link
Contributor

dyladan commented Mar 23, 2020

Fixes #886

@dyladan dyladan force-pushed the dynatrace-oss-contrib:parent-null branch from ee1776c to bd16a99 Mar 23, 2020
Copy link
Member

mayurkale22 left a comment

lgtm

packages/opentelemetry-tracing/src/Tracer.ts Outdated Show resolved Hide resolved
@Flarna

This comment has been minimized.

Copy link
Contributor

Flarna commented Mar 23, 2020

The extractedSpanContext is kept on context in this case and may jump in later, Is this expected or should we somehow remove it there?

@dyladan

This comment has been minimized.

Copy link
Contributor Author

dyladan commented Mar 23, 2020

The extractedSpanContext is kept on context in this case and may jump in later, Is this expected or should we somehow remove it there?

If I create a root span A, then a second root span B, if I create a span C without first setting B as the active span or specifying the C parentage in span options, I think it is reasonable to assume that span A would become its parent. Same holds true if A is not an active span but is just the extracted span context.

In other words, I think this is ok for now. If users want to clear context for some advanced use-case, there exist methods to do that. In the most common case, the span you want to be the parent will be set as the active span anyways.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 23, 2020

Codecov Report

Merging #889 into master will decrease coverage by 27.63%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #889       +/-   ##
===========================================
- Coverage   94.00%   66.36%   -27.64%     
===========================================
  Files         247       81      -166     
  Lines       10717     1995     -8722     
  Branches     1040      296      -744     
===========================================
- Hits        10074     1324     -8750     
- Misses        643      671       +28     
Impacted Files Coverage Δ
packages/opentelemetry-core/test/utils/url.test.ts 0.00% <0.00%> (-100.00%) ⬇️
...ackages/opentelemetry-core/src/platform/node/id.ts 0.00% <0.00%> (-100.00%) ⬇️
...ckages/opentelemetry-core/test/platform/id.test.ts 0.00% <0.00%> (-100.00%) ⬇️
...s/opentelemetry-core/test/trace/tracestate.test.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/test/internal/validators.test.ts 0.00% <0.00%> (-100.00%) ⬇️
...entelemetry-core/test/common/ConsoleLogger.test.ts 0.00% <0.00%> (-100.00%) ⬇️
...entelemetry-core/test/context/B3Propagator.test.ts 0.00% <0.00%> (-100.00%) ⬇️
...ntelemetry-core/src/platform/node/hex-to-base64.ts 0.00% <0.00%> (-100.00%) ⬇️
...ntelemetry-core/test/trace/NoRecordingSpan.test.ts 0.00% <0.00%> (-100.00%) ⬇️
... and 183 more
@mayurkale22

This comment has been minimized.

Copy link
Member

mayurkale22 commented Mar 25, 2020

@open-telemetry/javascript-approvers we need more reviews on this, please take a look when you get a minute.

@mayurkale22 mayurkale22 merged commit 9e114d1 into open-telemetry:master Mar 26, 2020
2 checks passed
2 checks passed
build Workflow: build
Details
cla/linuxfoundation dyladan authorized
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.