Consolidating Span and RootSpan to allow Spans to recursively have children #441

kenashcraft · 2019-03-22T16:25:41Z

Consolidate the functionality of Span and RootSpan to allow Spans to recursively have children.

Moved the functionality from Span into BaseSpan and then renamed the files. Unfortunately, that makes for a pretty terrible diff in Github. Try pulling from my fork and doing git diff master:packages/opencensus-core/src/trace/model/span-base.ts span-refactor:packages/opencensus-core/src/trace/model/span.ts
Ditto for no-record-span.ts
I had to make more extensive changes to tracez.page-handler.ts. Please pay attention to that one.
I added some tests to test-root-span.ts. Please advise whether that is the appropriate place for those tests (vs test-span.ts) and whether you can think of additional tests.
I have not updated all of the exporters to use allDescendants() yet. They still get just the root's immediate children. I think we can do that in a subsequent PR.

draffensperger

I think this is a good step in the right direction. I would also want Tracer to support trace contexts that aren't root spans, and maybe even to collapse the distinction between Span and RootSpan.

But doing this step-by-step makes sense to me, and I think this general approach is a decent first step.

@mayurkale22 what do you think?

packages/opencensus-core/src/trace/model/span.ts

codecov-io · 2019-04-08T21:30:51Z

Codecov Report

Merging #441 into master will increase coverage by 0.4%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #441     +/-   ##
=========================================
+ Coverage   94.34%   94.75%   +0.4%     
=========================================
  Files         150      148      -2     
  Lines        8884     9940   +1056     
  Branches      731      750     +19     
=========================================
+ Hits         8382     9419   +1037     
- Misses        502      521     +19

Impacted Files	Coverage Δ
src/trace/propagation/noop-propagation.ts	`50% <0%> (-50%)`	⬇️
src/trace/model/no-record/no-record-root-span.ts	`45.83% <0%> (-10.05%)`	⬇️
src/internal/cls.ts	`91.66% <0%> (-8.34%)`	⬇️
src/tags/propagation/text-format.ts	`95% <0%> (-5%)`	⬇️
src/tags/propagation/variant-encoding.ts	`95.83% <0%> (-4.17%)`	⬇️
src/trace/model/span.ts	`97.6% <0%> (-2.41%)`	⬇️
src/trace/model/root-span.ts	`96% <0%> (-1.96%)`	⬇️
src/tags/propagation/binary-serializer.ts	`95.23% <0%> (-0.92%)`	⬇️
test/test-ioredis.ts	`96.96% <0%> (-0.09%)`	⬇️
test/test-zpages.ts	`99.62% <0%> (-0.01%)`	⬇️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbc618b...d5895e9. Read the comment docs.

kenashcraft · 2019-04-08T21:33:18Z

@mayurkale22 @draffensperger Please take a look. PR description updated. All tests are passing. I'd like to get this merged quickly since it touches so many files that keeping it up to date is a challenge.

Thanks to @hekike for helping me keep this PR up to date!

googlebot · 2019-04-08T22:44:29Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

hekike · 2019-04-08T22:47:31Z

I signed the CLA in the meantime.

mayurkale22

First pass. Looks great!

Nit: Should we rename rootSpan -> span & rootSpans -> spans variable names in seperate PR?

packages/opencensus-core/src/trace/model/no-record/no-record-root-span.ts

packages/opencensus-core/src/trace/model/span.ts

draffensperger

LGTM pending a few minor comments.

I do support getting this in given the number of files it touches and think that further cleanup / improvement should be in follow up PRs.

draffensperger · 2019-04-09T12:57:09Z

packages/opencensus-core/src/trace/model/no-record/no-record-span.ts

+  /** Gets the ID of the parent span. */
+  get parentSpanId(): string {
+    if (!this.parentSpan) {
+      return 'no-parent';


Can this just return the empty string ('') which means "no parent" in other context?

draffensperger · 2019-04-09T12:59:22Z

packages/opencensus-core/src/trace/model/span.ts

+
+  /** Recursively gets the descendant spans. */
+  allDescendants(): types.Span[] {
+    console.log('Calling allDescendants', this.name);


Can this and the other log lines below be removed?

Removed. Sorry about that.

draffensperger · 2019-04-09T13:01:22Z

packages/opencensus-core/src/trace/model/span.ts

      traceId: this.traceId,
      spanId: this.id,
-      name: this.name,
+      options: 0x1,  // always traced


I'm assuming this was adapted from SpanBase - will it cause all spans to be sampled? @mayurkale22

Hmm... I didn't touch this line, just moved it.

I'm assuming this was adapted from SpanBase - will it cause all spans to be sampled?

No. The Sampling Decision happens at the top level (when we create the root span using startRootSpan). Depending on the decision, it creates either RootSpan (which has options set to 0x1) or NoRecordRootSpan (options set to 0x0). Does it make sense?

draffensperger · 2019-04-09T13:04:24Z

packages/opencensus-exporter-zpages/src/zpages-frontend/page-handlers/tracez.page-handler.ts

-                                            duration: childSpan.duration
-                                          }));
-  }
+  // tslint:disable-next-line:no-any


Since this doesn't use any below, can you remove this tslint disable comment?

Squashed commits: [a4f2234] Get the test passing [8276c98] Get allDescendants() working [379b2eb] Big refactor, tests are mostly passing [6d93312] Lerna handles cross-package dependencies [3567e2e] Rough draft of making Span and RootSpan almost identical [eba18f3] Set 'shared' based on the type of span [b56a884] Reformat tests [b1b6dfb] Use parentSpanId if provided [9d4e601] Tests that ensure nested spans are properly translated for Zipkin [585034e] Add tests for nested spans.

mayurkale22 · 2019-04-09T17:45:02Z

Ping @googlebot

googlebot · 2019-04-09T18:05:27Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

packages/opencensus-exporter-zpages/src/zpages-frontend/page-handlers/tracez.page-handler.ts

packages/opencensus-core/test/test-root-span.ts

googlebot · 2019-04-09T21:09:39Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-04-11T02:33:47Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

mayurkale22

LGTM, thanks for the contribution.

kenashcraft requested review from draffensperger, justindsmith and mayurkale22 as code owners March 22, 2019 16:25

googlebot added the cla: yes label Mar 22, 2019

kenashcraft mentioned this pull request Mar 22, 2019

Add support for nested spans #437

Closed

draffensperger reviewed Mar 22, 2019

View reviewed changes

packages/opencensus-core/src/trace/model/span.ts Show resolved Hide resolved

hekike mentioned this pull request Apr 2, 2019

Separate CLS from Tracer and make it optional #462

Closed

kenashcraft changed the title ~~Initial sketch of consolidating Span and RootSpan~~ Consolidating Span and RootSpan to allow Spans to recursively have children Apr 8, 2019

googlebot added cla: no and removed cla: yes labels Apr 8, 2019

mayurkale22 reviewed Apr 9, 2019

View reviewed changes

draffensperger approved these changes Apr 9, 2019

View reviewed changes

Ken Ashcraft and others added 5 commits April 9, 2019 09:20

Add these files back

8451af2

Undo spurious package-lock.json changes

ce33f52

Address PR comments

4fe5bb9

test(pluginloader): re-enable tests

9a9f92b

mayurkale22 added cla: yes and removed cla: no labels Apr 9, 2019

mayurkale22 reviewed Apr 9, 2019

View reviewed changes

Add tracer tests, clean ups to tracez.page-handler.ts

3154378

googlebot added cla: no and removed cla: yes labels Apr 9, 2019

Allow grandchildren via tracer.startChildSpan()

d5895e9

mayurkale22 added cla: yes and removed cla: no labels Apr 11, 2019

mayurkale22 approved these changes Apr 11, 2019

View reviewed changes

mayurkale22 merged commit abd41db into census-instrumentation:master Apr 11, 2019

mayurkale22 mentioned this pull request May 21, 2019

HTTPS plugin results in duplicate spans #542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidating Span and RootSpan to allow Spans to recursively have children #441

Consolidating Span and RootSpan to allow Spans to recursively have children #441

kenashcraft commented Mar 22, 2019 •

edited

Loading

draffensperger left a comment

codecov-io commented Apr 8, 2019 •

edited

Loading

kenashcraft commented Apr 8, 2019 •

edited

Loading

googlebot commented Apr 8, 2019

hekike commented Apr 8, 2019

mayurkale22 left a comment

draffensperger left a comment

draffensperger Apr 9, 2019

kenashcraft Apr 9, 2019

draffensperger Apr 9, 2019

kenashcraft Apr 9, 2019

draffensperger Apr 9, 2019

kenashcraft Apr 9, 2019

mayurkale22 Apr 9, 2019

draffensperger Apr 9, 2019

kenashcraft Apr 9, 2019

mayurkale22 commented Apr 9, 2019

googlebot commented Apr 9, 2019

googlebot commented Apr 9, 2019

googlebot commented Apr 11, 2019

mayurkale22 left a comment

Consolidating Span and RootSpan to allow Spans to recursively have children #441

Consolidating Span and RootSpan to allow Spans to recursively have children #441

Conversation

kenashcraft commented Mar 22, 2019 • edited Loading

draffensperger left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 8, 2019 • edited Loading

Codecov Report

kenashcraft commented Apr 8, 2019 • edited Loading

googlebot commented Apr 8, 2019

hekike commented Apr 8, 2019

mayurkale22 left a comment

Choose a reason for hiding this comment

draffensperger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurkale22 commented Apr 9, 2019

googlebot commented Apr 9, 2019

googlebot commented Apr 9, 2019

googlebot commented Apr 11, 2019

mayurkale22 left a comment

Choose a reason for hiding this comment

kenashcraft commented Mar 22, 2019 •

edited

Loading

codecov-io commented Apr 8, 2019 •

edited

Loading

kenashcraft commented Apr 8, 2019 •

edited

Loading