Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api)!: Move TextMapPropagator<Carrier> generic parameter to interface methods #5368

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(api)!: Move TextMapPropagator<Carrier> generic parameter to interface methods #5368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
30 changes: 28 additions & 2 deletions api/src/api/propagation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

@chancancode chancancode Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this signature change is also itself a breaking change, but code that would have stopped compiling after this change is probably broken IRL anyway. (Same for extract below.)

This signature is also not really ideal – Map is an object, Headers is an object too. None of those would work correctly if you don't also pass in a matching setter (and getter in the case of extract), but the first overload would let it slide.

We could change it to say Carrier extends Record<string, string> | null | undefined I suppose, I think that's technically more correct, but it is probably be too narrow for the intended ergonomic benefit here – a lot of types that you would want to pass into here and elide the setter/getter won't be accepted, and it would either require the caller to cast or explicitly import and pass the default setter/getter, which would be a shame.

As it stands it's a compromise position, but not a bad one at that.

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);
}
Expand All @@ -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);
}
Expand Down
4 changes: 2 additions & 2 deletions api/src/propagation/NoopTextMapPropagator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@
*/
export class NoopTextMapPropagator implements TextMapPropagator {
/** Noop inject function does nothing */
inject(_context: Context, _carrier: unknown): void {}
inject(): void {}

Check warning on line 25 in api/src/propagation/NoopTextMapPropagator.ts

View check run for this annotation

Codecov / codecov/patch

api/src/propagation/NoopTextMapPropagator.ts#L25 

Added line #L25 was not covered by tests
/** Noop extract function does nothing and returns the input context */
extract(context: Context, _carrier: unknown): Context {
extract(context: Context): Context {

Check warning on line 27 in api/src/propagation/NoopTextMapPropagator.ts

View check run for this annotation

Codecov / codecov/patch

api/src/propagation/NoopTextMapPropagator.ts#L27 

Added line #L27 was not covered by tests
return context;
}
fields(): string[] {
Expand Down
18 changes: 9 additions & 9 deletions api/src/propagation/TextMapPropagator.ts
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is where the main change occurred, the rest are updating the existing code/tests to work with this.

Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { Context } from '../context/types';
*
* @since 1.0.0
*/
export interface TextMapPropagator<Carrier = any> {
export interface TextMapPropagator {
Copy link
Contributor Author

@chancancode chancancode Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change if you supplied a different type

/**
* Injects values from a given `Context` into a carrier.
*
Expand All @@ -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>(
Copy link
Contributor Author

@chancancode chancancode Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change for most implementors

context: Context,
carrier: Carrier,
setter: TextMapSetter<Carrier>
Expand All @@ -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>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change for most implementors

context: Context,
carrier: Carrier,
getter: TextMapGetter<Carrier>
Expand All @@ -79,7 +79,7 @@ export interface TextMapPropagator<Carrier = any> {
*
* @since 1.0.0
*/
export interface TextMapSetter<Carrier = any> {
export interface TextMapSetter<Carrier> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change for someone referencing this type

/**
* Callback used to set a key/value pair on an object.
*
Expand All @@ -99,7 +99,7 @@ export interface TextMapSetter<Carrier = any> {
*
* @since 1.0.0
*/
export interface TextMapGetter<Carrier = any> {
export interface TextMapGetter<Carrier> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking change for someone referencing this type

/**
* Get a list of all keys available on the carrier.
*
Expand All @@ -119,12 +119,12 @@ export interface TextMapGetter<Carrier = any> {
/**
* @since 1.0.0
*/
export const defaultTextMapGetter: TextMapGetter = {
export const defaultTextMapGetter: TextMapGetter<object | null | undefined> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type changes here is also somewhat of a breaking change, though, it probably wouldn't have worked in practice if you tried to use it with anything else.

get(carrier, key) {
if (carrier == null) {
return undefined;
}
return carrier[key];
return (carrier as Record<string, string>)[key];
},

keys(carrier) {
Expand All @@ -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;
},
};
75 changes: 32 additions & 43 deletions api/test/common/api/api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ import * as assert from 'assert';
import api, {
context,
Context,
defaultTextMapGetter,
defaultTextMapSetter,
diag,
metrics,
propagation,
Expand Down Expand Up @@ -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[] {
Expand All @@ -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', () => {
Expand Down
12 changes: 10 additions & 2 deletions doc/propagation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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;
Expand Down
54 changes: 42 additions & 12 deletions experimental/packages/opentelemetry-instrumentation-http/test/utils/DummyPropagation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
12 changes: 10 additions & 2 deletions packages/opentelemetry-core/src/baggage/propagation/W3CBaggagePropagator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down
Loading
Loading