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

feat: add keys operation to getter #1576

Merged
merged 11 commits into from
Oct 19, 2020
2 changes: 1 addition & 1 deletion packages/opentelemetry-api/src/api/global-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,4 @@ export function makeGetter<T>(
* version. If the global API is not compatible with the API package
* attempting to get it, a NOOP API implementation will be returned.
*/
export const API_BACKWARDS_COMPATIBILITY_VERSION = 0;
export const API_BACKWARDS_COMPATIBILITY_VERSION = 1;
14 changes: 9 additions & 5 deletions packages/opentelemetry-api/src/api/propagation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@
*/

import { Context } from '@opentelemetry/context-base';
import { defaultGetter, GetterFunction } from '../context/propagation/getter';
import { TextMapPropagator } from '../context/propagation/TextMapPropagator';
import { NOOP_TEXT_MAP_PROPAGATOR } from '../context/propagation/NoopTextMapPropagator';
import { defaultSetter, SetterFunction } from '../context/propagation/setter';
import {
defaultGetter,
defaultSetter,
Getter,
Setter,
TextMapPropagator,
} from '../context/propagation/TextMapPropagator';
import { ContextAPI } from './context';
import {
API_BACKWARDS_COMPATIBILITY_VERSION,
Expand Down Expand Up @@ -74,7 +78,7 @@ export class PropagationAPI {
*/
public inject<Carrier>(
carrier: Carrier,
setter: SetterFunction<Carrier> = defaultSetter,
setter: Setter<Carrier> = defaultSetter,
context: Context = contextApi.active()
): void {
return this._getGlobalPropagator().inject(context, carrier, setter);
Expand All @@ -89,7 +93,7 @@ export class PropagationAPI {
*/
public extract<Carrier>(
carrier: Carrier,
getter: GetterFunction<Carrier> = defaultGetter,
getter: Getter<Carrier> = defaultGetter,
context: Context = contextApi.active()
): Context {
return this._getGlobalPropagator().extract(context, carrier, getter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import { TextMapPropagator } from './TextMapPropagator';
*/
export class NoopTextMapPropagator implements TextMapPropagator {
/** Noop inject function does nothing */
inject(context: Context, carrier: unknown, setter: Function): void {}
inject(context: Context, carrier: unknown): void {}
/** Noop extract function does nothing and returns the input context */
extract(context: Context, carrier: unknown, getter: Function): Context {
extract(context: Context, carrier: unknown): Context {
return context;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
*/

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

/**
* Injects `Context` into and extracts it from carriers that travel
Expand All @@ -29,7 +27,7 @@ import { GetterFunction } from './getter';
* usually implemented via library-specific request interceptors, where the
* client-side injects values and the server-side extracts them.
*/
export interface TextMapPropagator {
export interface TextMapPropagator<Carrier = any> {
/**
* Injects values from a given `Context` into a carrier.
*
Expand All @@ -40,10 +38,10 @@ export interface TextMapPropagator {
* the wire.
* @param carrier the carrier of propagation fields, such as http request
* headers.
* @param setter a function which accepts a carrier, key, and value, which
* sets the key on the carrier to the value.
* @param setter an optional {@link Setter}. If undefined, values will be
* set by direct object assignment.
*/
inject(context: Context, carrier: unknown, setter: SetterFunction): void;
inject(context: Context, carrier: Carrier, setter: Setter<Carrier>): void;

/**
* Given a `Context` and a carrier, extract context values from a
Expand All @@ -54,8 +52,73 @@ export interface TextMapPropagator {
* the wire.
* @param carrier the carrier of propagation fields, such as http request
* headers.
* @param getter a function which accepts a carrier and a key, and returns
* the value from the carrier identified by the key.
* @param getter an optional {@link Getter}. If undefined, keys will be all
* own properties, and keys will be accessed by direct object access.
*/
extract(context: Context, carrier: unknown, getter: GetterFunction): Context;
extract(context: Context, carrier: Carrier, getter: Getter<Carrier>): Context;
}

/**
* A setter is specified by the caller to define a specific method
* to set key/value pairs on the carrier within a propagator.
*/
export interface Setter<Carrier = any> {
Copy link
Member

Choose a reason for hiding this comment

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

should the interfaces lives in types file or maybe the Noop ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The interface was originally in its own file, but I think it makes sense to keep it more tightly coupled to the TextMapPropagator since this getter is specifically for this type of propagator. If other propagator types are added in the future like binary they may have different interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with keeping the interfaces together with class if they are not exported or keeping them in some general file like types.ts. Otherwise I would rather keep them in separate file or in general types.ts as it quickly gets messy later with any refactoring or when trying to find the desired interface.
Because of that:

If other propagator types are added in the future like binary they may have different interfaces.

Can you change names:

  • Setter into TextMapSetter
  • Getter into TextMapGetter
  • defaultGetter into defaultTextMapGetter

and then mentioned types.ts should become textMapTypes.ts ?
WDYT ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The TextMapPropagator is already just an interface. Won't it be weird to have a types file, but then also have some types (the TextMapPropagator interface) defined in a different file? I agree with the renaming though.

Copy link
Member

Choose a reason for hiding this comment

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

you right it is interface already I was blind :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I did the rename as I think that is a good idea to prevent future incompatibilities

/**
* Callback used to set a key/value pair on an object.
*
* Should be called by the propagator each time a key/value pair
* should be set, and should set that key/value pair on the propagator.
*
* @param carrier object or class which carries key/value pairs
* @param key string key to modify
* @param value value to be set to the key on the carrier
*/
set(carrier: Carrier, key: string, value: string): void;
}

/**
* A getter is specified by the caller to define a specific method
* to get the value of a key from a carrier.
*/
export interface Getter<Carrier = any> {
/**
* Get a list of all keys available on the carrier.
*
* @param carrier
*/
keys(carrier: Carrier): string[];

/**
* Get the value of a specific key from the carrier.
*
* @param carrier
* @param key
*/
get(carrier: Carrier, key: string): undefined | string | string[];
}

export const defaultGetter: Getter = {
get(carrier, key) {
if (carrier == null) {
return undefined;
}
return carrier[key];
},

keys(carrier) {
if (carrier == null) {
return [];
}
return Object.keys(carrier);
},
};

export const defaultSetter: Setter = {
set(carrier, key, value) {
if (carrier == null) {
return;
}

carrier[key] = value;
},
};
31 changes: 0 additions & 31 deletions packages/opentelemetry-api/src/context/propagation/getter.ts

This file was deleted.

31 changes: 0 additions & 31 deletions packages/opentelemetry-api/src/context/propagation/setter.ts

This file was deleted.

2 changes: 0 additions & 2 deletions packages/opentelemetry-api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@ export * from './common/Exception';
export * from './common/Logger';
export * from './common/Time';
export * from './context/context';
export * from './context/propagation/getter';
export * from './context/propagation/TextMapPropagator';
export * from './context/propagation/NoopTextMapPropagator';
export * from './context/propagation/setter';
export * from './correlation_context/CorrelationContext';
export * from './correlation_context/EntryValue';
export * from './metrics/BatchObserverResult';
Expand Down
38 changes: 16 additions & 22 deletions packages/opentelemetry-core/src/context/propagation/B3Propagator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@

import {
Context,
GetterFunction,
Getter,
TextMapPropagator,
SetterFunction,
Setter,
TraceFlags,
getParentSpanContext,
setExtractedSpanContext,
Expand Down Expand Up @@ -63,49 +63,43 @@ function parseHeader(header: unknown) {
return Array.isArray(header) ? header[0] : header;
}

function getHeaderValue(carrier: unknown, getter: GetterFunction, key: string) {
const header = getter(carrier, key);
function getHeaderValue(carrier: unknown, getter: Getter, key: string) {
const header = getter.get(carrier, key);
return parseHeader(header);
}

function getTraceId(carrier: unknown, getter: GetterFunction): string {
function getTraceId(carrier: unknown, getter: Getter): string {
const traceId = getHeaderValue(carrier, getter, X_B3_TRACE_ID);
if (typeof traceId === 'string') {
return traceId.padStart(32, '0');
}
return '';
}

function getSpanId(carrier: unknown, getter: GetterFunction): string {
function getSpanId(carrier: unknown, getter: Getter): string {
const spanId = getHeaderValue(carrier, getter, X_B3_SPAN_ID);
if (typeof spanId === 'string') {
return spanId;
}
return '';
}

function getParentSpanId(
carrier: unknown,
getter: GetterFunction
): string | undefined {
function getParentSpanId(carrier: unknown, getter: Getter): string | undefined {
const spanId = getHeaderValue(carrier, getter, X_B3_PARENT_SPAN_ID);
if (typeof spanId === 'string') {
return spanId;
}
return;
}

function getDebug(
carrier: unknown,
getter: GetterFunction
): string | undefined {
function getDebug(carrier: unknown, getter: Getter): string | undefined {
const debug = getHeaderValue(carrier, getter, X_B3_FLAGS);
return debug === '1' ? '1' : undefined;
}

function getTraceFlags(
carrier: unknown,
getter: GetterFunction
getter: Getter
): TraceFlags | undefined {
const traceFlags = getHeaderValue(carrier, getter, X_B3_SAMPLED);
const debug = getDebug(carrier, getter);
Expand All @@ -124,7 +118,7 @@ function getTraceFlags(
* Based on: https://github.com/openzipkin/b3-propagation
*/
export class B3Propagator implements TextMapPropagator {
inject(context: Context, carrier: unknown, setter: SetterFunction) {
inject(context: Context, carrier: unknown, setter: Setter) {
const spanContext = getParentSpanContext(context);
if (!spanContext) return;
const parentSpanId = context.getValue(PARENT_SPAN_ID_KEY) as
Expand All @@ -136,19 +130,19 @@ export class B3Propagator implements TextMapPropagator {
isValidParentSpanID(parentSpanId)
) {
const debug = context.getValue(DEBUG_FLAG_KEY);
setter(carrier, X_B3_TRACE_ID, spanContext.traceId);
setter(carrier, X_B3_SPAN_ID, spanContext.spanId);
setter.set(carrier, X_B3_TRACE_ID, spanContext.traceId);
setter.set(carrier, X_B3_SPAN_ID, spanContext.spanId);
if (parentSpanId) {
setter(carrier, X_B3_PARENT_SPAN_ID, parentSpanId);
setter.set(carrier, X_B3_PARENT_SPAN_ID, parentSpanId);
}
// According to the B3 spec, if the debug flag is set,
// the sampled flag shouldn't be propagated as well.
if (debug === '1') {
setter(carrier, X_B3_FLAGS, debug);
setter.set(carrier, X_B3_FLAGS, debug);
} else if (spanContext.traceFlags !== undefined) {
// We set the header only if there is an existing sampling decision.
// Otherwise we will omit it => Absent.
setter(
setter.set(
carrier,
X_B3_SAMPLED,
(TraceFlags.SAMPLED & spanContext.traceFlags) === TraceFlags.SAMPLED
Expand All @@ -159,7 +153,7 @@ export class B3Propagator implements TextMapPropagator {
}
}

extract(context: Context, carrier: unknown, getter: GetterFunction): Context {
extract(context: Context, carrier: unknown, getter: Getter): Context {
const traceId = getTraceId(carrier, getter);
const spanId = getSpanId(carrier, getter);
const parentSpanId = getParentSpanId(carrier, getter);
Expand Down
Loading