chore: use unique symbols to identify context values

open-telemetry · Feb 12, 2020 · 0a7998e · 0a7998e
1 parent 2987a59
commit 0a7998e
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 59 deletions.
diff --git a/packages/opentelemetry-core/src/context/context.ts b/packages/opentelemetry-core/src/context/context.ts
@@ -17,8 +17,8 @@
 import { Span, SpanContext } from '@opentelemetry/api';
 import { Context } from '@opentelemetry/scope-base';
 
-const ACTIVE_SPAN_KEY = 'ACTIVE_SPAN';
-const EXTRACTED_SPAN_CONTEXT_KEY = 'EXTRACTED_SPAN_CONTEXT';
+const ACTIVE_SPAN_KEY = Context.getKey('OpenTelemetry Context Key ACTIVE_SPAN');
+const EXTRACTED_SPAN_CONTEXT_KEY = Context.getKey('OpenTelemetry Context Key EXTRACTED_SPAN_CONTEXT');
 
 /**
  * Return the active span if one exists

diff --git a/packages/opentelemetry-scope-async-hooks/test/AsyncHooksScopeManager.test.ts b/packages/opentelemetry-scope-async-hooks/test/AsyncHooksScopeManager.test.ts
@@ -21,6 +21,7 @@ import { Context } from '@opentelemetry/scope-base';
 
 describe('AsyncHooksScopeManager', () => {
   let scopeManager: AsyncHooksScopeManager;
+  const key1 = Context.getKey('test key 1');
 
   beforeEach(() => {
     scopeManager = new AsyncHooksScopeManager();
@@ -55,7 +56,7 @@ describe('AsyncHooksScopeManager', () => {
     });
 
     it('should run the callback (object as target)', done => {
-      const test = Context.ROOT_CONTEXT.setValue('a', 1);
+      const test = Context.ROOT_CONTEXT.setValue(key1, 1);
       scopeManager.with(test, () => {
         assert.strictEqual(scopeManager.active(), test, 'should have scope');
         return done();
@@ -80,8 +81,8 @@ describe('AsyncHooksScopeManager', () => {
     });
 
     it('should finally restore an old scope', done => {
-      const scope1 = Context.ROOT_CONTEXT.setValue('name', 'scope1');
-      const scope2 = Context.ROOT_CONTEXT.setValue('name', 'scope2');
+      const scope1 = Context.ROOT_CONTEXT.setValue(key1, 'scope1');
+      const scope2 = Context.ROOT_CONTEXT.setValue(key1, 'scope2');
       scopeManager.with(scope1, () => {
         assert.strictEqual(scopeManager.active(), scope1);
         scopeManager.with(scope2, () => {
@@ -113,7 +114,7 @@ describe('AsyncHooksScopeManager', () => {
     });
 
     it('should return current scope (when enabled)', done => {
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const fn = scopeManager.bind(() => {
         assert.strictEqual(scopeManager.active(), scope, 'should have scope');
         return done();
@@ -127,7 +128,7 @@ describe('AsyncHooksScopeManager', () => {
      */
     it('should return current scope (when disabled)', done => {
       scopeManager.disable();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const fn = scopeManager.bind(() => {
         assert.strictEqual(scopeManager.active(), scope, 'should have scope');
         return done();
@@ -137,7 +138,7 @@ describe('AsyncHooksScopeManager', () => {
 
     it('should fail to return current scope (when disabled + async op)', done => {
       scopeManager.disable();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const fn = scopeManager.bind(() => {
         setTimeout(() => {
           assert.strictEqual(
@@ -153,7 +154,7 @@ describe('AsyncHooksScopeManager', () => {
 
     it('should return current scope (when re-enabled + async op)', done => {
       scopeManager.enable();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const fn = scopeManager.bind(() => {
         setTimeout(() => {
           assert.strictEqual(scopeManager.active(), scope, 'should have scope');
@@ -179,7 +180,7 @@ describe('AsyncHooksScopeManager', () => {
 
     it('should return current scope and removeListener (when enabled)', done => {
       const ee = new EventEmitter();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const patchedEe = scopeManager.bind(ee, scope);
       const handler = () => {
         assert.deepStrictEqual(scopeManager.active(), scope);
@@ -194,7 +195,7 @@ describe('AsyncHooksScopeManager', () => {
 
     it('should return current scope and removeAllListener (when enabled)', done => {
       const ee = new EventEmitter();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const patchedEe = scopeManager.bind(ee, scope);
       const handler = () => {
         assert.deepStrictEqual(scopeManager.active(), scope);
@@ -214,7 +215,7 @@ describe('AsyncHooksScopeManager', () => {
     it('should return scope (when disabled)', done => {
       scopeManager.disable();
       const ee = new EventEmitter();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const patchedEe = scopeManager.bind(ee, scope);
       const handler = () => {
         assert.deepStrictEqual(scopeManager.active(), scope);
@@ -231,7 +232,7 @@ describe('AsyncHooksScopeManager', () => {
     it('should not return current scope (when disabled + async op)', done => {
       scopeManager.disable();
       const ee = new EventEmitter();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const patchedEe = scopeManager.bind(ee, scope);
       const handler = () => {
         setImmediate(() => {
@@ -249,7 +250,7 @@ describe('AsyncHooksScopeManager', () => {
     it('should return current scope (when enabled + async op)', done => {
       scopeManager.enable();
       const ee = new EventEmitter();
-      const scope = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope = Context.ROOT_CONTEXT.setValue(key1, 1);
       const patchedEe = scopeManager.bind(ee, scope);
       const handler = () => {
         setImmediate(() => {

diff --git a/packages/opentelemetry-scope-base/src/context.ts b/packages/opentelemetry-scope-base/src/context.ts
@@ -15,7 +15,10 @@
  */
 
 /** Map of identifiers to an unknown value used internally to store context */
-type Store = { [identifer: string]: unknown };
+// type Store = { [identifer in symbol]: unknown };
+type Store = Map<symbol, unknown>;
+
+// const keys: { [identifier: string]: symbol } = {};
 
 /**
  * Class which stores and manages current context values. All methods which
@@ -28,22 +31,27 @@ export class Context {
   /** The root context is used as the default parent context when there is no active context */
   public static readonly ROOT_CONTEXT = new Context();
 
+  /** Get a key to uniquely identify a context value */
+  public static getKey(description: string) {
+    return Symbol(description);
+  }
+
   /**
    * Construct a new context which inherits values from an optional parent context.
    *
    * @param parentContext a context from which to inherit values
    */
   private constructor(parentContext?: Store) {
-    this._currentContext = Object.assign(Object.create(null), parentContext);
+    this._currentContext = parentContext ? new Map(parentContext) : new Map();
   }
 
   /**
    * Get a value from the context.
    *
    * @param key key which identifies a context value
    */
-  getValue(key: string): unknown {
-    return this._currentContext[key];
+  getValue(key: symbol): unknown {
+    return this._currentContext.get(key);
   }
 
   /**
@@ -53,9 +61,9 @@ export class Context {
    * @param key context key for which to set the value
    * @param value value to set for the given key
    */
-  setValue(key: string, value: unknown): Context {
+  setValue(key: symbol, value: unknown): Context {
     const context = new Context(this._currentContext);
-    context._currentContext[key] = value;
+    context._currentContext.set(key, value);
     return context;
   }
 
@@ -65,9 +73,9 @@ export class Context {
    *
    * @param key context key for which to clear a value
    */
-  deleteValue(key: string): Context {
+  deleteValue(key: symbol): Context {
     const context = new Context(this._currentContext);
-    delete context._currentContext[key];
+    context._currentContext.delete(key);
     return context;
   }
 }
diff --git a/packages/opentelemetry-scope-base/test/NoopScopeManager.test.ts b/packages/opentelemetry-scope-base/test/NoopScopeManager.test.ts
@@ -44,7 +44,8 @@ describe('NoopScopeManager', () => {
     });
 
     it('should run the callback (object as target)', done => {
-      const test = Context.ROOT_CONTEXT.setValue('a', 1);
+      const key = Context.getKey('test key 1');
+      const test = Context.ROOT_CONTEXT.setValue(key, 1);
       scopeManager.with(test, () => {
         assert.strictEqual(
           scopeManager.active(),

diff --git a/packages/opentelemetry-scope-zone-peer-dep/test/ZoneScopeManager.test.ts b/packages/opentelemetry-scope-zone-peer-dep/test/ZoneScopeManager.test.ts
@@ -24,6 +24,8 @@ let clock: any;
 
 describe('ZoneScopeManager', () => {
   let scopeManager: ZoneScopeManager;
+  const key1 = Context.getKey('test key 1');
+  const key2 = Context.getKey('test key 2');
 
   beforeEach(() => {
     clock = sinon.useFakeTimers();
@@ -38,7 +40,7 @@ describe('ZoneScopeManager', () => {
 
   describe('.enable()', () => {
     it('should work', () => {
-      const ctx = Context.ROOT_CONTEXT.setValue('a', 1);
+      const ctx = Context.ROOT_CONTEXT.setValue(key1, 1);
       assert.doesNotThrow(() => {
         assert(scopeManager.enable() === scopeManager, 'should return this');
         scopeManager.with(ctx, () => {
@@ -50,7 +52,7 @@ describe('ZoneScopeManager', () => {
 
   describe('.disable()', () => {
     it('should work', () => {
-      const ctx = Context.ROOT_CONTEXT.setValue('a', 1);
+      const ctx = Context.ROOT_CONTEXT.setValue(key1, 1);
       assert.doesNotThrow(() => {
         assert(scopeManager.disable() === scopeManager, 'should return this');
         scopeManager.with(ctx, () => {
@@ -69,7 +71,7 @@ describe('ZoneScopeManager', () => {
     });
 
     it('should run the callback (object as target)', done => {
-      const test = Context.ROOT_CONTEXT.setValue('a', 1);
+      const test = Context.ROOT_CONTEXT.setValue(key1, 1);
       scopeManager.with(test, () => {
         assert.strictEqual(scopeManager.active(), test, 'should have scope');
         return done();
@@ -94,9 +96,9 @@ describe('ZoneScopeManager', () => {
     });
 
     it('should finally restore an old scope, including the async task', done => {
-      const scope1 = Context.ROOT_CONTEXT.setValue('scope', 'scope1');
-      const scope2 = Context.ROOT_CONTEXT.setValue('scope', 'scope2');
-      const scope3 = Context.ROOT_CONTEXT.setValue('scope', 'scope3');
+      const scope1 = Context.ROOT_CONTEXT.setValue(key1, 'scope1');
+      const scope2 = Context.ROOT_CONTEXT.setValue(key1, 'scope2');
+      const scope3 = Context.ROOT_CONTEXT.setValue(key1, 'scope3');
 
       scopeManager.with(scope1, () => {
         assert.strictEqual(scopeManager.active(), scope1);
@@ -118,9 +120,9 @@ describe('ZoneScopeManager', () => {
     });
 
     it('should finally restore an old scope when scope is an object, including the async task', done => {
-      const scope1 = Context.ROOT_CONTEXT.setValue('x', 1);
-      const scope2 = Context.ROOT_CONTEXT.setValue('x', 2);
-      const scope3 = Context.ROOT_CONTEXT.setValue('x', 3);
+      const scope1 = Context.ROOT_CONTEXT.setValue(key1, 1);
+      const scope2 = Context.ROOT_CONTEXT.setValue(key1, 2);
+      const scope3 = Context.ROOT_CONTEXT.setValue(key1, 3);
       scopeManager.with(scope1, () => {
         assert.strictEqual(scopeManager.active(), scope1);
         scopeManager.with(scope2, () => {
@@ -140,29 +142,29 @@ describe('ZoneScopeManager', () => {
       assert.strictEqual(scopeManager.active(), window);
     });
     it('should correctly return the scopes for 3 parallel actions', () => {
-      const rootSpan = Context.ROOT_CONTEXT.setValue('name', 'root');
+      const rootSpan = Context.ROOT_CONTEXT.setValue(key1, 'root');
       scopeManager.with(rootSpan, () => {
         assert.ok(
-          scopeManager.active().getValue('name') === 'root',
+          scopeManager.active().getValue(key1) === 'root',
           'Current span is rootSpan'
         );
         const concurrentSpan1 = Context.ROOT_CONTEXT.setValue(
-          'span',
+          key2,
           'concurrentSpan1'
         );
         const concurrentSpan2 = Context.ROOT_CONTEXT.setValue(
-          'span',
+          key2,
           'concurrentSpan2'
         );
         const concurrentSpan3 = Context.ROOT_CONTEXT.setValue(
-          'span',
+          key2,
           'concurrentSpan3'
         );
 
         scopeManager.with(concurrentSpan1, () => {
           setTimeout(() => {
             assert.ok(
-              scopeManager.active().getValue('span') === concurrentSpan1,
+              scopeManager.active().getValue(key2) === concurrentSpan1,
               'Current span is concurrentSpan1'
             );
           }, 10);
@@ -171,7 +173,7 @@ describe('ZoneScopeManager', () => {
         scopeManager.with(concurrentSpan2, () => {
           setTimeout(() => {
             assert.ok(
-              scopeManager.active().getValue('span') === concurrentSpan2,
+              scopeManager.active().getValue(key2) === concurrentSpan2,
               'Current span is concurrentSpan2'
             );
           }, 20);
@@ -180,7 +182,7 @@ describe('ZoneScopeManager', () => {
         scopeManager.with(concurrentSpan3, () => {
           setTimeout(() => {
             assert.ok(
-              scopeManager.active().getValue('span') === concurrentSpan3,
+              scopeManager.active().getValue(key2) === concurrentSpan3,
               'Current span is concurrentSpan3'
             );
           }, 30);
@@ -199,12 +201,12 @@ describe('ZoneScopeManager', () => {
         }
 
         getTitle() {
-          return (scopeManager.active().getValue('obj') as Obj).title;
+          return (scopeManager.active().getValue(key1) as Obj).title;
         }
       }
 
       const obj1 = new Obj('a1');
-      const ctx = Context.ROOT_CONTEXT.setValue('obj', obj1);
+      const ctx = Context.ROOT_CONTEXT.setValue(key1, obj1);
       obj1.title = 'a2';
       const obj2 = new Obj('b1');
       const wrapper: any = scopeManager.bind(obj2.getTitle, ctx);
@@ -230,7 +232,7 @@ describe('ZoneScopeManager', () => {
     });
 
     it('should return current scope (when enabled)', done => {
-      const scope = Context.ROOT_CONTEXT.setValue('ctx', { a: 1 });
+      const scope = Context.ROOT_CONTEXT.setValue(key1, { a: 1 });
       const fn: any = scopeManager.bind(() => {
         assert.strictEqual(scopeManager.active(), scope, 'should have scope');
         return done();
@@ -240,7 +242,7 @@ describe('ZoneScopeManager', () => {
 
     it('should return root scope (when disabled)', done => {
       scopeManager.disable();
-      const scope = Context.ROOT_CONTEXT.setValue('ctx', { a: 1 });
+      const scope = Context.ROOT_CONTEXT.setValue(key1, { a: 1 });
       const fn: any = scopeManager.bind(() => {
         assert.strictEqual(
           scopeManager.active(),
@@ -253,7 +255,7 @@ describe('ZoneScopeManager', () => {
     });
 
     it('should bind the the certain scope to the target "addEventListener" function', done => {
-      const scope1 = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope1 = Context.ROOT_CONTEXT.setValue(key1, 1);
       const element = document.createElement('div');
 
       scopeManager.bind(element, scope1);
@@ -277,7 +279,7 @@ describe('ZoneScopeManager', () => {
     });
 
     it('should preserve zone when creating new click event inside zone', done => {
-      const scope1 = Context.ROOT_CONTEXT.setValue('a', 1);
+      const scope1 = Context.ROOT_CONTEXT.setValue(key1, 1);
       const element = document.createElement('div');
 
       scopeManager.bind(element, scope1);