doc(api): clearify TextMapPropagator API requirements

The current interface places the generic paramter on the interface
itself. This implies that the implementors of `TextMapPropagator`
can specify what carrier types it accepts (and that each implementor
only work with one specific type of carrier).

```ts
interface TextMapPropagator<Carrier> {
  inject(context: Context, carrier: Carrier, setter: TextMapSetter<Carrier>): void;
  extract(context: Context, carrier: Carrier, getter: TextMapGetter<Carrier>): void;
}
```

In reality, this is not the case.

The propagator API is designed to be called by participating code
around the various transport layers (such as the `fetch` inst on
the browser, or integration with the HTTP server library on the
backend), and it is these callers that ultimately controls what
carrier type the currently configured propagator is called with.

Therefore, a correct implementation of this interface must treat
the carrier as an opaque value, and only work with it using the
provided getter/setter.

Ideally, the interface should look like this instead:

```ts
interface TextMapPropagator {
  inject<Carrier>(context: Context, carrier: Carrier, setter: TextMapSetter<Carrier>): void;
  extract<Carrier>(context: Context, carrier: Carrier, getter: TextMapGetter<Carrier>): void;
}
```

This communicates and enforces the contract. Unfortunately, that
would be a breking change we are not currently prepared to make.

Instead, this commit updates the documentation to explicitly
document the discrapancy and advice implemntors the correct way
forward.

It also updates our own implementations to follow the recommended
pattern, as well as updating the tests to be more well-behaved
around this, as some of them are written to rely on this exact
behavior that would be problematic in the real world.

Ref open-telemetry#5365
Ref open-telemetry#5368

Author: chancancode

api/src/propagation/TextMapPropagator.ts

@@ -22,10 +22,30 @@ import { Context } from '../context/types';
  * HTTP Header Field semantics. Values are often encoded as RPC/HTTP request
  * headers.
  *
- * The carrier of propagated data on both the client (injector) and server
- * (extractor) side is usually an object such as http headers. Propagation is
- * usually implemented via library-specific request interceptors, where the
- * client-side injects values and the server-side extracts them.
+ * Propagation is usually implemented via library-specific request
+ * interceptors, where the client-side injects values and the server-side
+ * extracts them.
+ *
+ * @template Carrier **It is strongly recommended to set the `Carrier` type
+ * parameter to `unknown`, as it is the only correct type here.**
+ *
+ * The carrier is the medium for communicating propagated data between the
+ * client (injector) and server (extractor) side, such as HTTP headers. To
+ * work with `TextMapPropagator`, the carrier type must semantically function
+ * as an abstract map data structure, supporting string keys and values.
+ *
+ * This type parameter exists on the interface for historical reasons. While
+ * it may be suggest that implementors have a choice over what type of carrier
+ * medium it accepts, this is not true in practice.
+ *
+ * The propagator API is designed to be called by participating code around the
+ * various transport layers (such as the `fetch` instrumentation on the browser
+ * client, or integration with the HTTP server library on the backend), and it
+ * is these callers that ultimately controls what carrier type your propagator
+ * is called with.
+ *
+ * Therefore, a correct implementation of this interface must treat the carrier
+ * as an opaque value, and only work with it using the provided getter/setter.
  *
  * @since 1.0.0
  */
@@ -38,10 +58,10 @@ export interface TextMapPropagator<Carrier = any> {
    *
    * @param context the Context from which to extract values to transmit over
    *     the wire.
-   * @param carrier the carrier of propagation fields, such as http request
+   * @param carrier the carrier of propagation fields, such as HTTP request
    *     headers.
-   * @param setter an optional {@link TextMapSetter}. If undefined, values will be
-   *     set by direct object assignment.
+   * @param setter a {@link TextMapSetter} guaranteed to work with the given
+   *     carrier, implementors must use this to set values on the carrier.
    */
   inject(
     context: Context,
@@ -56,10 +76,10 @@ export interface TextMapPropagator<Carrier = any> {
    *
    * @param context the Context from which to extract values to transmit over
    *     the wire.
-   * @param carrier the carrier of propagation fields, such as http request
+   * @param carrier the carrier of propagation fields, such as HTTP request
    *     headers.
-   * @param getter an optional {@link TextMapGetter}. If undefined, keys will be all
-   *     own properties, and keys will be accessed by direct object access.
+   * @param getter a {@link TextMapGetter} guaranteed to work with the given
+   *     carrier, implementors must use this to read values from the carrier.
    */
   extract(
     context: Context,

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> {
+      class TestTextMapPropagation implements TextMapPropagator<unknown> {
         inject(
           context: Context,
-          carrier: Carrier,
-          setter: TextMapSetter
+          carrier: unknown,
+          setter: TextMapSetter<unknown>
         ): 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: unknown,
+          getter: TextMapGetter<unknown>
         ): 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

@@ -32,9 +32,13 @@ import { isTracingSuppressed } from '@opentelemetry/core';
 // Example header, the content format can be `<trace-id>:<span-id>`
 const MyHeader = 'my-header';
 
-export class MyPropagator implements TextMapPropagator {
+export class MyPropagator implements TextMapPropagator<unknown> {
   // Inject the header to the outgoing request.
-  inject(context: Context, carrier: unknown, setter: TextMapSetter): void {
+  inject(
+    context: Context,
+    carrier: unknown,
+    setter: TextMapSetter<unknown>
+  ): 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(
+    context: Context,
+    carrier: unknown,
+    getter: TextMapGetter<unknown>
+  ): 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,60 @@
  */
 import {
   Context,
+  TextMapGetter,
   TextMapPropagator,
+  TextMapSetter,
   trace,
   TraceFlags,
 } from '@opentelemetry/api';
-import type * as http from 'http';
 
-export class DummyPropagation implements TextMapPropagator {
+export class DummyPropagation implements TextMapPropagator<unknown> {
   static TRACE_CONTEXT_KEY = 'x-dummy-trace-id';
   static SPAN_CONTEXT_KEY = 'x-dummy-span-id';
-  extract(context: Context, carrier: http.OutgoingHttpHeaders) {
+
+  extract(context: Context, carrier: unknown, getter: TextMapGetter<unknown>) {
+    const traceId = getter.get(carrier, DummyPropagation.TRACE_CONTEXT_KEY);
+
+    if (typeof traceId !== 'string') {
+      throw new Error('expecting traceId to be a string');
+    }
+
+    const spanId = getter.get(carrier, DummyPropagation.SPAN_CONTEXT_KEY);
+
+    if (typeof spanId !== 'string') {
+      throw new Error('expecting spanId to be a string');
+    }
+
     const extractedSpanContext = {
-      traceId: carrier[DummyPropagation.TRACE_CONTEXT_KEY] as string,
-      spanId: carrier[DummyPropagation.SPAN_CONTEXT_KEY] as string,
+      traceId,
+      spanId,
       traceFlags: TraceFlags.SAMPLED,
       isRemote: true,
     };
+
     if (extractedSpanContext.traceId && extractedSpanContext.spanId) {
       return trace.setSpanContext(context, extractedSpanContext);
     }
+
     return context;
   }
-  inject(context: Context, headers: { [custom: string]: string }): void {
+
+  inject(
+    context: Context,
+    carrier: unknown,
+    setter: TextMapSetter<unknown>
+  ): 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

@@ -38,8 +38,12 @@ import { getKeyPairs, parsePairKeyValue, serializeKeyPairs } from '../utils';
  * Based on the Baggage specification:
  * https://w3c.github.io/baggage/
  */
-export class W3CBaggagePropagator implements TextMapPropagator {
-  inject(context: Context, carrier: unknown, setter: TextMapSetter): void {
+export class W3CBaggagePropagator implements TextMapPropagator<unknown> {
+  inject(
+    context: Context,
+    carrier: unknown,
+    setter: TextMapSetter<unknown>
+  ): 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(
+    context: Context,
+    carrier: unknown,
+    getter: TextMapGetter<unknown>
+  ): Context {
     const headerValue = getter.get(carrier, BAGGAGE_HEADER);
     const baggageString = Array.isArray(headerValue)
       ? headerValue.join(BAGGAGE_ITEMS_SEPARATOR)

packages/opentelemetry-core/src/propagation/composite.ts

@@ -33,7 +33,7 @@ export interface CompositePropagatorConfig {
 }
 
 /** Combines multiple propagators into a single propagator. */
-export class CompositePropagator implements TextMapPropagator {
+export class CompositePropagator implements TextMapPropagator<unknown> {
   private readonly _propagators: TextMapPropagator[];
   private readonly _fields: string[];
 
@@ -64,7 +64,11 @@ export class CompositePropagator implements TextMapPropagator {
    * @param context Context to inject
    * @param carrier Carrier into which context will be injected
    */
-  inject(context: Context, carrier: unknown, setter: TextMapSetter): void {
+  inject(
+    context: Context,
+    carrier: unknown,
+    setter: TextMapSetter<unknown>
+  ): void {
     for (const propagator of this._propagators) {
       try {
         propagator.inject(context, carrier, setter);
@@ -85,7 +89,11 @@ export class CompositePropagator implements TextMapPropagator {
    * @param context Context to add values to
    * @param carrier Carrier from which to extract context
    */
-  extract(context: Context, carrier: unknown, getter: TextMapGetter): Context {
+  extract(
+    context: Context,
+    carrier: unknown,
+    getter: TextMapGetter<unknown>
+  ): Context {
     return this._propagators.reduce((ctx, propagator) => {
       try {
         return propagator.extract(ctx, carrier, getter);

packages/opentelemetry-core/src/trace/W3CTraceContextPropagator.ts

@@ -71,8 +71,12 @@ export function parseTraceParent(traceParent: string): SpanContext | null {
  * Based on the Trace Context specification:
  * https://www.w3.org/TR/trace-context/
  */
-export class W3CTraceContextPropagator implements TextMapPropagator {
-  inject(context: Context, carrier: unknown, setter: TextMapSetter): void {
+export class W3CTraceContextPropagator implements TextMapPropagator<unknown> {
+  inject(
+    context: Context,
+    carrier: unknown,
+    setter: TextMapSetter<unknown>
+  ): void {
     const spanContext = trace.getSpanContext(context);
     if (
       !spanContext ||
@@ -95,7 +99,11 @@ export class W3CTraceContextPropagator implements TextMapPropagator {
     }
   }
 
-  extract(context: Context, carrier: unknown, getter: TextMapGetter): Context {
+  extract(
+    context: Context,
+    carrier: unknown,
+    getter: TextMapGetter<unknown>
+  ): Context {
     const traceParentHeader = getter.get(carrier, TRACE_PARENT_HEADER);
     if (!traceParentHeader) return context;
     const traceParent = Array.isArray(traceParentHeader)