fix: class-ify cls implementations by kjin · Pull Request #708 · googleapis/cloud-trace-nodejs

kjin · 2018-03-30T22:57:21Z

Summary

This PR refactors the CLS implementations as classes implementing a common interface. There are a number of implementations offered:

UniversalCLS treats all code as running in the same continuation.
AsyncHooksCLS uses async_hooks to propagate context within a continuation, with no semantic changes w.r.t the original implementation.
AsyncListenerCLS uses async-listener to propagate context with async-listener (a la continuation-local-storage), also with no semantic changes compared to the original implementation.
TraceCLS is a meta-implementation that wraps either of the Async* mechanisms, based on its config object. When enabled it uses either AsyncHooksCLS or AsyncListenerCLS with a default value (when there is no available context) set to { type: UNCORRELATED }. When disabled, it uses UniversalCLS with the universal value set to { type: UNTRACED }. I do not expect this to be a semantic change, as this roughly describes the current implementation today (that when the Trace Agent is disabled, all spans are UNTRACED phantom spans).

The reasons for making such a change is as follows:

The current async_hooks implementation is treated as an implementation of the continuation-local-storage Namespace interface, but we should de-couple the two implementations by instead introducing our own common interface.
More modularity. This can help us test code -- both CLS as well as the Trace Agent in isolation.
It's possible that we would need to support more than two mechanisms (or even slight differences within a single mechanism) for CLS. This paves the road towards achieving that.
- In particular, UniversalCLS may be useful in environments where CLS doesn't matter.

Change Overview

Bold denotes a new file, and ~~strikethrough~~ denotes a removed file.

src
- ~~cls.ts~~: Formerly exposed functions to manipulate a c-l-s Namespace (implementation of which is dependent on GCLOUD_TRACE_NEW_CONTEXT).
- ~~cls-ah.ts~~: Formerly an implementation of the Namespace interface from continuation-local-storage using async_hooks.
- cls.ts: Exports TraceCLS as described above.
- cls
  - base.ts: Exports the CLS interface definition.
  - async-hooks.ts: Exports an implementation of the CLS interface based on async_hooks.
  - async-listener.ts: Exports an implementation of the CLS interface based on async-listener.
  - universal.ts: Exports UniversalCLS as described above.

Reading the environmental variable GCLOUD_TRACE_NEW_CONTEXT is now done in index.ts.

codecov · 2018-04-03T21:07:50Z

Codecov Report

Merging #708 into master will decrease coverage by 0.02%.
The diff coverage is 89.93%.

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
- Coverage   90.76%   90.74%   -0.03%     
==========================================
  Files          28       30       +2     
  Lines        1495     1556      +61     
  Branches      296      304       +8     
==========================================
+ Hits         1357     1412      +55     
- Misses         59       61       +2     
- Partials       79       83       +4

Impacted Files	Coverage Δ
src/util.ts	`95.58% <100%> (+0.06%)`	⬆️
src/trace-api.ts	`94.38% <100%> (ø)`	⬆️
src/cls/async-listener.ts	`100% <100%> (ø)`
src/index.ts	`83.33% <63.63%> (-8.67%)`	⬇️
src/cls/universal.ts	`76.92% <76.92%> (ø)`
src/cls.ts	`93.47% <93.18%> (+12.52%)`	⬆️
src/cls/async-hooks.ts	`95.65% <95.65%> (ø)`

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 395a0c7...8c2198b. Read the comment docs.

ofrobots

Still going though, but left some high level comments that will probably need discussion.

Sign in to view

+    this.logger = logger;
+    const uncorrelated: RootContext = {type: SpanDataType.UNCORRELATED};
+    const untraced: RootContext = {type: SpanDataType.UNTRACED};
+    const useAH = config.mechanism === 'async-hooks' && asyncHooksAvailable;


Sign in to view

+              `propagation is not available in Node ${process.version}.`,
+              'Falling back to using async-listener.');
+        } else {
+          this.logger.warn(


Sign in to view

-    // special case where _all_ requests are explicitly not being traced,
-    // so return UNTRACED_SPAN to be consistent with that.
-    return UNTRACED_SPAN;
+  disable(): void {


ofrobots

There is more to look at, but I will leave it here for the initial round.

Sign in to view

@@ -1,5 +1,5 @@
 /**
- * Copyright 2015 Google Inc. All Rights Reserved.
+ * Copyright 2018 Google LLC


Sign in to view

+   * path in which they were attached to the EventEmitter object.
+   * @param ee The EventEmitter to bind. This instance will be mutated.
+   */
+  patchEmitterToPropagateContext<T>(ee: EventEmitter): void;


Sign in to view

+  private hook: asyncHooksModule.AsyncHook;
+  private enabled = false;
+
+  constructor(defaultContext: Context) {


Sign in to view

+ * module.
+ */
+export class AsyncHooksCLS<Context extends {}> implements CLS<Context> {
+  private current: {value: Context};


Sign in to view

+  runWithNewContext<T>(fn: Func<T>): T {
+    const oldContext = this.current.value;
+    this.current.value = this.defaultContext;
+    const res = fn();


Sign in to view

+    const contextWrapper = function(this: {}) {
+      const oldContext = current.value;
+      current.value = boundContext;
+      const res = fn.apply(this, arguments) as T;


Sign in to view

+
+const EVENT_EMITTER_METHODS: Array<keyof EventEmitter> =
+    ['addListener', 'on', 'once', 'prependListener', 'prependOnceListener'];
+const WRAPPED = Symbol('context_wrapped');


Sign in to view

+      }
+    });
+  }
+}


Sign in to view

+ *
+ * CLS stands for continuation-local storage.
+ */
+export interface CLS<Context extends {}> {


Sign in to view

+  /**
+   * Enables this instance.
+   */
+  enable(): void;


Sign in to view

+export class TraceCLS implements CLS<RootContext> {
+  private currentCLS: CLS<RootContext>;
+  // CLSClass is a constructor.
+  // tslint:disable-next-line:variable-name


Sign in to view

+   */
+  readonly rootSpanStackOffset: number;
+
+  constructor(logger: Logger, config: TraceCLSConfig) {


Sign in to view

-  return cls.createNamespace(TRACE_NAMESPACE);
-}
+  isEnabled(): boolean {
+    return this.enabled;


Sign in to view

+const EVENT_EMITTER_METHODS: Array<keyof EventEmitter> =
+    ['addListener', 'on', 'once', 'prependListener', 'prependOnceListener'];
+// A symbol used to check if a method has been wrapped for context.
+const WRAPPED = Symbol('@google-cloud/trace-agent::AsyncHooksCLS::WRAPPED');


Sign in to view

 import * as semver from 'semver';

+import {AsyncHooksCLS} from './cls/async-hooks';
+import {AsyncListenerCLS} from './cls/async-listener';


Sign in to view

+  }
+
+  patchEmitterToPropagateContext(ee: EventEmitter): void {}
+}


Sign in to view

+export class UniversalCLS<Context> implements CLS<Context> {
+  private enabled = false;
+
+  constructor(private defaultContext: Context) {}


Sign in to view

+ * limitations under the License.
+ */
+
+import * as clsModule from 'continuation-local-storage';


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

kjin force-pushed the cls-new-2 branch 9 times, most recently from b8c45f5 to 57ca019 Compare April 3, 2018 20:49

kjin requested review from DominicKramer, jinwoo and ofrobots April 3, 2018 21:06

kjin force-pushed the cls-new-2 branch from 57ca019 to 160171b Compare April 3, 2018 21:08

kjin requested review from DominicKramer, jinwoo and ofrobots and removed request for DominicKramer, jinwoo and ofrobots April 12, 2018 17:24

ofrobots reviewed Apr 12, 2018

View reviewed changes

kjin force-pushed the cls-new-2 branch 3 times, most recently from 52e7e65 to 03bc2df Compare April 13, 2018 18:22

ofrobots reviewed Apr 16, 2018

View reviewed changes

kjin changed the title ~~refactor: class-ify cls implementations~~ fix: class-ify cls implementations Apr 17, 2018

kjin force-pushed the cls-new-2 branch from 4c43232 to 42bd1d6 Compare April 17, 2018 22:52

refactor: rewrite cls implementation

ab56937

kjin added 5 commits April 17, 2018 15:53

refactor: address comments, pt 0

120c090

refactor: address comments, pt 1

45d97f9

test: adjust tests

4a30bb8

refactor: address comments, pt 2

7f33294

fix: when an error is thrown during init, disable

f1ba346

kjin force-pushed the cls-new-2 branch from 42bd1d6 to f1ba346 Compare April 17, 2018 22:54

-___________________-

95c2290

ofrobots approved these changes Apr 18, 2018

View reviewed changes

Comment thread src/cls/async-listener.ts

* limitations under the License.

*/

import * as clsModule from 'continuation-local-storage';

This comment was marked as spam.

Sign in to view

refactor: address comments, pt 3

8c2198b

kjin merged commit 132db9b into googleapis:master Apr 19, 2018

kjin mentioned this pull request Apr 19, 2018

fix: restore context when a function run with a given context throws #727

Merged

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

chore(main): release 7.0.0 #1452

Merged

+                    }
+                  });
+                }
+              }

                
                    No newline at end of file

Conversation

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

Uh oh!

Summary

Change Overview

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

ofrobots left a comment

Choose a reason for hiding this comment

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!

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!

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 Mar 30, 2018 •

edited

Loading

codecov Bot commented Apr 3, 2018 •

edited

Loading