feat: ensure spans are non-null by kjin · Pull Request #680 · googleapis/cloud-trace-nodejs

kjin · 2018-03-02T00:04:32Z

Fixes #656

This PR changes the public API so that spans are never null (in runInRootSpan or createChildSpan). Instead, they now expose a type field which represents whether they are real or "virtual spans". The code docs for SpanDataType in this file should explain the purpose of type.

Open questions:

Is this semver-major?
Does SpanData#type need to have distinct ROOT and CHILD values?

Example

import * as trace from '.';
import { createReadStream } from 'fs';

const traceApi = trace.start();

// original
{
  function doWork(onChunk) {
    const stream = createReadStream('large-file', 'utf8');
    traceApi.runInRootSpan({ name: 'outer' }, rootSpan => {
      if (rootSpan) {
        // only attempt to propagate context if root span isn't null
        traceApi.wrapEmitter(stream);
      }
      let count = 0;
      stream.on('data', onChunk);
      stream.on('end', () => {
        // rootSpan could be null
        if (rootSpan) {
          rootSpan.endSpan();
        }
      });
    });
  }

  doWork((data) => {
    const childSpan = traceApi.createChildSpan({ name: `inner-${count++}` });
    // childSpan could be null
    if (childSpan) {
      childSpan.addLabel('data-length', data.length);
      childSpan.endSpan();
    } else {
      // Why not? Unclear
    }
  });
}

// changed
{
  function doWork(onChunk) {
    const stream = createReadStream('large-file', 'utf8');
    traceApi.runInRootSpan({ name: 'outer' }, rootSpan => {
      if (rootSpan.type === traceApi.spanTypes.ROOT) {
        // only attempt to propagate context if this span is really being traced
        traceApi.wrapEmitter(stream);
      }
      let count = 0;
      stream.on('data', onChunk);
      stream.on('end', () => {
        rootSpan.endSpan();
      });
    });
  }

  doWork((data) => {
    const childSpan = traceApi.createChildSpan({ name: `inner-${count++}` });
    childSpan.addLabel('data-length', data.length);
    childSpan.endSpan();
    // Log if context was lost somehow
    if (childSpan.type === traceApi.spanTypes.UNCORRELATED) {
      console.log('we must have lost context');
    }
  });
}

ofrobots · 2018-03-02T19:09:27Z

This would be semver major if existing user code must be changed in order to continue working. I haven't reviewed due to the DNR warning at the top, but my suspicion is that this is not semver major.

kjin · 2018-03-02T19:23:03Z

@ofrobots I'll add an example of how it changed in the abstract shortly. I also think it's not, but wanted to make sure everyone was on the same page.

codecov · 2018-03-06T19:44:22Z

Codecov Report

Merging #680 into master will decrease coverage by 0.01%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master     #680      +/-   ##
==========================================
- Coverage      90%   89.99%   -0.02%     
==========================================
  Files          30       29       -1     
  Lines        1411     1419       +8     
  Branches      289      288       -1     
==========================================
+ Hits         1270     1277       +7     
- Misses         56       57       +1     
  Partials       85       85

Impacted Files	Coverage Δ
src/plugins/plugin-http2.ts	`85.45% <0%> (ø)`	⬆️
src/plugins/plugin-redis.ts	`91.66% <100%> (-0.2%)`	⬇️
src/plugins/plugin-restify.ts	`96.96% <100%> (ø)`	⬆️
src/plugins/plugin-connect.ts	`91.89% <100%> (+0.22%)`	⬆️
src/constants.ts	`100% <100%> (ø)`	⬆️
src/plugins/plugin-http.ts	`90.24% <100%> (ø)`	⬆️
src/trace.ts	`100% <100%> (ø)`	⬆️
src/plugins/plugin-mysql2.ts	`83.78% <100%> (ø)`	⬆️
src/plugins/plugin-koa.ts	`88.13% <100%> (+0.2%)`	⬆️
src/trace-writer.ts	`87.69% <100%> (+0.87%)`	⬆️
... and 9 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 6b8a82b...3803fa9. Read the comment docs.

Sign in to view

+
+// This is only used in tests (and is temporary), so it doesn't apply in the
+// comment in getRootContext about the possible values of namespace.get('root').
+// It's functionally identical to setRootContext(null).


Sign in to view

      // The span name will be of form "grpc:/[Service]/[MethodName]".
      var span = api.createChildSpan({ name: 'grpc:' + method.path });
-      if (!span) {
+      if (span.type !== api.spanTypes.CHILD) {


Sign in to view

-        traceUtil.truncate(spanName, Constants.TRACE_SERVICE_SPAN_NAME_LIMIT);
-    this.span = new TraceSpan(spanName, randomSpanId(), parentSpanId);
-    trace.spans.push(this.span);
+      trace: Trace, spanName: string, parentSpanId: string,


Sign in to view

+  readonly span: TraceSpan;
+  readonly trace: Trace;
+  readonly type: SpanDataType.ROOT;
+}|{


Sign in to view

+
+// This is only used in tests (and is temporary), so it doesn't apply in the
+// comment in getRootContext about the possible values of namespace.get('root').
+// It's functionally identical to setRootContext(null).


Sign in to view

  endSpan(): void;
 }

+// export type SpanData = RootSpanData|ChildSpanData;


Sign in to view

   * @returns The return value of calling fn.
   */
-  runInRootSpan<T>(options: RootSpanOptions, fn: (span: SpanData|null) => T): T;
+  runInRootSpan<T>(options: RootSpanOptions, fn: (span: SpanData) => T): T;


Sign in to view

-        traceUtil.truncate(spanName, Constants.TRACE_SERVICE_SPAN_NAME_LIMIT);
-    this.span = new TraceSpan(spanName, randomSpanId(), parentSpanId);
-    trace.spans.push(this.span);
+      trace: Trace, spanName: string, parentSpanId: string,


Sign in to view

+        // tslint:disable-next-line:no-any
+        addLabel(key: string, value: any) {},
+        endSpan() {}
+      } as SpanData & {readonly type: T});


Sign in to view

+}
+
+// Helper function to generate static virtual trace spans.
+const createDummySpanData =


Sign in to view

      // The span name will be of form "grpc:/[Service]/[MethodName]".
      var span = api.createChildSpan({ name: 'grpc:' + method.path });
-      if (!span) {
+      if (span.type !== api.spanTypes.CHILD) {


ofrobots · 2018-03-10T01:40:05Z

Note the example you provide in OP

      if (rootSpan.type === traceApi.spanTypes.ROOT) {
        // only attempt to propagate context if this span is really being traced
        traceApi.wrapEmitter(stream);
      }

The test should be self-documenting. Right now you needed a comment to explain what the test is achieving.

kjin · 2018-03-14T23:11:21Z

@ofrobots PTAL

Sign in to view

 import {Trace, TraceSpan} from './trace';

+export type RealRootContext = {
+  readonly span: TraceSpan; readonly trace: Trace; readonly type: SpanDataType


Sign in to view

  name: string;
  spanId: string;
-  parentSpanId: string;
+  parentSpanId?: string;


ofrobots

LGTM

kjin · 2018-03-16T21:36:19Z

Node 4 CI will pass in ~10 min (tests passed, but SSH connection is open)

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 2, 2018

kjin force-pushed the new-span-data branch from 6e5ab84 to 2064af9 Compare March 6, 2018 19:44

kjin force-pushed the new-span-data branch 2 times, most recently from 8d50449 to b23273e Compare March 6, 2018 21:34

ofrobots reviewed Mar 6, 2018

View reviewed changes

kjin force-pushed the new-span-data branch from b23273e to b87a1c0 Compare March 6, 2018 22:06

kjin requested a review from a team March 6, 2018 23:09

kjin force-pushed the new-span-data branch from f90cb2e to 5f09ad9 Compare March 9, 2018 23:47

ofrobots reviewed Mar 10, 2018

View reviewed changes

kjin force-pushed the new-span-data branch 4 times, most recently from 8e885e7 to d4f2bfd Compare March 13, 2018 19:51

ofrobots reviewed Mar 14, 2018

View reviewed changes

kjin force-pushed the new-span-data branch from d4f2bfd to 9d7df16 Compare March 15, 2018 00:21

ofrobots approved these changes Mar 16, 2018

View reviewed changes

kjin added 7 commits March 16, 2018 14:19

chore: upgrade typescript version

738439f

feat: ensure spans are non-null

dc6f6b6

refactor: fix up plugins

27a3cf5

refactor: fix up tests

ebb6303

doc: update docs

de3f8ae

address comments

93bb96d

address comments

3803fa9

kjin force-pushed the new-span-data branch from 9d7df16 to 3803fa9 Compare March 16, 2018 21:19

kjin merged commit 49b2118 into googleapis:master Mar 16, 2018

kjin mentioned this pull request Apr 2, 2018

fix: treat instanceId metadata as a number #713

Merged

kjin mentioned this pull request May 14, 2018

refactor: address TODOs related to spans #749

Merged

Conversation

kjin commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Uh oh!

ofrobots commented Mar 2, 2018

Uh oh!

kjin commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

ofrobots commented Mar 10, 2018

Uh oh!

kjin commented Mar 14, 2018

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

kjin commented Mar 16, 2018

Uh oh!

Reviewers

kjin commented Mar 2, 2018 •

edited

Loading

kjin commented Mar 2, 2018 •

edited

Loading

codecov Bot commented Mar 6, 2018 •

edited

Loading