Add getter and setter arguments to propagation API #827
Conversation
dyladan
requested review from
bg451,
markwolff,
mayurkale22,
obecny,
OlivierAlbertini,
rochdev and
vmarchaud
as code owners
Codecov Report
@@ Coverage Diff @@
## master #827 +/- ##
==========================================
- Coverage 94.47% 92.55% -1.92%
==========================================
Files 249 251 +2
Lines 11005 10977 -28
Branches 1059 1061 +2
==========================================
- Hits 10397 10160 -237
- Misses 608 817 +209
|
| * @param context Context carrying tracing data to inject. Defaults to the currently active context. | ||
| */ | ||
| public inject(carrier: Carrier, context = contextApi.active()): void { | ||
| return this._propagator.inject(context, carrier); | ||
| public inject<Carrier = any>( |
There was a problem hiding this comment.
would it be better to put unknown instead of any if we need to modify those functions later ?
packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts
Outdated
Show resolved
Hide resolved
|
@open-telemetry/javascript-approvers we need more reviews on this! |
…telemetry-js into getter-setter
| * @param context Context carrying tracing data to inject. Defaults to the currently active context. | ||
| */ | ||
| public inject(carrier: Carrier, context = contextApi.active()): void { | ||
| return this._propagator.inject(context, carrier); | ||
| public inject<Carrier>( |
There was a problem hiding this comment.
why the order of params has changed ? I see later the order is correct (carrier, context, setter)
There was a problem hiding this comment.
It is based on which arguments are most likely to be omitted. The most common use case is extract(carrier), the second most common is extract(carrier, getter), the third most common is extract(carrier, getter, context).
The order of the actual propagator itself has context first because that was the order before. Originally, before the global API, it was inject(spanContext, format, carrier). Format has been removed, but the other order was preserved.
There was a problem hiding this comment.
Hmm I'm a bit confused with this as for for interface HttpTextFormat the order is (carrier, context, setter/getter), maybe I'm wrong but my understanding is that it should be the same in both cases ?
There was a problem hiding this comment.
One is the order of the public API, which I have ordered for the reasons above (some arguments optional and unlikely to be used). The other is the order of the interface, which accepts all arguments. I kept the order of the interface for now, but it can be made consistent in a future PR if we want.
* feat: add getter and setter arguments to propagation API * chore: add getter and setter to composite propagator
* feat: add getter and setter arguments to propagation API * chore: add getter and setter to composite propagator
* docs: readme instrumentations aws-sdk and fastify * docs: add instrumentation-long-task to readme * docs: sort web instrumentations alphbetically
Fixes #822
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md#setter-argument
https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-propagators.md#getter-argument
Primary benefit of this can be seen in the grpc plugin https://github.com/open-telemetry/opentelemetry-js/pull/827/files#diff-671cbf0fed27b96717e412e39a4dfc30