feat(tracer): separate cls from tracer #484

hekike · 2019-04-11T23:50:00Z

implements: #462

Changes

removed the argument notation interface of the tracer.startChildSpan because it brings ambiguity into span creation
Separation of CoreTracer and CoreTracerCls

Breaking Changes

startChildSpan only accepts options object
Tracer become TracerCls
Tracer is the basic non CLS version now

Action Items

~~separate tests out to test-tracer.ts and test-tracer-cls.ts~~ (done)
~~Improve non-cls tracer test coverage~~ (done)

Child of child with CLS Tracer

It's worth calling out that the CLS implementation still supports only one level of relationships as the rootSpan in the namespace context remains the same for child span creation. I guess it's expected based on current instrumentations.

A couple of further optimization ideas

turn startRootSpan sync in the non-CLS Tracer
merge startRootSpan and startChildSpan in the non CLS Tracer (startSpan)
if we merge the two and make name required, we could get closer to the OpenTracing API: https://github.com/opentracing/opentracing-javascript/blob/master/src/tracer.ts#L79 which I guess after the project convergence is prefered (but maybe this is too early and ambitious)

codecov-io · 2019-04-11T23:56:21Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #484      +/-   ##
==========================================
- Coverage   95.01%   94.99%   -0.02%     
==========================================
  Files         143      145       +2     
  Lines        9299    10222     +923     
  Branches      515      874     +359     
==========================================
+ Hits         8835     9710     +875     
- Misses        464      512      +48

Impacted Files	Coverage Δ
src/trace/model/tracer.ts	`66.66% <0%> (-19.3%)`	⬇️
src/trace/propagation/noop-propagation.ts	`40% <0%> (-10%)`	⬇️
src/http2.ts	`80.64% <0%> (-8.65%)`	⬇️
src/common/noop-logger.ts	`11.11% <0%> (-5.56%)`	⬇️
test/test-instana.ts	`85.18% <0%> (-3.28%)`	⬇️
test/test-http2.ts	`96.81% <0%> (-2.53%)`	⬇️
src/stackdriver-monitoring.ts	`78.65% <0%> (-1.91%)`	⬇️
src/trace/instrumentation/base-plugin.ts	`9.52% <0%> (-1.59%)`	⬇️
test/test-zpages.ts	`99.27% <0%> (-0.36%)`	⬇️
src/internal/string-utils.ts	`100% <0%> (ø)`	⬆️
... and 102 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 2e6709a...749fceb. Read the comment docs.

mayurkale22

Can you please update CHANGELOG.md with a breaking change line. How do you plan to expose basic tracer (with no CLS) using tracing instance?

mayurkale22 · 2019-04-12T01:49:15Z

examples/http/server.js

@@ -42,7 +42,7 @@ function startServer (port) {

 /** A function which handles requests and send response. */
 function handleRequest (request, response) {
-  const span = tracer.startChildSpan('octutorials.handleRequest');
+  const span = tracer.startChildSpan({ name: 'octutorials.handleRequest' });


I would suggest to update the examples in separate PR, WDYT?

Any update here?

mayurkale22 · 2019-04-12T01:55:56Z

packages/opencensus-core/src/trace/model/tracer-cls.ts

@@ -0,0 +1,142 @@
+/**
+ * Copyright 2018, OpenCensus Authors


Nit: s/2018/2019

mayurkale22 · 2019-04-12T01:57:40Z

packages/opencensus-core/src/trace/model/tracer-cls.ts

+ */
+
+import * as cls from '../../internal/cls';
+


Optional Nit: Remove blank line

packages/opencensus-core/src/trace/model/tracer-cls.ts

mayurkale22 · 2019-04-12T02:16:40Z

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

-        parentSpanId = options.spanContext.spanId || '';
-        traceState = options.spanContext.traceState;
+    let traceId;
+    if (options && options.spanContext && options.spanContext.traceId) {


Can we just do options = options || {name: 'span'}; at the start and remove falsiness check on options (multiple places below)?

actually, we could do just:

function (options = { name: 'span' }) { ... }

mayurkale22 · 2019-04-12T02:17:19Z

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

  /**
   * Starts a span.
-   * @param nameOrOptions Span name string or SpanOptions object.
-   * @param kind Span kind if not using SpanOptions object.
+   * @param [options] span options


Same as above

mayurkale22 · 2019-04-12T02:20:20Z

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

+   * @param childOf Span
+   * @param [name] Span name
+   * @param [kind] Span kind
+   * @param [options] Span Options


Same as above and just keep options param.

mayurkale22 · 2019-04-12T02:20:46Z

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

+  /** Get and set the currentRootSpan to tracer instance */
+  currentRootSpan: Span;
+
+


Nit: Remove extra blank line.

hekike · 2019-04-12T02:55:40Z

@mayurkale22 earlier I posted some API ideas as a comment here, I just moved them to the PR description now. Could you please share what do you think about them?

mayurkale22 · 2019-04-12T09:58:33Z

@mayurkale22 earlier I posted some API ideas as a comment here, I just moved them to the PR description now. Could you please share what do you think about them?

Thanks for the detailed description, the Changes and Action items makes sense to me. In terms of a breaking changes, could you please elaborate more on ambiguity on span creation? My initial impression was to keep default tracer as TracerCls, so this won't create any breaking change for the
current users. Also, if feasible, (one approach) we can add basicTracer to Tracing instance, which users will call to get the basic non CLS version of tracer object.

merge startRootSpan and startChildSpan in the non CLS Tracer (startSpan)

SGTM, But would love to see same for CLS Tracer (easier to maintain and switch around). WDYT?

hekike · 2019-04-12T14:40:22Z

@mayurkale22 thanks for the feedback! About the API ambiguity: The startChildSpan's original syntax is startChildSpan(name?: string, kind?: types.SpanKind), now we need to add a third childOf argument. This is problematic as all the arguments are optional today. If we add it as a first argument, the user needs to pass undefined for tracerCls.startChildSpan(undefined, 'span-name'). If we add it as the last argument you need to pass undefined when you want optional name and kind. We could build variadic arguments but in my understanding, it is against TypeScript philosophy, but maybe I'm missing something. That's why in think the OpenTracing span creation API is a good tradeoff with a required first string argument and optional secondary options: tracer.startSpan('foo', { childOf: rootSpan }).

Only keeping the object options notations looks clear and robust solution to me, due to the TypeScript types it's easy to discover the available option properties.

mayurkale22 · 2019-04-12T15:12:09Z

Only keeping the object options notations looks clear and robust solution to me, due to the TypeScript types it's easy to discover the available option properties.

Makes sense to me, please update CHANGELOG.md with a breaking change line. Thanks

kenashcraft · 2019-04-12T16:19:43Z

packages/opencensus-core/test/test-tracer.ts

-        const child1 =
-            tracer.startChildSpan('child1', types.SpanKind.UNSPECIFIED);
+        const child1 = tracer.startChildSpan(
+            {name: 'child1', kind: types.SpanKind.UNSPECIFIED});


To simplify these tests and the API, can kind just have a default value? Then we wouldn't need to specify it again and again.

kenashcraft · 2019-04-12T16:20:53Z

packages/opencensus-core/src/trace/model/tracer-cls.ts

+import * as types from './types';
+
+/**
+ * This class represent a tracer with CLS.


Explain what CLS is and when you would or would not want to use it.

hekike · 2019-04-12T17:56:28Z

@mayurkale22 @kenashcraft thanks for the review, I addressed the PR comments:

non-breaking change CoreTracer and CoreTracerBase separation
fixes in docs and typos
documenting CLS usage
updating CHANGELOG.md with startChildSpan changes
cleaning up startRootSpan defaults

mayurkale22 · 2019-04-13T09:03:27Z

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

 import * as types from './types';

 /**
- * This class represent a tracer.
+ * This class represents a tracer with Cointunues Local Storage (CLS).


Nit: s/Cointunues/Continuation/

mayurkale22 · 2019-04-13T09:12:12Z

packages/opencensus-core/src/trace/model/tracer-base.ts

+  private readonly IS_SAMPLED = 0x1;
+  /** A sampler used to make sample decisions */
+  sampler!: samplerTypes.Sampler;
+  /** A configuration for starting the tracer */


Please update this comment to something like -> An object to log information to

mayurkale22 · 2019-04-13T09:16:52Z

packages/opencensus-core/src/trace/model/tracer-base.ts

+        rootSpan.start();
+        return fn(rootSpan);
+      }
+    } else {


Can we remove this else, and update the debug msg: Tracer is inactive, starting new no record root span?

mayurkale22 · 2019-04-13T09:24:12Z

packages/opencensus-core/src/trace/model/tracer-cls.ts

+import * as types from './types';
+
+/**
+ * This class represent a tracer with CLS.


mayurkale22 · 2019-04-13T09:24:48Z

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

- * This class represent a tracer.
+ * This class represents a tracer with Cointunues Local Storage (CLS).
+ *
+ * CLS helps keep tracking the root span over function calls automatically.


Can we add -> It is capable of storing, propagating and retrieving arbitrary continuation-local data (also called "context").

mayurkale22 · 2019-04-13T09:28:58Z

packages/opencensus-core/test/test-tracer.ts

@@ -52,7 +52,7 @@ describe('Tracer', () => {
    });
  });

-  /** Should get/set the current RootSpan from tracer instance */
+  // /** Should get/set the current RootSpan from tracer instance */


Is this typo or intended stuff?

hekike · 2019-04-13T16:44:40Z

@mayurkale22 I addressed your new round of comments. Thanks!

mayurkale22 · 2019-04-14T06:05:45Z

Action Items (Will do after initial feedback)
separate tests out to test-tracer.ts and test-tracer-cls.ts
Improve non-cls tracer test coverage

@hekike Thanks for the work! What's your plan on above action items. Also, How do you plan to expose basic tracer (with no CLS) using tracing instance? OR as a user, how do I use basic tracer in my app?

hekike · 2019-04-15T17:45:53Z

@mayurkale22 I finished the open action items: separates CoreTracer and CoreTracerBase tests and added some context specific tests.

mtwo · 2019-04-29T21:49:46Z

@hekike Mayur has been on vacation for the past two weeks, but he's back next Monday. Do you need someone to approve prior to that?

hekike · 2019-04-29T23:11:11Z

It would be helpful if we could move this work forward. I'm planning to open further PRs which would require this work.

draffensperger

I think this is a reasonable refactor and so LGTM with a few nit-picky comments.

I personally like the breaking change to simplify tracer.startChildSpan, but I'm not sure how impactful that would be to library users, and I wonder if it would be possible to separate the CLS functionality of the tracer without making that change. I would leave it to @mayurkale22 to make a call here.

draffensperger · 2019-05-01T12:22:28Z

packages/opencensus-core/src/trace/model/tracer-base.ts

+  /** Indicates if the tracer is active */
+  private activeLocal: boolean;
+  /** A configuration for starting the tracer */
+  private config!: configTypes.TracerConfig;


Rather than using a ! would it be possible to assign some reasonable default config?

The default config lives in the opencensus-node today. I could move it to opencensus-core and add as a static property to this class. Then use it here as defaults and consume from here in the opencensus-node. My only concern is that this is not a change in this PR and I would prefer to keep the scope focused. But I'm happy to do this as a separate PR.

draffensperger · 2019-05-01T12:22:49Z

packages/opencensus-core/src/trace/model/tracer-base.ts

+  /** Bit to represent whether trace is sampled or not. */
+  private readonly IS_SAMPLED = 0x1;
+  /** A sampler used to make sample decisions */
+  sampler!: samplerTypes.Sampler;


Similar comment here on the sampler - can we assign some reasonable default sampler, e.g. one that samples at the default rate?

draffensperger · 2019-05-01T12:26:48Z

packages/opencensus-core/src/trace/model/tracer-base.ts

+    // if there is a context propagation, keep the decision
+    if (options && options.spanContext && options.spanContext.options) {
+      propagatedSample =
+          ((options.spanContext.options & this.IS_SAMPLED) !== 0);


Nit: can the outer parentheses around this expression be removed?

draffensperger · 2019-05-01T12:27:30Z

packages/opencensus-core/src/trace/model/tracer-base.ts

+          ((options.spanContext.options & this.IS_SAMPLED) !== 0);
+    }
+
+    let sampleDecision = !!propagatedSample;


Could this logic be simplified to return !!propagatedSample || this.sampler.shouldSample(traceId)?

BREAKING CHANGE: startChildSpan only accepts option object

…raceBase

hekike requested review from draffensperger, justindsmith and mayurkale22 as code owners April 11, 2019 23:50

googlebot added the cla: yes label Apr 11, 2019

mayurkale22 reviewed Apr 12, 2019

View reviewed changes

kenashcraft reviewed Apr 12, 2019

View reviewed changes

mayurkale22 reviewed Apr 13, 2019

View reviewed changes

draffensperger approved these changes May 1, 2019

View reviewed changes

Peter Marton added 5 commits May 1, 2019 16:16

feat(tracer): separate cls from tracer

7677bc2

BREAKING CHANGE: startChildSpan only accepts option object

refactor(tracer): keep CLS CoreTracer the default and introduce CoreT…

7a4eb5d

…raceBase

refactor(tracer): address PR comments

a6fd42f

refactor(tracer): address PR comments

abbd8b9

test(tracer): separate CoreTracerBasic and CoreTracer tests

cebd25e

hekike force-pushed the feat/tracer-cls-separation branch from f9c0569 to cebd25e Compare May 2, 2019 15:09

Peter Marton added 2 commits May 2, 2019 09:38

refactor(opencensus-core): tracer base code improvement

2f164df

refactor(opencensus-core): tracer base code improvement

749fceb

bogdandrutu merged commit 92edaa9 into census-instrumentation:master May 3, 2019

hekike mentioned this pull request May 3, 2019

Exporting TracerBase as a separate @opencensus/nodejs-base package #495

Merged

mayurkale22 added this to the Release 0.0.12 milestone May 9, 2019

This was referenced May 14, 2019

startChildSpan with SpanOptions interface only. #521

Merged

set CurrentRootSpan correctly #532

Merged

Separate CLS from Tracer and make it optional #462

Closed

		/** Get and set the currentRootSpan to tracer instance */
		currentRootSpan: Span;

feat(tracer): separate cls from tracer #484

feat(tracer): separate cls from tracer #484

Conversation

hekike commented Apr 11, 2019 • edited Loading

codecov-io commented Apr 11, 2019 • edited Loading

Codecov Report

mayurkale22 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

hekike Apr 12, 2019 • edited Loading

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

hekike commented Apr 12, 2019 • edited Loading

mayurkale22 commented Apr 12, 2019

hekike commented Apr 12, 2019 • edited Loading

mayurkale22 commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekike commented Apr 12, 2019

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

hekike commented Apr 13, 2019 • edited Loading

mayurkale22 commented Apr 14, 2019

hekike commented Apr 15, 2019

mtwo commented Apr 29, 2019

hekike commented Apr 29, 2019

draffensperger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hekike May 2, 2019 • edited Loading

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

hekike commented Apr 11, 2019 •

edited

Loading

codecov-io commented Apr 11, 2019 •

edited

Loading

hekike Apr 12, 2019 •

edited

Loading

hekike commented Apr 12, 2019 •

edited

Loading

hekike commented Apr 12, 2019 •

edited

Loading

hekike commented Apr 13, 2019 •

edited

Loading

hekike May 2, 2019 •

edited

Loading