chore: requires user to pass context to propagation APIs (#1734)

* chore: requires user to pass context to propagation APIs At least for extract() using the current active context is often a bad choice because usually a plugin wants to use an extracted span instead the current active. Change propagation APIs to require context as first argument instead of using active context on default. This ensures that plugin developers have to be explicit instead of relying on a potential wrong default. * (chore) fix lint * add test * Adapt http instrumentation * fix: update integration testserver
open-telemetry · Dec 14, 2020 · 5701473 · 5701473
1 parent 0d06a52
commit 5701473
Show file tree

Hide file tree

Showing 12 changed files with 120 additions and 36 deletions.
diff --git a/integration-tests/propagation-validation-server/validation-server.js b/integration-tests/propagation-validation-server/validation-server.js
@@ -1,7 +1,7 @@
 const axios = require("axios");
 const { HttpTraceContext } = require("@opentelemetry/core");
 const { BasicTracerProvider } = require("@opentelemetry/tracing");
-const { context, propagation, trace } = require("@opentelemetry/api");
+const { context, propagation, trace, ROOT_CONTEXT } = require("@opentelemetry/api");
 const {
   AsyncHooksContextManager,
 } = require("@opentelemetry/context-async-hooks");
@@ -31,14 +31,14 @@ app.use(bodyParser.json());
 
 // Mount our demo route
 app.post("/verify-tracecontext", (req, res) => {
-  context.with(propagation.extract(req.headers), () => {
+  context.with(propagation.extract(ROOT_CONTEXT, req.headers), () => {
     Promise.all(
       req.body.map((action) => {
         const span = tracer.startSpan("propagate-w3c");
         let promise;
         tracer.withSpan(span, () => {
           const headers = {};
-          propagation.inject(headers);
+          propagation.inject(context.active(), headers);
           promise = axios
             .post(
               action.url,
@@ -57,8 +57,8 @@ app.post("/verify-tracecontext", (req, res) => {
         return promise;
       })
     )
-      .then(() => res.send("hello"))
-      .catch((err) => res.status(500).send(err));
+    .then(() => res.send("hello"))
+    .catch((err) => res.status(500).send(err));
   });
 });
 

diff --git a/packages/opentelemetry-api/src/api/propagation.ts b/packages/opentelemetry-api/src/api/propagation.ts
@@ -23,16 +23,13 @@ import {
   TextMapPropagator,
   TextMapSetter,
 } from '../context/propagation/TextMapPropagator';
-import { ContextAPI } from './context';
 import {
   API_BACKWARDS_COMPATIBILITY_VERSION,
   GLOBAL_PROPAGATION_API_KEY,
   makeGetter,
   _global,
 } from './global-utils';
 
-const contextApi = ContextAPI.getInstance();
-
 /**
  * Singleton object which represents the entry point to the OpenTelemetry Propagation API
  */
@@ -72,29 +69,29 @@ export class PropagationAPI {
   /**
    * Inject context into a carrier to be propagated inter-process
    *
+   * @param context Context carrying tracing data to inject
    * @param carrier carrier to inject context into
    * @param setter Function used to set values on the carrier
-   * @param context Context carrying tracing data to inject. Defaults to the currently active context.
    */
   public inject<Carrier>(
+    context: Context,
     carrier: Carrier,
-    setter: TextMapSetter<Carrier> = defaultTextMapSetter,
-    context: Context = contextApi.active()
+    setter: TextMapSetter<Carrier> = defaultTextMapSetter
   ): void {
     return this._getGlobalPropagator().inject(context, carrier, setter);
   }
 
   /**
    * Extract context from a carrier
    *
+   * @param context Context which the newly created context will inherit from
    * @param carrier Carrier to extract context from
    * @param getter Function used to extract keys from a carrier
-   * @param context Context which the newly created context will inherit from. Defaults to the currently active context.
    */
   public extract<Carrier>(
+    context: Context,
     carrier: Carrier,
-    getter: TextMapGetter<Carrier> = defaultTextMapGetter,
-    context: Context = contextApi.active()
+    getter: TextMapGetter<Carrier> = defaultTextMapGetter
   ): Context {
     return this._getGlobalPropagator().extract(context, carrier, getter);
   }

diff --git a/packages/opentelemetry-api/test/api/api.test.ts b/packages/opentelemetry-api/test/api/api.test.ts
@@ -26,6 +26,13 @@ import api, {
   trace,
   propagation,
   metrics,
+  TextMapPropagator,
+  Context,
+  TextMapSetter,
+  TextMapGetter,
+  ROOT_CONTEXT,
+  defaultTextMapSetter,
+  defaultTextMapGetter,
 } from '../../src';
 
 describe('API', () => {
@@ -84,5 +91,81 @@ describe('API', () => {
         return new TestTracer();
       }
     }
+
+    describe('should use the global propagation', () => {
+      const testKey = Symbol('kTestKey');
+
+      interface Carrier {
+        context?: Context;
+        setter?: TextMapSetter;
+      }
+
+      class TestTextMapPropagation implements TextMapPropagator<Carrier> {
+        inject(
+          context: Context,
+          carrier: Carrier,
+          setter: TextMapSetter
+        ): void {
+          carrier.context = context;
+          carrier.setter = setter;
+        }
+
+        extract(
+          context: Context,
+          carrier: Carrier,
+          getter: TextMapGetter
+        ): Context {
+          return context.setValue(testKey, {
+            context,
+            carrier,
+            getter,
+          });
+        }
+
+        fields(): string[] {
+          return ['TestField'];
+        }
+      }
+
+      it('inject', () => {
+        api.propagation.setGlobalPropagator(new TestTextMapPropagation());
+
+        const context = ROOT_CONTEXT.setValue(testKey, 15);
+        const carrier: Carrier = {};
+        api.propagation.inject(context, carrier);
+        assert.strictEqual(carrier.context, context);
+        assert.strictEqual(carrier.setter, defaultTextMapSetter);
+
+        const setter: TextMapSetter = {
+          set: () => {},
+        };
+        api.propagation.inject(context, carrier, setter);
+        assert.strictEqual(carrier.context, context);
+        assert.strictEqual(carrier.setter, setter);
+      });
+
+      it('extract', () => {
+        api.propagation.setGlobalPropagator(new TestTextMapPropagation());
+
+        const carrier: Carrier = {};
+        let context = api.propagation.extract(ROOT_CONTEXT, carrier);
+        let data: any = context.getValue(testKey);
+        assert.ok(data != null);
+        assert.strictEqual(data.context, ROOT_CONTEXT);
+        assert.strictEqual(data.carrier, carrier);
+        assert.strictEqual(data.getter, defaultTextMapGetter);
+
+        const getter: TextMapGetter = {
+          keys: () => [],
+          get: () => undefined,
+        };
+        context = api.propagation.extract(ROOT_CONTEXT, carrier, getter);
+        data = context.getValue(testKey);
+        assert.ok(data != null);
+        assert.strictEqual(data.context, ROOT_CONTEXT);
+        assert.strictEqual(data.carrier, carrier);
+        assert.strictEqual(data.getter, getter);
+      });
+    });
   });
 });
diff --git a/packages/opentelemetry-instrumentation-http/src/http.ts b/packages/opentelemetry-instrumentation-http/src/http.ts
@@ -24,6 +24,7 @@ import {
   setActiveSpan,
   SpanContext,
   TraceFlags,
+  ROOT_CONTEXT,
 } from '@opentelemetry/api';
 import { NoRecordingSpan } from '@opentelemetry/core';
 import type * as http from 'http';
@@ -403,7 +404,7 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
         }),
       };
 
-      return context.with(propagation.extract(headers), () => {
+      return context.with(propagation.extract(ROOT_CONTEXT, headers), () => {
         const span = instrumentation._startHttpSpan(
           `${component.toLocaleUpperCase()} ${method}`,
           spanOptions
@@ -539,9 +540,8 @@ export class HttpInstrumentation extends InstrumentationBase<Http> {
         optionsParsed.headers = {};
       }
       propagation.inject(
-        optionsParsed.headers,
-        undefined,
-        setActiveSpan(context.active(), span)
+        setActiveSpan(context.active(), span),
+        optionsParsed.headers
       );
 
       const request: http.ClientRequest = safeExecuteInTheMiddle(

diff --git a/packages/opentelemetry-instrumentation-http/src/version.ts b/packages/opentelemetry-instrumentation-http/src/version.ts
@@ -15,4 +15,4 @@
  */
 
 // this is autogenerated file, see scripts/version-update.js
-export const VERSION = '0.12.0';
+export const VERSION = '0.13.0';
diff --git a/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts b/packages/opentelemetry-instrumentation-xml-http-request/src/xhr.ts
@@ -109,7 +109,7 @@ export class XMLHttpRequestInstrumentation extends InstrumentationBase<XMLHttpRe
       return;
     }
     const headers: { [key: string]: unknown } = {};
-    api.propagation.inject(headers);
+    api.propagation.inject(api.context.active(), headers);
     Object.keys(headers).forEach(key => {
       xhr.setRequestHeader(key, String(headers[key]));
     });

diff --git a/packages/opentelemetry-plugin-fetch/src/fetch.ts b/packages/opentelemetry-plugin-fetch/src/fetch.ts
@@ -21,7 +21,6 @@ import * as web from '@opentelemetry/web';
 import { AttributeNames } from './enums/AttributeNames';
 import { FetchError, FetchResponse, SpanData } from './types';
 import { VERSION } from './version';
-import { setActiveSpan } from '@opentelemetry/api';
 
 // how long to wait for observer to collect information about resources
 // this is needed as event "load" is called before observer
@@ -69,7 +68,7 @@ export class FetchPlugin extends core.BasePlugin<Promise<Response>> {
       {
         startTime: corsPreFlightRequest[web.PerformanceTimingNames.FETCH_START],
       },
-      setActiveSpan(api.context.active(), span)
+      api.setActiveSpan(api.context.active(), span)
     );
     web.addSpanNetworkEvents(childSpan, corsPreFlightRequest);
     childSpan.end(
@@ -113,12 +112,12 @@ export class FetchPlugin extends core.BasePlugin<Promise<Response>> {
     }
 
     if (options instanceof Request) {
-      api.propagation.inject(options.headers, {
+      api.propagation.inject(api.context.active(), options.headers, {
         set: (h, k, v) => h.set(k, typeof v === 'string' ? v : String(v)),
       });
     } else {
       const headers: Partial<Record<string, unknown>> = {};
-      api.propagation.inject(headers);
+      api.propagation.inject(api.context.active(), headers);
       options.headers = Object.assign({}, headers, options.headers || {});
     }
   }

diff --git a/packages/opentelemetry-plugin-grpc-js/src/client/utils.ts b/packages/opentelemetry-plugin-grpc-js/src/client/utils.ts
@@ -22,6 +22,7 @@ import {
   StatusCode,
   Status,
   propagation,
+  context,
 } from '@opentelemetry/api';
 import { RpcAttribute } from '@opentelemetry/semantic-conventions';
 import type * as grpcJs from '@grpc/grpc-js';
@@ -243,7 +244,7 @@ function getMetadata(
  * @param metadata
  */
 export function setSpanContext(metadata: grpcJs.Metadata): void {
-  propagation.inject(metadata, {
+  propagation.inject(context.active(), metadata, {
     set: (metadata, k, v) => metadata.set(k, v as grpcJs.MetadataValue),
   });
 }
diff --git a/packages/opentelemetry-plugin-grpc-js/src/server/patchServer.ts b/packages/opentelemetry-plugin-grpc-js/src/server/patchServer.ts
@@ -29,6 +29,7 @@ import {
   SpanKind,
   propagation,
   Span,
+  ROOT_CONTEXT,
 } from '@opentelemetry/api';
 import { RpcAttribute } from '@opentelemetry/semantic-conventions';
 import { clientStreamAndUnaryHandler } from './clientStreamAndUnary';
@@ -101,7 +102,7 @@ export function patchServer(
             plugin.logger.debug('patch func: %s', JSON.stringify(spanOptions));
 
             context.with(
-              propagation.extract(call.metadata, {
+              propagation.extract(ROOT_CONTEXT, call.metadata, {
                 get: (carrier, key) => carrier.get(key).map(String),
                 keys: carrier => Object.keys(carrier.getMap()),
               }),

diff --git a/packages/opentelemetry-plugin-grpc/src/grpc.ts b/packages/opentelemetry-plugin-grpc/src/grpc.ts
@@ -22,6 +22,7 @@ import {
   SpanKind,
   SpanOptions,
   Status,
+  ROOT_CONTEXT,
 } from '@opentelemetry/api';
 import { RpcAttribute } from '@opentelemetry/semantic-conventions';
 import { BasePlugin } from '@opentelemetry/core';
@@ -124,7 +125,7 @@ export class GrpcPlugin extends BasePlugin<grpc> {
   }
 
   private _setSpanContext(metadata: grpcTypes.Metadata): void {
-    propagation.inject(metadata, {
+    propagation.inject(context.active(), metadata, {
       set: (metadata, k, v) => metadata.set(k, v as grpcTypes.MetadataValue),
     });
   }
@@ -182,7 +183,7 @@ export class GrpcPlugin extends BasePlugin<grpc> {
               );
 
               context.with(
-                propagation.extract(call.metadata, {
+                propagation.extract(ROOT_CONTEXT, call.metadata, {
                   get: (metadata, key) => metadata.get(key).map(String),
                   keys: metadata => Object.keys(metadata.getMap()),
                 }),

diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts
@@ -24,6 +24,7 @@ import {
   SpanContext,
   TraceFlags,
   setActiveSpan,
+  ROOT_CONTEXT,
 } from '@opentelemetry/api';
 import { BasePlugin, NoRecordingSpan } from '@opentelemetry/core';
 import type {
@@ -306,7 +307,7 @@ export class HttpPlugin extends BasePlugin<Http> {
         }),
       };
 
-      return context.with(propagation.extract(headers), () => {
+      return context.with(propagation.extract(ROOT_CONTEXT, headers), () => {
         const span = plugin._startHttpSpan(`HTTP ${method}`, spanOptions);
 
         return plugin._tracer.withSpan(span, () => {
@@ -414,9 +415,8 @@ export class HttpPlugin extends BasePlugin<Http> {
         optionsParsed.headers = {};
       }
       propagation.inject(
-        optionsParsed.headers,
-        undefined,
-        setActiveSpan(context.active(), span)
+        setActiveSpan(context.active(), span),
+        optionsParsed.headers
       );
 
       const request: ClientRequest = plugin._safeExecute(

diff --git a/packages/opentelemetry-shim-opentracing/src/shim.ts b/packages/opentelemetry-shim-opentracing/src/shim.ts
@@ -175,12 +175,11 @@ export class TracerShim extends opentracing.Tracer {
       case opentracing.FORMAT_HTTP_HEADERS:
       case opentracing.FORMAT_TEXT_MAP: {
         api.propagation.inject(
-          carrier,
-          api.defaultTextMapSetter,
           setCorrelationContext(
             api.setExtractedSpanContext(api.ROOT_CONTEXT, oTelSpanContext),
             oTelSpanCorrelationContext
-          )
+          ),
+          carrier
         );
         return;
       }
@@ -199,7 +198,10 @@ export class TracerShim extends opentracing.Tracer {
     switch (format) {
       case opentracing.FORMAT_HTTP_HEADERS:
       case opentracing.FORMAT_TEXT_MAP: {
-        const context: api.Context = api.propagation.extract(carrier);
+        const context: api.Context = api.propagation.extract(
+          api.ROOT_CONTEXT,
+          carrier
+        );
         const spanContext = api.getParentSpanContext(context);
         const correlationContext = getCorrelationContext(context);