From 26ae4c463e3fd660198076e98a3e7000c78db964 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Wed, 12 May 2021 11:47:16 -0400 Subject: [PATCH 1/4] chore: document the reason for symbol.for (#64) --- src/context/context.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/context/context.ts b/src/context/context.ts index ab058b85..825726f3 100644 --- a/src/context/context.ts +++ b/src/context/context.ts @@ -18,6 +18,12 @@ import { Context } from './types'; /** Get a key to uniquely identify a context value */ export function createContextKey(description: string) { + // The specification states that for the same input, multiple calls should + // return different keys. Due to the nature of the JS dependency management + // system, this creates problems where multiple versions of some package + // could hold different keys for the same property. + // + // Therefore, we use Symbol.for which returns the same key for the same input. return Symbol.for(description); } From 0a3bee49f487d2138c6ab9a2f6e4994faed314de Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Fri, 14 May 2021 03:22:06 -0400 Subject: [PATCH 2/4] chore: remove suppress instrumentation (#65) --- src/trace/context-utils.ts | 38 --------------- test/trace/context-utils.test.ts | 80 -------------------------------- 2 files changed, 118 deletions(-) delete mode 100644 test/trace/context-utils.test.ts diff --git a/src/trace/context-utils.ts b/src/trace/context-utils.ts index 4d8d2fe8..3714eb96 100644 --- a/src/trace/context-utils.ts +++ b/src/trace/context-utils.ts @@ -25,14 +25,6 @@ import { NonRecordingSpan } from './NonRecordingSpan'; */ const SPAN_KEY = createContextKey('OpenTelemetry Context Key SPAN'); -/** - * Shared key for indicating if instrumentation should be suppressed beyond - * this current scope. - */ -const SUPPRESS_INSTRUMENTATION_KEY = createContextKey( - 'OpenTelemetry Context Key SUPPRESS_INSTRUMENTATION' -); - /** * Return the span if one exists * @@ -74,33 +66,3 @@ export function setSpanContext( export function getSpanContext(context: Context): SpanContext | undefined { return getSpan(context)?.spanContext(); } - -/** - * Sets value on context to indicate that instrumentation should - * be suppressed beyond this current scope. - * - * @param context context to set the suppress instrumentation value on. - */ -export function suppressInstrumentation(context: Context): Context { - return context.setValue(SUPPRESS_INSTRUMENTATION_KEY, true); -} - -/** - * Sets value on context to indicate that instrumentation should - * no-longer be suppressed beyond this current scope. - * - * @param context context to set the suppress instrumentation value on. - */ -export function unsuppressInstrumentation(context: Context): Context { - return context.setValue(SUPPRESS_INSTRUMENTATION_KEY, false); -} - -/** - * Return current suppress instrumentation value for the given context, - * if it exists. - * - * @param context context check for the suppress instrumentation value. - */ -export function isInstrumentationSuppressed(context: Context): boolean { - return Boolean(context.getValue(SUPPRESS_INSTRUMENTATION_KEY)); -} diff --git a/test/trace/context-utils.test.ts b/test/trace/context-utils.test.ts deleted file mode 100644 index 1f24c34c..00000000 --- a/test/trace/context-utils.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Copyright The 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. - */ - -import * as assert from 'assert'; -import { - isInstrumentationSuppressed, - suppressInstrumentation, - unsuppressInstrumentation, -} from '../../src/trace/context-utils'; -import { createContextKey, ROOT_CONTEXT } from '../../src/context/context'; - -const SUPPRESS_INSTRUMENTATION_KEY = createContextKey( - 'OpenTelemetry Context Key SUPPRESS_INSTRUMENTATION' -); - -describe('Context Helpers', () => { - describe('suppressInstrumentation', () => { - it('should set suppress to true', () => { - const context = suppressInstrumentation(ROOT_CONTEXT); - assert.deepStrictEqual(isInstrumentationSuppressed(context), true); - }); - }); - - describe('unsuppressInstrumentation', () => { - it('should set suppress to false', () => { - const context = unsuppressInstrumentation(ROOT_CONTEXT); - assert.deepStrictEqual(isInstrumentationSuppressed(context), false); - }); - }); - - describe('isInstrumentationSuppressed', () => { - it('should get value as bool', () => { - const expectedValue = true; - const context = ROOT_CONTEXT.setValue( - SUPPRESS_INSTRUMENTATION_KEY, - expectedValue - ); - - const value = isInstrumentationSuppressed(context); - - assert.equal(value, expectedValue); - }); - - describe('when suppress instrumentation set to null', () => { - const context = ROOT_CONTEXT.setValue(SUPPRESS_INSTRUMENTATION_KEY, null); - - it('should return false', () => { - const value = isInstrumentationSuppressed(context); - - assert.equal(value, false); - }); - }); - - describe('when suppress instrumentation set to undefined', () => { - const context = ROOT_CONTEXT.setValue( - SUPPRESS_INSTRUMENTATION_KEY, - undefined - ); - - it('should return false', () => { - const value = isInstrumentationSuppressed(context); - - assert.equal(value, false); - }); - }); - }); -}); From 8435e0abe47e27b76ff6ad317311d64295816dc4 Mon Sep 17 00:00:00 2001 From: Valentin Marchaud Date: Fri, 14 May 2021 17:46:34 +0200 Subject: [PATCH 3/4] chore: move baggage methods in propagation namespace (#55) * chore: move baggage methods in propagation namespace * chore: add upgrade guidelines --- README.md | 1 + src/api/propagation.ts | 7 +++++++ src/index.ts | 2 +- test/baggage/Baggage.test.ts | 28 ++++++++++++++-------------- 4 files changed, 23 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index ff7a382b..d75aa41c 100644 --- a/README.md +++ b/README.md @@ -111,6 +111,7 @@ Because the npm installer and node module resolution algorithm could potentially - `HttpBaggage` renamed to `HttpBaggagePropagator` - [#45](https://github.com/open-telemetry/opentelemetry-js-api/pull/45) `Span#context` renamed to `Span#spanContext` - [#47](https://github.com/open-telemetry/opentelemetry-js-api/pull/47) `getSpan`/`setSpan`/`getSpanContext`/`setSpanContext` moved to `trace` namespace +- [#55](https://github.com/open-telemetry/opentelemetry-js-api/pull/55) `getBaggage`/`setBaggage`/`createBaggage` moved to `propagation` namespace ## Useful links diff --git a/src/api/propagation.ts b/src/api/propagation.ts index d83d4e7d..a554d2be 100644 --- a/src/api/propagation.ts +++ b/src/api/propagation.ts @@ -28,6 +28,7 @@ import { registerGlobal, unregisterGlobal, } from '../internal/global-utils'; +import { getBaggage, createBaggage, setBaggage } from '../baggage/index'; const API_NAME = 'propagation'; @@ -100,6 +101,12 @@ export class PropagationAPI { unregisterGlobal(API_NAME); } + public createBaggage = createBaggage; + + public getBaggage = getBaggage; + + public setBaggage = setBaggage; + private _getGlobalPropagator(): TextMapPropagator { return getGlobal(API_NAME) || NOOP_TEXT_MAP_PROPAGATOR; } diff --git a/src/index.ts b/src/index.ts index 34931412..29e6e7f5 100644 --- a/src/index.ts +++ b/src/index.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -export * from './baggage'; +export { baggageEntryMetadataFromString } from './baggage'; export * from './common/Exception'; export * from './common/Time'; export * from './diag'; diff --git a/test/baggage/Baggage.test.ts b/test/baggage/Baggage.test.ts index 45eb59d7..080b6a41 100644 --- a/test/baggage/Baggage.test.ts +++ b/test/baggage/Baggage.test.ts @@ -16,24 +16,22 @@ import * as assert from 'assert'; import { - createBaggage, - setBaggage, - getBaggage, ROOT_CONTEXT, + propagation, baggageEntryMetadataFromString, } from '../../src'; describe('Baggage', () => { describe('create', () => { it('should create an empty bag', () => { - const bag = createBaggage(); + const bag = propagation.createBaggage(); assert.deepStrictEqual(bag.getAllEntries(), []); }); it('should create a bag with entries', () => { const meta = baggageEntryMetadataFromString('opaque string'); - const bag = createBaggage({ + const bag = propagation.createBaggage({ key1: { value: 'value1' }, key2: { value: 'value2', metadata: meta }, }); @@ -47,7 +45,9 @@ describe('Baggage', () => { describe('get', () => { it('should not allow modification of returned entries', () => { - const bag = createBaggage().setEntry('key', { value: 'value' }); + const bag = propagation + .createBaggage() + .setEntry('key', { value: 'value' }); const entry = bag.getEntry('key'); assert.ok(entry); @@ -59,7 +59,7 @@ describe('Baggage', () => { describe('set', () => { it('should create a new bag when an entry is added', () => { - const bag = createBaggage(); + const bag = propagation.createBaggage(); const bag2 = bag.setEntry('key', { value: 'value' }); @@ -73,7 +73,7 @@ describe('Baggage', () => { describe('remove', () => { it('should create a new bag when an entry is removed', () => { - const bag = createBaggage({ + const bag = propagation.createBaggage({ key: { value: 'value' }, }); @@ -87,7 +87,7 @@ describe('Baggage', () => { }); it('should create an empty bag multiple keys are removed', () => { - const bag = createBaggage({ + const bag = propagation.createBaggage({ key: { value: 'value' }, key1: { value: 'value1' }, key2: { value: 'value2' }, @@ -107,7 +107,7 @@ describe('Baggage', () => { }); it('should create an empty bag when it cleared', () => { - const bag = createBaggage({ + const bag = propagation.createBaggage({ key: { value: 'value' }, key1: { value: 'value1' }, }); @@ -125,11 +125,11 @@ describe('Baggage', () => { describe('context', () => { it('should set and get a baggage from a context', () => { - const bag = createBaggage(); + const bag = propagation.createBaggage(); - const ctx = setBaggage(ROOT_CONTEXT, bag); + const ctx = propagation.setBaggage(ROOT_CONTEXT, bag); - assert.strictEqual(bag, getBaggage(ctx)); + assert.strictEqual(bag, propagation.getBaggage(ctx)); }); }); @@ -148,7 +148,7 @@ describe('Baggage', () => { }); it('should retain metadata', () => { - const bag = createBaggage({ + const bag = propagation.createBaggage({ key: { value: 'value', metadata: baggageEntryMetadataFromString('meta'), From 28fabc4916a1fdbd7d0e4f0dcdc57974d3b98088 Mon Sep 17 00:00:00 2001 From: Naseem Date: Mon, 17 May 2021 13:27:20 -0400 Subject: [PATCH 4/4] feat: add tracer.startActiveSpan() (#54) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Gerhard Stöbich Co-authored-by: Daniel Dyla --- src/trace/NoopTracer.ts | 42 ++++++++++++- src/trace/ProxyTracer.ts | 10 ++++ src/trace/tracer.ts | 59 +++++++++++++++++++ test/noop-implementations/noop-tracer.test.ts | 29 ++++++++- .../proxy-tracer.test.ts | 47 ++++++++++++--- 5 files changed, 177 insertions(+), 10 deletions(-) diff --git a/src/trace/NoopTracer.ts b/src/trace/NoopTracer.ts index 67eac54f..7f810522 100644 --- a/src/trace/NoopTracer.ts +++ b/src/trace/NoopTracer.ts @@ -14,8 +14,9 @@ * limitations under the License. */ -import { getSpanContext } from '../trace/context-utils'; +import { context } from '../'; import { Context } from '../context/types'; +import { getSpanContext, setSpan } from '../trace/context-utils'; import { NonRecordingSpan } from './NonRecordingSpan'; import { Span } from './span'; import { isSpanContextValid } from './spancontext-utils'; @@ -45,6 +46,45 @@ export class NoopTracer implements Tracer { return new NonRecordingSpan(); } } + + startActiveSpan ReturnType>( + name: string, + arg2: F | SpanOptions, + arg3?: F | Context, + arg4?: F + ): ReturnType | undefined { + let fn: F | undefined, + options: SpanOptions | undefined, + activeContext: Context | undefined; + if (arguments.length === 2 && typeof arg2 === 'function') { + fn = arg2; + } else if ( + arguments.length === 3 && + typeof arg2 === 'object' && + typeof arg3 === 'function' + ) { + options = arg2; + fn = arg3; + } else if ( + arguments.length === 4 && + typeof arg2 === 'object' && + typeof arg3 === 'object' && + typeof arg4 === 'function' + ) { + options = arg2; + activeContext = arg3; + fn = arg4; + } + + const parentContext = activeContext ?? context.active(); + const span = this.startSpan(name, options, parentContext); + const contextWithSpanSet = setSpan(parentContext, span); + + if (fn) { + return context.with(contextWithSpanSet, fn, undefined, span); + } + return; + } } function isSpanContext(spanContext: any): spanContext is SpanContext { diff --git a/src/trace/ProxyTracer.ts b/src/trace/ProxyTracer.ts index 05e55f2d..9cd399f9 100644 --- a/src/trace/ProxyTracer.ts +++ b/src/trace/ProxyTracer.ts @@ -38,6 +38,16 @@ export class ProxyTracer implements Tracer { return this._getTracer().startSpan(name, options, context); } + startActiveSpan unknown>( + _name: string, + _options: F | SpanOptions, + _context?: F | Context, + _fn?: F + ): ReturnType { + const tracer = this._getTracer(); + return Reflect.apply(tracer.startActiveSpan, tracer, arguments); + } + /** * Try to get a tracer from the proxy tracer provider. * If the proxy tracer provider has no delegate, return a noop tracer. diff --git a/src/trace/tracer.ts b/src/trace/tracer.ts index c5903c12..13c19ecc 100644 --- a/src/trace/tracer.ts +++ b/src/trace/tracer.ts @@ -37,4 +37,63 @@ export interface Tracer { * span.end(); */ startSpan(name: string, options?: SpanOptions, context?: Context): Span; + + /** + * Starts a new {@link Span} and calls the given function passing it the + * created span as first argument. + * Additionally the new span gets set in context and this context is activated + * for the duration of the function call. + * + * @param name The name of the span + * @param [options] SpanOptions used for span creation + * @param [context] Context to use to extract parent + * @param fn function called in the context of the span and receives the newly created span as an argument + * @returns return value of fn + * @example + * const something = tracer.startActiveSpan('op', span => { + * try { + * do some work + * span.setStatus({code: SpanStatusCode.OK}); + * return something; + * } catch (err) { + * span.setStatus({ + * code: SpanStatusCode.ERROR, + * message: err.message, + * }); + * throw err; + * } finally { + * span.end(); + * } + * }); + * @example + * const span = tracer.startActiveSpan('op', span => { + * try { + * do some work + * return span; + * } catch (err) { + * span.setStatus({ + * code: SpanStatusCode.ERROR, + * message: err.message, + * }); + * throw err; + * } + * }); + * do some more work + * span.end(); + */ + startActiveSpan unknown>( + name: string, + fn: F + ): ReturnType; + startActiveSpan unknown>( + name: string, + options: SpanOptions, + fn: F + ): ReturnType; + startActiveSpan unknown>( + name: string, + options: SpanOptions, + context: Context, + fn: F + ): ReturnType; } diff --git a/test/noop-implementations/noop-tracer.test.ts b/test/noop-implementations/noop-tracer.test.ts index 292b2fbb..0d4ca1a3 100644 --- a/test/noop-implementations/noop-tracer.test.ts +++ b/test/noop-implementations/noop-tracer.test.ts @@ -16,12 +16,13 @@ import * as assert from 'assert'; import { + context, NoopTracer, + Span, SpanContext, SpanKind, - TraceFlags, - context, trace, + TraceFlags, } from '../../src'; import { NonRecordingSpan } from '../../src/trace/NonRecordingSpan'; @@ -56,4 +57,28 @@ describe('NoopTracer', () => { assert(span.spanContext().spanId === parent.spanId); assert(span.spanContext().traceFlags === parent.traceFlags); }); + + it('should accept 2 to 4 args and start an active span', () => { + const tracer = new NoopTracer(); + const name = 'span-name'; + const fn = (span: Span) => { + try { + return 1; + } finally { + span.end(); + } + }; + const opts = { attributes: { foo: 'bar' } }; + const ctx = context.active(); + + const a = tracer.startActiveSpan(name, fn); + assert.strictEqual(a, 1); + + const b = tracer.startActiveSpan(name, opts, fn); + + assert.strictEqual(b, 1); + + const c = tracer.startActiveSpan(name, opts, ctx, fn); + assert.strictEqual(c, 1); + }); }); diff --git a/test/proxy-implementations/proxy-tracer.test.ts b/test/proxy-implementations/proxy-tracer.test.ts index c350913a..57e3e2dc 100644 --- a/test/proxy-implementations/proxy-tracer.test.ts +++ b/test/proxy-implementations/proxy-tracer.test.ts @@ -17,18 +17,18 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import { - ProxyTracerProvider, - SpanKind, - TracerProvider, - ProxyTracer, - Tracer, - Span, + context, NoopTracer, + ProxyTracer, + ProxyTracerProvider, ROOT_CONTEXT, + Span, + SpanKind, SpanOptions, + Tracer, + TracerProvider, } from '../../src'; import { NonRecordingSpan } from '../../src/trace/NonRecordingSpan'; - describe('ProxyTracer', () => { let provider: ProxyTracerProvider; const sandbox = sinon.createSandbox(); @@ -96,6 +96,10 @@ describe('ProxyTracer', () => { startSpan() { return delegateSpan; }, + + startActiveSpan() { + // stubbed + }, }; tracer = provider.getTracer('test'); @@ -114,6 +118,34 @@ describe('ProxyTracer', () => { assert.strictEqual(span, delegateSpan); }); + it('should create active spans using the delegate tracer', () => { + // sinon types are broken with overloads, hence the any + // https://github.com/DefinitelyTyped/DefinitelyTyped/issues/36436 + const startActiveSpanStub = sinon.stub( + delegateTracer, + 'startActiveSpan' + ); + + const name = 'span-name'; + const fn = (span: Span) => { + try { + return 1; + } finally { + span.end(); + } + }; + const opts = { attributes: { foo: 'bar' } }; + const ctx = context.active(); + + startActiveSpanStub.withArgs(name, fn).returns(1); + startActiveSpanStub.withArgs(name, opts, fn).returns(2); + startActiveSpanStub.withArgs(name, opts, ctx, fn).returns(3); + + assert.strictEqual(tracer.startActiveSpan(name, fn), 1); + assert.strictEqual(tracer.startActiveSpan(name, opts, fn), 2); + assert.strictEqual(tracer.startActiveSpan(name, opts, ctx, fn), 3); + }); + it('should pass original arguments to DelegateTracer#startSpan', () => { const startSpanStub = sandbox.stub(delegateTracer, 'startSpan'); @@ -130,6 +162,7 @@ describe('ProxyTracer', () => { assert.deepStrictEqual(Object.getOwnPropertyNames(NoopTracer.prototype), [ 'constructor', 'startSpan', + 'startActiveSpan', ]); sandbox.assert.calledOnceWithExactly(startSpanStub, name, options, ctx); });