From e03dfaf833f72d22dd3ee21045b02bc26105cea3 Mon Sep 17 00:00:00 2001
From: Jakub Malinowski <645127+jtmalinowski@users.noreply.github.com>
Date: Sat, 10 Apr 2021 20:42:22 +0200
Subject: [PATCH] fix: feedback from PR

---
 .../test/NodeTracerProvider.test.ts           | 34 +++++++++++++++++++
 .../src/BasicTracerProvider.ts                |  5 +--
 .../test/BasicTracerProvider.test.ts          | 33 +++++++++---------
 3 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/packages/opentelemetry-node/test/NodeTracerProvider.test.ts b/packages/opentelemetry-node/test/NodeTracerProvider.test.ts
index 110ef6973a..81fe634775 100644
--- a/packages/opentelemetry-node/test/NodeTracerProvider.test.ts
+++ b/packages/opentelemetry-node/test/NodeTracerProvider.test.ts
@@ -20,6 +20,7 @@ import {
   setSpan,
   setSpanContext,
   getSpan,
+  propagation,
 } from '@opentelemetry/api';
 import { AlwaysOnSampler, AlwaysOffSampler } from '@opentelemetry/core';
 import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
@@ -211,4 +212,37 @@ describe('NodeTracerProvider', () => {
       return patchedFn();
     });
   });
+
+  describe('.register()', () => {
+    let originalPropagators: string | number | undefined | string[];
+    beforeEach(() => {
+      originalPropagators = process.env.OTEL_PROPAGATORS;
+    });
+
+    afterEach(() => {
+      // otherwise we may assign 'undefined' (a string)
+      if (originalPropagators !== undefined) {
+        (process.env as any).OTEL_PROPAGATORS = originalPropagators;
+      } else {
+        delete (process.env as any).OTEL_PROPAGATORS;
+      }
+    });
+
+    it('should allow propagators as per the specification', () => {
+      (process.env as any).OTEL_PROPAGATORS = 'b3,b3multi,jaeger';
+
+      const provider = new NodeTracerProvider();
+      provider.register();
+
+      assert.deepStrictEqual(propagation.fields(), [
+        'b3',
+        'x-b3-traceid',
+        'x-b3-spanid',
+        'x-b3-flags',
+        'x-b3-sampled',
+        'x-b3-parentspanid',
+        'uber-trace-id',
+      ]);
+    });
+  });
 });
diff --git a/packages/opentelemetry-tracing/src/BasicTracerProvider.ts b/packages/opentelemetry-tracing/src/BasicTracerProvider.ts
index a0cf67e9d1..b5db060271 100644
--- a/packages/opentelemetry-tracing/src/BasicTracerProvider.ts
+++ b/packages/opentelemetry-tracing/src/BasicTracerProvider.ts
@@ -50,10 +50,6 @@ export class BasicTracerProvider implements TracerProvider {
     ['baggage', () => new HttpBaggage()],
   ]);
 
-  static registerPropagator(name: string, factory: PROPAGATOR_FACTORY): void {
-    this._registeredPropagators.set(name, factory);
-  }
-
   private readonly _config: TracerConfig;
   private readonly _registeredSpanProcessors: SpanProcessor[] = [];
   private readonly _tracers: Map<string, Tracer> = new Map();
@@ -125,6 +121,7 @@ export class BasicTracerProvider implements TracerProvider {
   }
 
   protected _buildPropagatorFromEnv(): TextMapPropagator | undefined {
+    // per spec, propagators from env must be deduplicated
     const uniquePropagatorNames = [...new Set(getEnv().OTEL_PROPAGATORS)];
 
     const propagators = uniquePropagatorNames.map(name => {
diff --git a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts
index d70ca4e056..981d7edb49 100644
--- a/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts
+++ b/packages/opentelemetry-tracing/test/BasicTracerProvider.test.ts
@@ -29,12 +29,12 @@ import {
   propagation,
   diag,
 } from '@opentelemetry/api';
+import { CompositePropagator } from '@opentelemetry/core';
 import {
   AlwaysOnSampler,
   AlwaysOffSampler,
   TraceState,
 } from '@opentelemetry/core';
-import { RAW_ENVIRONMENT } from '@opentelemetry/core/src/utils/environment';
 import { Resource } from '@opentelemetry/resources';
 import * as assert from 'assert';
 import * as sinon from 'sinon';
@@ -125,7 +125,7 @@ describe('BasicTracerProvider', () => {
   describe('.register()', () => {
     const envSource = (typeof window !== 'undefined'
       ? window
-      : process.env) as RAW_ENVIRONMENT;
+      : process.env) as any;
 
     describe('propagator', () => {
       class DummyPropagator implements TextMapPropagator {
@@ -152,7 +152,7 @@ describe('BasicTracerProvider', () => {
         [TextMapPropagator],
         TextMapPropagator
       >;
-      let originalPropagators: RAW_ENVIRONMENT['OTEL_PROPAGATORS'];
+      let originalPropagators: string | number | undefined | string[];
       beforeEach(() => {
         setGlobalPropagatorStub = sinon.spy(propagation, 'setGlobalPropagator');
         originalPropagators = envSource.OTEL_PROPAGATORS;
@@ -160,7 +160,13 @@ describe('BasicTracerProvider', () => {
 
       afterEach(() => {
         setGlobalPropagatorStub.restore();
-        envSource.OTEL_PROPAGATORS = originalPropagators;
+
+        // otherwise we may assign 'undefined' (a string)
+        if (originalPropagators !== undefined) {
+          envSource.OTEL_PROPAGATORS = originalPropagators;
+        } else {
+          delete envSource.OTEL_PROPAGATORS;
+        }
       });
 
       it('should be set to a given value if it it provided', () => {
@@ -175,27 +181,22 @@ describe('BasicTracerProvider', () => {
         );
       });
 
-      it('should be set to a given value if it it provided in an environment variable', () => {
-        envSource.OTEL_PROPAGATORS = 'dummy';
-        BasicTracerProvider.registerPropagator(
-          'dummy',
-          () => new DummyPropagator()
-        );
-
+      it('should be composite if 2 or more propagators provided in an environment variable', () => {
         const provider = new BasicTracerProvider();
         provider.register();
 
         assert.ok(
           setGlobalPropagatorStub.calledOnceWithExactly(
-            sinon.match.instanceOf(DummyPropagator)
+            sinon.match.instanceOf(CompositePropagator)
           )
         );
+        assert.deepStrictEqual(setGlobalPropagatorStub.args[0][0].fields(), [
+          'traceparent',
+          'tracestate',
+          'baggage',
+        ]);
       });
 
-      it(
-        'should be composite if more than 2 propagators provided in an environment variable'
-      );
-
       it('warns if there is no propagator registered with a given name', () => {
         const warnStub = sinon.spy(diag, 'warn');