feat(api)!: Revamp TextMapPropagator TypeScript interface

The previous interface placed the generic paramter on the interface
itself. This implies that the implementors of `TextMapPropagator`
can specify what carrier types it accepts.

As far as I can tell, this has never been the case.

The propagator API is designed to be called from the transport
layers. The transport layer (caller of `inject`/`extract`) is the
one who has control over what the carrier type is. The constraint
is that the carrier must semantically behave as an abstract map
data structure that supports setting and getting string keys/values,
and the caller must supply a matching getter/setter that works with
the given carrier type.

For example, the fetch instrumentation calls the propogation API
with one of many carrier types – POJOs, `Headers`, `Map`, etc.

Therefore, a _correct_ implementation of `TextMapPropagator` must
treat the carrier as opaque and only work with it using the supplied
getter/setter.

Unfortunately, the previous interface definition allowed a lot of
sloppiness in the implementations that would cause problems in the
real world. Fortunately, it seems like these all happen in tests,
and all the production propagators are already compliant with the
spirit of the API.

This commit moves the generic parameter from the interface into
the `inject` and `extract` methods. This forces implementors to
comply with the API more rigiously.

This is a breaking change even for compliant implementations, as
it requires adjusting the method signatures to match. If an
implementation supplied a custom type for the generic parameter
previously existed on the interface, then that probably represents
a case where it wouldn't have worked in the real world and should
be refactored/rewritten to work with the abstracted carrier type
as intended.

Ref #5365

api/CHANGELOG.md

@@ -7,6 +7,11 @@ All notable changes to this project will be documented in this file.
 
 ### :boom: Breaking Change
 
+* feat(api): revamped `TextMapPropagator` TypeScript interface [#5368](https://github.com/open-telemetry/opentelemetry-js/pull/5368) @chancancode
+  * Removed generic parameter from the `TextMapPropagator` interface
+  * Introduced a new `Carrier` generic parameter on its `inject` and `extract` methods
+  * The previous interface implies the implementor of `TextMapPropagator` can specify what carrier type it accepts, this has never been the case in practice; instead, they must treat the carrier as opaque and use the provided getter/setter.
+
 ### :rocket: (Enhancement)
 
 ### :bug: (Bug Fix)

api/src/api/propagation.ts

@@ -76,10 +76,23 @@ export class PropagationAPI {
    * @param carrier carrier to inject context into
    * @param setter Function used to set values on the carrier
    */
+  public inject<Carrier extends object | null | undefined>(
+    context: Context,
+    carrier: Carrier,
+    setter?: TextMapSetter<Carrier>
+  ): void;
+  public inject<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    setter: TextMapSetter<Carrier>
+  ): void;
   public inject<Carrier>(
     context: Context,
     carrier: Carrier,
-    setter: TextMapSetter<Carrier> = defaultTextMapSetter
+    // Note: this is only safe as long as the TypeScript overloads are enforced.
+    // If an incompatible `Carrier` is paired with `defaultTextMapSetter`, it
+    // will error at runtime when the operation is performed.
+    setter: TextMapSetter<Carrier> = defaultTextMapSetter as TextMapSetter<Carrier>
   ): void {
     return this._getGlobalPropagator().inject(context, carrier, setter);
   }
@@ -91,10 +104,23 @@ export class PropagationAPI {
    * @param carrier Carrier to extract context from
    * @param getter Function used to extract keys from a carrier
    */
+  public extract<Carrier extends object | null | undefined>(
+    context: Context,
+    carrier: Carrier,
+    getter?: TextMapGetter<Carrier>
+  ): Context;
+  public extract<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    getter: TextMapGetter<Carrier>
+  ): Context;
   public extract<Carrier>(
     context: Context,
     carrier: Carrier,
-    getter: TextMapGetter<Carrier> = defaultTextMapGetter
+    // Note: this is only safe as long as the TypeScript overloads are enforced.
+    // If an incompatible `Carrier` is paired with `defaultTextMapSetter`, it
+    // will error at runtime when the operation is performed.
+    getter: TextMapGetter<Carrier> = defaultTextMapGetter as TextMapGetter<Carrier>
   ): Context {
     return this._getGlobalPropagator().extract(context, carrier, getter);
   }

api/src/propagation/NoopTextMapPropagator.ts

@@ -22,9 +22,9 @@ import { TextMapPropagator } from './TextMapPropagator';
  */
 export class NoopTextMapPropagator implements TextMapPropagator {
   /** Noop inject function does nothing */
-  inject(_context: Context, _carrier: unknown): void {}
+  inject(): void {}
   /** Noop extract function does nothing and returns the input context */
-  extract(context: Context, _carrier: unknown): Context {
+  extract(context: Context): Context {
     return context;
   }
   fields(): string[] {

api/src/propagation/TextMapPropagator.ts

@@ -29,7 +29,7 @@ import { Context } from '../context/types';
  *
  * @since 1.0.0
  */
-export interface TextMapPropagator<Carrier = any> {
+export interface TextMapPropagator {
   /**
    * Injects values from a given `Context` into a carrier.
    *
@@ -43,7 +43,7 @@ export interface TextMapPropagator<Carrier = any> {
    * @param setter an optional {@link TextMapSetter}. If undefined, values will be
    *     set by direct object assignment.
    */
-  inject(
+  inject<Carrier>(
     context: Context,
     carrier: Carrier,
     setter: TextMapSetter<Carrier>
@@ -61,7 +61,7 @@ export interface TextMapPropagator<Carrier = any> {
    * @param getter an optional {@link TextMapGetter}. If undefined, keys will be all
    *     own properties, and keys will be accessed by direct object access.
    */
-  extract(
+  extract<Carrier>(
     context: Context,
     carrier: Carrier,
     getter: TextMapGetter<Carrier>
@@ -79,7 +79,7 @@ export interface TextMapPropagator<Carrier = any> {
  *
  * @since 1.0.0
  */
-export interface TextMapSetter<Carrier = any> {
+export interface TextMapSetter<Carrier> {
   /**
    * Callback used to set a key/value pair on an object.
    *
@@ -99,7 +99,7 @@ export interface TextMapSetter<Carrier = any> {
  *
  * @since 1.0.0
  */
-export interface TextMapGetter<Carrier = any> {
+export interface TextMapGetter<Carrier> {
   /**
    * Get a list of all keys available on the carrier.
    *
@@ -119,12 +119,12 @@ export interface TextMapGetter<Carrier = any> {
 /**
  * @since 1.0.0
  */
-export const defaultTextMapGetter: TextMapGetter = {
+export const defaultTextMapGetter: TextMapGetter<object | null | undefined> = {
   get(carrier, key) {
     if (carrier == null) {
       return undefined;
     }
-    return carrier[key];
+    return (carrier as Record<string, string>)[key];
   },
 
   keys(carrier) {
@@ -138,12 +138,12 @@ export const defaultTextMapGetter: TextMapGetter = {
 /**
  * @since 1.0.0
  */
-export const defaultTextMapSetter: TextMapSetter = {
+export const defaultTextMapSetter: TextMapSetter<object | null | undefined> = {
   set(carrier, key, value) {
     if (carrier == null) {
       return;
     }
 
-    carrier[key] = value;
+    (carrier as Record<string, string>)[key] = value;
   },
 };

api/test/common/api/api.test.ts

@@ -18,8 +18,6 @@ import * as assert from 'assert';
 import api, {
   context,
   Context,
-  defaultTextMapGetter,
-  defaultTextMapSetter,
   diag,
   metrics,
   propagation,
@@ -137,31 +135,23 @@ describe('API', () => {
     describe('should use the global propagation', () => {
       const testKey = Symbol('kTestKey');
 
-      interface Carrier {
-        context?: Context;
-        setter?: TextMapSetter;
-      }
+      type Carrier = Record<string, string | undefined>;
 
-      class TestTextMapPropagation implements TextMapPropagator<Carrier> {
-        inject(
+      class TestTextMapPropagation implements TextMapPropagator {
+        inject<C>(
           context: Context,
-          carrier: Carrier,
-          setter: TextMapSetter
+          carrier: C,
+          setter: TextMapSetter<C>
         ): void {
-          carrier.context = context;
-          carrier.setter = setter;
+          setter.set(carrier, 'TestField', String(context.getValue(testKey)));
         }
 
-        extract(
+        extract<C>(
           context: Context,
-          carrier: Carrier,
-          getter: TextMapGetter
+          carrier: C,
+          getter: TextMapGetter<C>
         ): Context {
-          return context.setValue(testKey, {
-            context,
-            carrier,
-            getter,
-          });
+          return context.setValue(testKey, getter.get(carrier, 'TestField'));
         }
 
         fields(): string[] {
@@ -172,41 +162,40 @@ describe('API', () => {
       it('inject', () => {
         api.propagation.setGlobalPropagator(new TestTextMapPropagation());
 
-        const context = ROOT_CONTEXT.setValue(testKey, 15);
+        const context = ROOT_CONTEXT.setValue(testKey, 'test-value');
         const carrier: Carrier = {};
         api.propagation.inject(context, carrier);
-        assert.strictEqual(carrier.context, context);
-        assert.strictEqual(carrier.setter, defaultTextMapSetter);
+        assert.strictEqual(carrier['TestField'], 'test-value');
 
-        const setter: TextMapSetter = {
-          set: () => {},
+        const setter: TextMapSetter<Carrier> = {
+          set: (carrier, key, value) => {
+            carrier[key.toLowerCase()] = value.toUpperCase();
+          },
         };
         api.propagation.inject(context, carrier, setter);
-        assert.strictEqual(carrier.context, context);
-        assert.strictEqual(carrier.setter, setter);
+        assert.strictEqual(carrier['testfield'], 'TEST-VALUE');
       });
 
       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,
+        let context = api.propagation.extract(ROOT_CONTEXT, {
+          TestField: 'test-value',
+        });
+        let data = context.getValue(testKey);
+        assert.strictEqual(data, 'test-value');
+
+        const getter: TextMapGetter<Carrier> = {
+          keys: carrier => Object.keys(carrier),
+          get: (carrier, key) => carrier[key.toLowerCase()],
         };
-        context = api.propagation.extract(ROOT_CONTEXT, carrier, getter);
+        context = api.propagation.extract(
+          ROOT_CONTEXT,
+          { testfield: 'TEST-VALUE' },
+          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);
+        assert.strictEqual(data, 'TEST-VALUE');
       });
 
       it('fields', () => {

doc/propagation.md

@@ -34,7 +34,11 @@ const MyHeader = 'my-header';
 
 export class MyPropagator implements TextMapPropagator {
   // Inject the header to the outgoing request.
-  inject(context: Context, carrier: unknown, setter: TextMapSetter): void {
+  inject<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    setter: TextMapSetter<Carrier>
+  ): void {
     const spanContext = trace.getSpanContext(context);
     // Skip if the current span context is not valid or suppressed.
     if (
@@ -51,7 +55,11 @@ export class MyPropagator implements TextMapPropagator {
   }
 
   // Extract the header from the incoming request.
-  extract(context: Context, carrier: unknown, getter: TextMapGetter): Context {
+  extract<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    getter: TextMapGetter<Carrier>
+  ): Context {
     const headers = getter.get(carrier, MyHeader);
     const header = Array.isArray(headers) ? headers[0] : headers;
     if (typeof header !== 'string') return context;

experimental/packages/opentelemetry-instrumentation-http/test/utils/DummyPropagation.ts

@@ -15,33 +15,63 @@
  */
 import {
   Context,
+  TextMapGetter,
   TextMapPropagator,
+  TextMapSetter,
   trace,
   TraceFlags,
 } from '@opentelemetry/api';
-import type * as http from 'http';
 
 export class DummyPropagation implements TextMapPropagator {
   static TRACE_CONTEXT_KEY = 'x-dummy-trace-id';
   static SPAN_CONTEXT_KEY = 'x-dummy-span-id';
-  extract(context: Context, carrier: http.OutgoingHttpHeaders) {
-    const extractedSpanContext = {
-      traceId: carrier[DummyPropagation.TRACE_CONTEXT_KEY] as string,
-      spanId: carrier[DummyPropagation.SPAN_CONTEXT_KEY] as string,
-      traceFlags: TraceFlags.SAMPLED,
-      isRemote: true,
-    };
-    if (extractedSpanContext.traceId && extractedSpanContext.spanId) {
+
+  extract<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    getter: TextMapGetter<Carrier>
+  ) {
+    const traceId = getter.get(carrier, DummyPropagation.TRACE_CONTEXT_KEY);
+    const spanId = getter.get(carrier, DummyPropagation.SPAN_CONTEXT_KEY);
+
+    if (traceId && spanId) {
+      if (typeof traceId !== 'string') {
+        throw new Error('expecting traceId to be a string');
+      }
+
+      if (typeof spanId !== 'string') {
+        throw new Error('expecting spanId to be a string');
+      }
+
+      const extractedSpanContext = {
+        traceId,
+        spanId,
+        traceFlags: TraceFlags.SAMPLED,
+        isRemote: true,
+      };
+
       return trace.setSpanContext(context, extractedSpanContext);
     }
+
     return context;
   }
-  inject(context: Context, headers: { [custom: string]: string }): void {
+
+  inject<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    setter: TextMapSetter<Carrier>
+  ): void {
     const spanContext = trace.getSpanContext(context);
     if (!spanContext) return;
-    headers[DummyPropagation.TRACE_CONTEXT_KEY] = spanContext.traceId;
-    headers[DummyPropagation.SPAN_CONTEXT_KEY] = spanContext.spanId;
+
+    setter.set(
+      carrier,
+      DummyPropagation.TRACE_CONTEXT_KEY,
+      spanContext.traceId
+    );
+    setter.set(carrier, DummyPropagation.SPAN_CONTEXT_KEY, spanContext.spanId);
   }
+
   fields(): string[] {
     return [
       DummyPropagation.TRACE_CONTEXT_KEY,

packages/opentelemetry-core/src/baggage/propagation/W3CBaggagePropagator.ts

@@ -39,7 +39,11 @@ import { getKeyPairs, parsePairKeyValue, serializeKeyPairs } from '../utils';
  * https://w3c.github.io/baggage/
  */
 export class W3CBaggagePropagator implements TextMapPropagator {
-  inject(context: Context, carrier: unknown, setter: TextMapSetter): void {
+  inject<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    setter: TextMapSetter<Carrier>
+  ): void {
     const baggage = propagation.getBaggage(context);
     if (!baggage || isTracingSuppressed(context)) return;
     const keyPairs = getKeyPairs(baggage)
@@ -53,7 +57,11 @@ export class W3CBaggagePropagator implements TextMapPropagator {
     }
   }
 
-  extract(context: Context, carrier: unknown, getter: TextMapGetter): Context {
+  extract<Carrier>(
+    context: Context,
+    carrier: Carrier,
+    getter: TextMapGetter<Carrier>
+  ): Context {
     const headerValue = getter.get(carrier, BAGGAGE_HEADER);
     const baggageString = Array.isArray(headerValue)
       ? headerValue.join(BAGGAGE_ITEMS_SEPARATOR)