feat(api)!: Move TextMapPropagator<Carrier> generic parameter to interface methods
#5368
Conversation
There was a problem hiding this comment.
This file is where the main change occurred, the rest are updating the existing code/tests to work with this.
chancancode
force-pushed
the
fix-propagator-generic
branch
from
48e1372 to
9e5fdb2
Compare
chancancode
changed the title
TextMapPropagator TypeScript interfaceTextMapPropagator Carrier generic parameter, in favor of generics on its methods
chancancode
changed the title
TextMapPropagator Carrier generic parameter, in favor of generics on its methodsTextMapPropagator<Carrier> generic parameter, in favor of generics on its methods
| @@ -119,12 +119,12 @@ export interface TextMapGetter<Carrier = any> { | |||
| /** | |||
| * @since 1.0.0 | |||
| */ | |||
| export const defaultTextMapGetter: TextMapGetter = { | |||
| export const defaultTextMapGetter: TextMapGetter<object | null | undefined> = { | |||
There was a problem hiding this comment.
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.
| @@ -29,7 +29,7 @@ import { Context } from '../context/types'; | |||
| * | |||
| * @since 1.0.0 | |||
| */ | |||
| export interface TextMapPropagator<Carrier = any> { | |||
| export interface TextMapPropagator { | |||
There was a problem hiding this comment.
Breaking change if you supplied a different type
| @@ -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>( | |||
There was a problem hiding this comment.
Breaking change for most implementors
| @@ -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>( | |||
There was a problem hiding this comment.
Breaking change for most implementors
| @@ -79,7 +79,7 @@ export interface TextMapPropagator<Carrier = any> { | |||
| * | |||
| * @since 1.0.0 | |||
| */ | |||
| export interface TextMapSetter<Carrier = any> { | |||
| export interface TextMapSetter<Carrier> { | |||
There was a problem hiding this comment.
Breaking change for someone referencing this type
| @@ -99,7 +99,7 @@ export interface TextMapSetter<Carrier = any> { | |||
| * | |||
| * @since 1.0.0 | |||
| */ | |||
| export interface TextMapGetter<Carrier = any> { | |||
| export interface TextMapGetter<Carrier> { | |||
There was a problem hiding this comment.
Breaking change for someone referencing this type
|
@dyladan @pichlermarc @trentm I'd like to nominate for this chance to be considered for 2.0. For correct implementations, it shouldn't be a hard change to absorb, though it requires some manual work, but it would be a pain to figure out how to resolve this when not coincided with a major version bump. We could also consider what is our policy for breaking changes that purely happen within type space, which better enforces semantics that are already expected in practice (type-level bug fixes). There is an argument to be made that those kind of breakages are different, because if you aren't using TypeScript, it won't affect you, and if you do, the compiler would have caught it, and it's not like it would accidentally slip runtime behavior changes into production code behind your back. Anyway, if we can land it in time for 2.0, we won't have to litigate that now. |
chancancode
changed the title
TextMapPropagator<Carrier> generic parameter, in favor of generics on its methodsTextMapPropagator<Carrier> generic parameter to interface methods
| context: Context, | ||
| carrier: Carrier, | ||
| setter: TextMapSetter<Carrier> | ||
| ): void; |
There was a problem hiding this comment.
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.
The SDK 2.0 efforts only apply to stable (1.x) packages in this repo, with the exception of
Both have different release cycles, and the API has some exemptions around breaking changes for implementers, but I don't think this would be covered by it. For the API in particular, bumping it to 2.0 would mean that we'd have to support the 1.x API for 3 years after that - we're not ready to make such a change yet and we would have to figure out a lot of details around that before we can make such a change. That being said, I think your change makes sense - it's just not the right time for it. |
|
Ah I see, we can keep this on ice for now then. Happy to close it; or put some kind of label/milestone on it? As I said I think there is a good argument for treating type-level breakages differently than runtime code (every type-level bug fix is arguably a breaking change to an extent), so if we want to have that discussion some day that may open the doors for this change as well. Though even then, this may be too big of a change still. |
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
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
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
|
In the meantime, moving the non-breaking parts to #5370 |
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
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 open-telemetry#5365
chancancode
force-pushed
the
fix-propagator-generic
branch
from
9e5fdb2 to
483f2a0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5368 +/- ##
==========================================
+ Coverage 94.54% 94.62% +0.07%
==========================================
Files 318 318
Lines 8052 8052
Branches 1694 1694
==========================================
+ Hits 7613 7619 +6
+ Misses 439 433 -6
|
Which problem is this PR solving?
The previous interface placed the generic paramter on the interface itself. This implies that the implementors of
TextMapPropagatorcan specify what carrier types it accepts (and that each implementor only work with one specific type of carrier).In reality, 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 layers (caller of
inject/extract) is the one who has control over what the carrier type is, and have no idea what propagator is in use. 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 propagation API with one of many carrier types – POJOs,
Headers,Map, etc.Therefore, a correct implementation of
TextMapPropagatormust 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.
Ref #5365
Short description of the changes
This commit moves the generic parameter from the interface into the
injectandextractmethods. This forces implementors to comply with the API more rigorously.When implementing the interface, the implementor cannot choose what the type of
carrieris. The fact that the generic parameter exists on the interface method means that, when implementing the method, the implementor cannot make any assumptions about thecarriermethod parameter and cannot directly interact it, except through thegetter/setter, which is guaranteed to be compatible with the givencarrier.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.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: