feat: allow "disabling" cls, and relax requirements for creating root spans by kjin · Pull Request #728 · googleapis/cloud-trace-nodejs

kjin · 2018-04-19T16:26:37Z

Fixes #719

This PR introduces a new config option clsMechanism that can either be set to 'none' or 'auto' (and defaults to the latter, preserving the current behavior). Setting it to 'none' effectively treats the entire application as sharing the same root context, so any code running subsequent to runInRootSpan(spanConfig, (rootSpan) => { /*...*/ }) will see all child spans created under that root until rootSpan.endSpan() is called.

The 'none' CLS mechanism is at odds with the previous limitation that there may only be one root span per context, so this PR also changes behavior so that when a root span ends, context is cleared. As a result, this code path should now never be executed, so I have removed it.

The new semantics for creating root spans is as follows:

If there is no root context in the current continuation, create a new root span, and a corresponding root context in the current continuation.
If there is a root context in the current continuation, do not create a new root span.
When a root span is ended, clear the root context in the current continuation. (This is the new change)

Note that the "current continuation" in the first and third bullet points may not be the same continuation, even if the root spans mentioned are the same. It is still OK to clear the root context in the current continuation (as stated in the third bullet point) because there was none to begin with.

2

codecov · 2018-04-19T21:45:17Z

Codecov Report

Merging #728 into master will increase coverage by 0.17%.
The diff coverage is 89.65%.

@@            Coverage Diff             @@
##           master     #728      +/-   ##
==========================================
+ Coverage   90.74%   90.92%   +0.17%     
==========================================
  Files          30       30              
  Lines        1556     1564       +8     
  Branches      304      302       -2     
==========================================
+ Hits         1412     1422      +10     
+ Misses         61       59       -2     
  Partials       83       83

Impacted Files	Coverage Δ
src/cls/async-listener.ts	`100% <ø> (ø)`	⬆️
src/config.ts	`100% <ø> (ø)`	⬆️
src/cls/universal.ts	`92.3% <100%> (+15.38%)`	⬆️
src/trace-api.ts	`94.44% <100%> (+0.06%)`	⬆️
src/index.ts	`83.72% <83.33%> (+0.38%)`	⬆️
src/cls.ts	`94.23% <90%> (+0.75%)`	⬆️

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 edb8135...035b045. Read the comment docs.

ofrobots · 2018-04-19T22:21:04Z

Is RootSpan synonymous with a Context now?

Sign in to view

+        if (asyncHooksAvailable) {
+          this.CLSClass = AsyncHooksCLS;
+          this.rootSpanStackOffset = 4;
+          break;


Sign in to view

  }

+  clearContext(): void {
+    if (this.getContext() !== this.defaultContext) {


Sign in to view

 /** Available configuration options. */
 export interface Config {
+  /**
+   * The context propagation mechanism to use. The following options are


Sign in to view

 * user-provided value will be used to extend the default value.
 */
 export const defaultConfig = {
+  clsMechanism: 'auto' as ContextPropagationMechanism,


Sign in to view

    googleGax = require('./fixtures/google-gax0.16');
  });

+  after(() => {


Sign in to view

-          this.logger.error(
-              'TraceCLS#constructor: The specified CLS mechanism',
-              `[${config.mechanism}] was not recognized.`);
+          throw new Error(`CLS mechanism [${config.mechanism}] is invalid.`);


Sign in to view

+      default:
+        this.logger.error(
+            'TraceCLS#constructor: The specified CLS mechanism',
+            `[${config.mechanism}] was not recognized.`);


Sign in to view

  runWithNewContext<T>(fn: Func<T>): T {
    return this.getNamespace().runAndReturn(() => {
-      this.setContext(this.defaultContext);
+      this.clearContext();


Sign in to view

 * user-provided value will be used to extend the default value.
 */
 export const defaultConfig = {
+  clsMechanism: 'auto' as ContextPropagationMechanism,


Sign in to view

 interface TopLevelConfig {
  enabled: boolean;
  logLevel: number;
+  clsMechanism: 'none'|'auto';


Sign in to view

-        .enable();
+    const m = config.clsMechanism;
+    const clsConfig: Forceable<TraceCLSConfig> = {
+      mechanism: m === 'auto' ? (useAH ? 'async-hooks' : 'async-listener') : m,


Sign in to view

-            options.name}] because root span [${
-            rootSpan.span.name}] was already closed.`);
-        return UNCORRELATED_SPAN;
-      }


kjin · 2018-04-20T18:01:38Z

Summary of changes in commit order:

I added the TraceCLSMechanism enum. It is not user-facing.
Various other changes to address comments.
Instead of clearing context, we instead simply explicitly allow root spans to be created when a root span context already exists, but that root span is ended, while still warning if child spans are created under those same conditions. This means that clearContext is now unnecessary and has been removed.

Sign in to view

+ * An enumeration of the possible mechanisms for supporting context propagation
+ * through continuation-local storage.
+ */
+export enum TraceCLSMechanism {


jinwoo

Chatted offline. Using a string enum sounds good to me when their values may come directly from user configs in the future.

LGTM once CI passes.

ofrobots

lgtm w/ nit.

Sign in to view

 import * as crypto from 'crypto';
 import * as util from 'util';

+import {cls} from './cls';


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

kjin added 2 commits April 19, 2018 09:28

feat: allow none cls as an option

630e85e

test: amend tests to allow none CLS

f2fd261

kjin force-pushed the cls-new-4 branch 2 times, most recently from 248e332 to 6693f8a Compare April 19, 2018 18:26

kjin added 3 commits April 19, 2018 13:29

feat: allow sequential root spans

8d1198c

test: add test for sequential root spans

650853d

test: check cls config

11697e5

2

kjin force-pushed the cls-new-4 branch from 6693f8a to 11697e5 Compare April 19, 2018 21:45

kjin requested review from DominicKramer, jinwoo and ofrobots April 19, 2018 22:04

jinwoo reviewed Apr 19, 2018

View reviewed changes

ofrobots reviewed Apr 19, 2018

View reviewed changes

kjin added 2 commits April 20, 2018 10:51

refactor: use enums

0b305ab

refactor: address comments, pt 0

07fe3b1

kjin force-pushed the cls-new-4 branch from cdd12b7 to ce016ee Compare April 20, 2018 17:56

fix: add back warning for confused context

273e6ad

kjin force-pushed the cls-new-4 branch from ce016ee to 273e6ad Compare April 20, 2018 18:03

jinwoo reviewed Apr 20, 2018

View reviewed changes

refactor: small change to stop using string literals

5110ddd

jinwoo approved these changes Apr 20, 2018

View reviewed changes

chore: fix mysql docker image at version 5

c7f46cb

ofrobots approved these changes Apr 25, 2018

View reviewed changes

Comment thread src/span-data.ts Outdated

import * as crypto from 'crypto';

import * as util from 'util';

import {cls} from './cls';

This comment was marked as spam.

Sign in to view

refactor: remove unused import

035b045

kjin merged commit 5d000e9 into googleapis:master Apr 25, 2018

tjmehta mentioned this pull request Apr 25, 2018

Option to disable automatic tracing #719

Closed

kjin mentioned this pull request Apr 27, 2018

feat: add rootSpan.createChildSpan and change none CLS semantics #731

Merged

release-please Bot mentioned this pull request Jun 30, 2022

chore(main): release 7.0.0 #1452

Merged

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ofrobots commented Apr 19, 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!

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!

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

Uh oh!

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!

jinwoo left a comment

Choose a reason for hiding this comment

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kjin commented Apr 19, 2018 •

edited

Loading

codecov Bot commented Apr 19, 2018 •

edited

Loading

kjin commented Apr 20, 2018 •

edited

Loading