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.

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

Add getter and setter arguments to propagation API #827

Merged
merged 11 commits into from
Mar 5, 2020
21 changes: 16 additions & 5 deletions packages/opentelemetry-api/src/api/propagation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@
*/

import { Context } from '@opentelemetry/scope-base';
import { Carrier } from '../context/propagation/carrier';
import { defaultGetter, GetterFunction } from '../context/propagation/getter';
import { HttpTextFormat } from '../context/propagation/HttpTextFormat';
import { NOOP_HTTP_TEXT_FORMAT } from '../context/propagation/NoopHttpTextFormat';
import { defaultSetter, SetterFunction } from '../context/propagation/setter';
import { ContextAPI } from './context';

const contextApi = ContextAPI.getInstance();
Expand Down Expand Up @@ -53,19 +54,29 @@ export class PropagationAPI {
* Inject context into a carrier to be propagated inter-process
*
* @param carrier carrier to inject context into
* @param setter Function used to set values on the carrier
* @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>(
Copy link
Member

Choose a reason for hiding this comment

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

why the order of params has changed ? I see later the order is correct (carrier, context, setter)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

carrier: Carrier,
setter: SetterFunction<Carrier> = defaultSetter,
context = contextApi.active()
): void {
return this._propagator.inject(context, carrier, setter);
}

/**
* Extract context from a carrier
*
* @param carrier Carrier to extract context from
* @param getter Function used to extract keys from a carrier
* @param context Context which the newly created context will inherit from. Defaults to the currently active context.
*/
public extract(carrier: Carrier, context = contextApi.active()): Context {
return this._propagator.extract(context, carrier);
public extract<Carrier>(
carrier: Carrier,
dyladan marked this conversation as resolved.
Show resolved Hide resolved
getter: GetterFunction<Carrier> = defaultGetter,
context = contextApi.active()
): Context {
return this._propagator.extract(context, carrier, getter);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
*/

import { Context } from '@opentelemetry/scope-base';
import { Carrier } from './carrier';
import { SetterFunction } from './setter';
import { GetterFunction } from './getter';

/**
* Injects {@link Context} into and extracts it from carriers that travel
Expand All @@ -38,7 +39,7 @@ export interface HttpTextFormat {
* @param carrier the carrier of propagation fields, such as http request
* headers.
*/
inject(context: Context, carrier: Carrier): void;
inject(context: Context, carrier: unknown, setter: SetterFunction): void;
dyladan marked this conversation as resolved.
Show resolved Hide resolved

/**
* Given a {@link Context} and a carrier, extract context values from a
Expand All @@ -50,5 +51,5 @@ export interface HttpTextFormat {
* @param carrier the carrier of propagation fields, such as http request
* headers.
*/
extract(context: Context, carrier: Carrier): Context;
extract(context: Context, carrier: unknown, getter: GetterFunction): Context;
dyladan marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,16 @@
*/

import { Context } from '@opentelemetry/scope-base';
import { Carrier } from './carrier';
import { HttpTextFormat } from './HttpTextFormat';

/**
* No-op implementations of {@link HttpTextFormat}.
*/
export class NoopHttpTextFormat implements HttpTextFormat {
/** Noop inject function does nothing */
inject(context: Context, carrier: Carrier): void {}
inject(context: Context, carrier: unknown, setter: Function): void {}
/** Noop extract function does nothing and returns the input context */
extract(context: Context, carrier: Carrier): Context {
extract(context: Context, carrier: unknown, getter: Function): Context {
return context;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
* limitations under the License.
*/

export type Carrier = {
[key: string]: unknown;
};
export type GetterFunction<Carrier = any> = (
carrier: Carrier,
key: string
) => unknown;

export function defaultGetter(carrier: any, key: string): unknown {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
return carrier[key];
dyladan marked this conversation as resolved.
Show resolved Hide resolved
}
25 changes: 25 additions & 0 deletions packages/opentelemetry-api/src/context/propagation/setter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*!
* Copyright 2020, OpenTelemetry Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

export type SetterFunction<Carrier = any> = (
carrier: Carrier,
key: string,
value: unknown
dyladan marked this conversation as resolved.
Show resolved Hide resolved
) => void;

export function defaultSetter(carrier: any, key: string, value: unknown) {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
carrier[key] = value;
}
3 changes: 2 additions & 1 deletion packages/opentelemetry-api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@

export * from './common/Logger';
export * from './common/Time';
export * from './context/propagation/carrier';
export * from './context/propagation/getter';
export * from './context/propagation/HttpTextFormat';
export * from './context/propagation/setter';
export * from './distributed_context/DistributedContext';
export * from './distributed_context/EntryValue';
export * from './metrics/BoundInstrument';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
* limitations under the License.
*/

import { Context } from '@opentelemetry/scope-base';
import * as assert from 'assert';
import { NoopTracer, NOOP_SPAN, SpanKind } from '../../src';
import { Context } from '@opentelemetry/scope-base';
import { defaultGetter } from '../../src/context/propagation/getter';
import { defaultSetter } from '../../src/context/propagation/setter';

describe('NoopTracer', () => {
it('should not crash', () => {
Expand All @@ -39,9 +41,9 @@ describe('NoopTracer', () => {
const httpTextFormat = tracer.getHttpTextFormat();
assert.ok(httpTextFormat);

httpTextFormat.inject(Context.ROOT_CONTEXT, {});
httpTextFormat.inject(Context.ROOT_CONTEXT, {}, defaultSetter);
assert.deepStrictEqual(
httpTextFormat.extract(Context.ROOT_CONTEXT, {}),
httpTextFormat.extract(Context.ROOT_CONTEXT, {}, defaultGetter),
Context.ROOT_CONTEXT
);
});
Expand Down
19 changes: 10 additions & 9 deletions packages/opentelemetry-core/src/context/propagation/B3Format.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@
*/

import {
Carrier,
Context,
GetterFunction,
HttpTextFormat,
SetterFunction,
TraceFlags,
} from '@opentelemetry/api';
import { getParentSpanContext, setExtractedSpanContext } from '../context';
Expand All @@ -42,29 +43,29 @@ function isValidSpanId(spanId: string): boolean {
* Based on: https://github.com/openzipkin/b3-propagation
*/
export class B3Format implements HttpTextFormat {
inject(context: Context, carrier: Carrier) {
inject(context: Context, carrier: unknown, setter: SetterFunction) {
const spanContext = getParentSpanContext(context);
if (!spanContext) return;

if (
isValidTraceId(spanContext.traceId) &&
isValidSpanId(spanContext.spanId)
) {
carrier[X_B3_TRACE_ID] = spanContext.traceId;
carrier[X_B3_SPAN_ID] = spanContext.spanId;
setter(carrier, X_B3_TRACE_ID, spanContext.traceId);
setter(carrier, X_B3_SPAN_ID, spanContext.spanId);

// We set the header only if there is an existing sampling decision.
// Otherwise we will omit it => Absent.
if (spanContext.traceFlags !== undefined) {
carrier[X_B3_SAMPLED] = Number(spanContext.traceFlags);
setter(carrier, X_B3_SAMPLED, Number(spanContext.traceFlags));
}
}
}

extract(context: Context, carrier: Carrier): Context {
const traceIdHeader = carrier[X_B3_TRACE_ID];
const spanIdHeader = carrier[X_B3_SPAN_ID];
const sampledHeader = carrier[X_B3_SAMPLED];
extract(context: Context, carrier: unknown, getter: GetterFunction): Context {
dyladan marked this conversation as resolved.
Show resolved Hide resolved
const traceIdHeader = getter(carrier, X_B3_TRACE_ID);
const spanIdHeader = getter(carrier, X_B3_SPAN_ID);
const sampledHeader = getter(carrier, X_B3_SAMPLED);
if (!traceIdHeader || !spanIdHeader) return context;
const traceId = Array.isArray(traceIdHeader)
? traceIdHeader[0]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@
*/

import {
Carrier,
Context,
HttpTextFormat,
SpanContext,
TraceFlags,
} from '@opentelemetry/api';
import { GetterFunction } from '@opentelemetry/api';
import { SetterFunction } from '@opentelemetry/api';
dyladan marked this conversation as resolved.
Show resolved Hide resolved
import { TraceState } from '../../trace/TraceState';
import { getParentSpanContext, setExtractedSpanContext } from '../context';

Expand Down Expand Up @@ -63,22 +64,22 @@ export function parseTraceParent(traceParent: string): SpanContext | null {
* https://www.w3.org/TR/trace-context/
*/
export class HttpTraceContext implements HttpTextFormat {
inject(context: Context, carrier: Carrier) {
inject(context: Context, carrier: unknown, setter: SetterFunction) {
const spanContext = getParentSpanContext(context);
if (!spanContext) return;

const traceParent = `${VERSION}-${spanContext.traceId}-${
spanContext.spanId
}-0${Number(spanContext.traceFlags || TraceFlags.UNSAMPLED).toString(16)}`;

carrier[TRACE_PARENT_HEADER] = traceParent;
setter(carrier, TRACE_PARENT_HEADER, traceParent);
if (spanContext.traceState) {
carrier[TRACE_STATE_HEADER] = spanContext.traceState.serialize();
setter(carrier, TRACE_STATE_HEADER, spanContext.traceState.serialize());
}
}

extract(context: Context, carrier: Carrier): Context {
const traceParentHeader = carrier[TRACE_PARENT_HEADER];
extract(context: Context, carrier: unknown, getter: GetterFunction): Context {
const traceParentHeader = getter(carrier, TRACE_PARENT_HEADER);
if (!traceParentHeader) return context;
const traceParent = Array.isArray(traceParentHeader)
? traceParentHeader[0]
Expand All @@ -88,7 +89,7 @@ export class HttpTraceContext implements HttpTextFormat {

spanContext.isRemote = true;

const traceStateHeader = carrier[TRACE_STATE_HEADER];
const traceStateHeader = getter(carrier, TRACE_STATE_HEADER);
if (traceStateHeader) {
// If more than one `tracestate` header is found, we merge them into a
// single header.
Expand Down
Loading