From 4a2deb28b88291c80e3da778dc2f02b26207b2d9 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Wed, 8 Apr 2020 15:38:17 +0200 Subject: [PATCH 1/6] feat(http-plugin): add options to disable new spans if no parent #931 --- packages/opentelemetry-plugin-http/README.md | 2 + .../opentelemetry-plugin-http/src/http.ts | 54 +++++++++++-- .../opentelemetry-plugin-http/src/types.ts | 4 + .../test/functionals/http-enable.test.ts | 79 +++++++++++++++++++ 4 files changed, 132 insertions(+), 7 deletions(-) diff --git a/packages/opentelemetry-plugin-http/README.md b/packages/opentelemetry-plugin-http/README.md index 8daf06f844..bcdc59e230 100644 --- a/packages/opentelemetry-plugin-http/README.md +++ b/packages/opentelemetry-plugin-http/README.md @@ -55,6 +55,8 @@ Http plugin has few options available to choose from. You can set the following: | [`ignoreIncomingPaths`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `IgnoreMatcher[]` | Http plugin will not trace all incoming requests that match paths | | [`ignoreOutgoingUrls`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `IgnoreMatcher[]` | Http plugin will not trace all outgoing requests that match urls | | [`serverName`](https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-plugin-http/src/types.ts#L28) | `string` | The primary server name of the matched virtual host. | +| `requireParentforOutgoingSpans` | Boolean | Require that is a parent span to create new span for outgoing requests. | +| `requireParentforIncomingSpans` | Boolean | Require that is a parent span to create new span for incoming requests. | ## Useful links - For more information on OpenTelemetry, visit: diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts index 51263cc89e..3db015e8e3 100644 --- a/packages/opentelemetry-plugin-http/src/http.ts +++ b/packages/opentelemetry-plugin-http/src/http.ts @@ -22,8 +22,15 @@ import { SpanKind, SpanOptions, Status, + SpanContext, + TraceFlags, + NoopSpan, } from '@opentelemetry/api'; -import { BasePlugin } from '@opentelemetry/core'; +import { + BasePlugin, + NoRecordingSpan, + getExtractedSpanContext, +} from '@opentelemetry/core'; import { ClientRequest, IncomingMessage, @@ -57,6 +64,12 @@ export class HttpPlugin extends BasePlugin { /** keep track on spans not ended */ private readonly _spanNotEnded: WeakSet; + private readonly _emptySpanContext: SpanContext = { + traceId: '', + spanId: '', + traceFlags: TraceFlags.NONE, + }; + constructor(readonly moduleName: string, readonly version: string) { super(`@opentelemetry/plugin-${moduleName}`, VERSION); // For now component is equal to moduleName but it can change in the future. @@ -295,10 +308,23 @@ export class HttpPlugin extends BasePlugin { }; return context.with(propagation.extract(headers), () => { - const span = plugin._startHttpSpan( - `${method} ${pathname}`, - spanOptions - ); + let span: Span = new NoopSpan(); + const hasParent = plugin._tracer.getCurrentSpan() !== undefined; + /* + * If a parent is required but not present, we use a `NoRecordingSpan` to still + * propagate context without recording it. + */ + if ( + plugin._config.requireParentforIncomingSpans === true && + hasParent === false + ) { + const spanContext = + getExtractedSpanContext(context.active()) ?? + plugin._emptySpanContext; + span = new NoRecordingSpan(spanContext); + } else { + span = plugin._startHttpSpan(`${method} ${pathname}`, spanOptions); + } return plugin._tracer.withSpan(span, () => { context.bind(request); @@ -396,8 +422,22 @@ export class HttpPlugin extends BasePlugin { const spanOptions: SpanOptions = { kind: SpanKind.CLIENT, }; - - const span = plugin._startHttpSpan(operationName, spanOptions); + const hasParent = plugin._tracer.getCurrentSpan() !== undefined; + let span: Span = new NoopSpan(); + /* + * If a parent is required but not present, we use a `NoRecordingSpan` to still + * propagate context without recording it. + */ + if ( + plugin._config.requireParentforOutgoingSpans === true && + hasParent === false + ) { + const spanContext = + getExtractedSpanContext(context.active()) ?? plugin._emptySpanContext; + span = new NoRecordingSpan(spanContext); + } else { + span = plugin._startHttpSpan(operationName, spanOptions); + } return plugin._tracer.withSpan(span, () => { if (!optionsParsed.headers) optionsParsed.headers = {}; diff --git a/packages/opentelemetry-plugin-http/src/types.ts b/packages/opentelemetry-plugin-http/src/types.ts index 335e0bbc30..b7e230d37a 100644 --- a/packages/opentelemetry-plugin-http/src/types.ts +++ b/packages/opentelemetry-plugin-http/src/types.ts @@ -69,6 +69,10 @@ export interface HttpPluginConfig extends PluginConfig { applyCustomAttributesOnSpan?: HttpCustomAttributeFunction; /** The primary server name of the matched virtual host. */ serverName?: string; + /** Require parent to create span for outgoing requests */ + requireParentforOutgoingSpans?: boolean; + /** Require parent to create span for incoming requests */ + requireParentforIncomingSpans?: boolean; } export interface Err extends Error { diff --git a/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts b/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts index 10fa530ba3..078edf32ff 100644 --- a/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts +++ b/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts @@ -696,5 +696,84 @@ describe('HttpPlugin', () => { req.end(); }); }); + + describe('with require parent span', () => { + beforeEach(() => { + memoryExporter.reset(); + plugin.enable(http, provider, provider.logger, {}); + server = http.createServer((request, response) => { + response.end('Test Server Response'); + }); + server.listen(serverPort); + }); + + afterEach(() => { + server.close(); + plugin.disable(); + }); + + it(`should not trace without parent with options enabled (both client & server)`, async () => { + plugin.disable(); + const config: HttpPluginConfig = { + requireParentforIncomingSpans: true, + requireParentforOutgoingSpans: true, + }; + plugin.enable(http, provider, provider.logger, config); + const testPath = `/test/test`; + await httpRequest.get( + `${protocol}://${hostname}:${serverPort}${testPath}` + ); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 0); + }); + + it(`should not trace without parent with options enabled (client only)`, async () => { + plugin.disable(); + const config: HttpPluginConfig = { + requireParentforOutgoingSpans: true, + }; + plugin.enable(http, provider, provider.logger, config); + const testPath = `/test/test`; + const result = await httpRequest.get( + `${protocol}://${hostname}:${serverPort}${testPath}` + ); + assert( + result.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY] !== undefined + ); + assert( + result.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY] !== undefined + ); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1); + assert.strictEqual( + spans.every(span => span.kind === SpanKind.SERVER), + true + ); + }); + + it(`should not trace without parent with options enabled (server only)`, async () => { + plugin.disable(); + const config: HttpPluginConfig = { + requireParentforIncomingSpans: true, + }; + plugin.enable(http, provider, provider.logger, config); + const testPath = `/test/test`; + const result = await httpRequest.get( + `${protocol}://${hostname}:${serverPort}${testPath}` + ); + assert( + result.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY] !== undefined + ); + assert( + result.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY] !== undefined + ); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 1); + assert.strictEqual( + spans.every(span => span.kind === SpanKind.CLIENT), + true + ); + }); + }); }); }); From 830c49d1b6542f89a4aba537dd4f22177cdbcb1a Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Wed, 15 Apr 2020 10:46:02 +0200 Subject: [PATCH 2/6] chore: remove use of NoopSpan --- packages/opentelemetry-plugin-http/src/http.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts index 3db015e8e3..fcc136df96 100644 --- a/packages/opentelemetry-plugin-http/src/http.ts +++ b/packages/opentelemetry-plugin-http/src/http.ts @@ -24,7 +24,6 @@ import { Status, SpanContext, TraceFlags, - NoopSpan, } from '@opentelemetry/api'; import { BasePlugin, @@ -308,7 +307,7 @@ export class HttpPlugin extends BasePlugin { }; return context.with(propagation.extract(headers), () => { - let span: Span = new NoopSpan(); + let span: Span; const hasParent = plugin._tracer.getCurrentSpan() !== undefined; /* * If a parent is required but not present, we use a `NoRecordingSpan` to still @@ -423,7 +422,7 @@ export class HttpPlugin extends BasePlugin { kind: SpanKind.CLIENT, }; const hasParent = plugin._tracer.getCurrentSpan() !== undefined; - let span: Span = new NoopSpan(); + let span: Span; /* * If a parent is required but not present, we use a `NoRecordingSpan` to still * propagate context without recording it. From 3e1292bea323d3e13d2165c6710887c6238875c6 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sat, 18 Apr 2020 14:09:33 +0200 Subject: [PATCH 3/6] test(http): add test case --- .../test/functionals/http-enable.test.ts | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts b/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts index 078edf32ff..7197a7410d 100644 --- a/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts +++ b/packages/opentelemetry-plugin-http/test/functionals/http-enable.test.ts @@ -698,13 +698,13 @@ describe('HttpPlugin', () => { }); describe('with require parent span', () => { - beforeEach(() => { + beforeEach(done => { memoryExporter.reset(); plugin.enable(http, provider, provider.logger, {}); server = http.createServer((request, response) => { response.end('Test Server Response'); }); - server.listen(serverPort); + server.listen(serverPort, done); }); afterEach(() => { @@ -774,6 +774,47 @@ describe('HttpPlugin', () => { true ); }); + + it(`should trace with parent with both requireParent options enabled`, done => { + plugin.disable(); + const config: HttpPluginConfig = { + requireParentforIncomingSpans: true, + requireParentforOutgoingSpans: true, + }; + plugin.enable(http, provider, provider.logger, config); + const testPath = `/test/test`; + const tracer = provider.getTracer('default'); + const span = tracer.startSpan('parentSpan', { + kind: SpanKind.INTERNAL, + }); + tracer.withSpan(span, () => { + httpRequest + .get(`${protocol}://${hostname}:${serverPort}${testPath}`) + .then(result => { + span.end(); + assert( + result.reqHeaders[DummyPropagation.TRACE_CONTEXT_KEY] !== + undefined + ); + assert( + result.reqHeaders[DummyPropagation.SPAN_CONTEXT_KEY] !== + undefined + ); + const spans = memoryExporter.getFinishedSpans(); + assert.strictEqual(spans.length, 2); + assert.strictEqual( + spans.filter(span => span.kind === SpanKind.CLIENT).length, + 1 + ); + assert.strictEqual( + spans.filter(span => span.kind === SpanKind.INTERNAL).length, + 1 + ); + return done(); + }) + .catch(done); + }); + }); }); }); }); From 300d94565bd5fe6436b88e5c190c77f8fcfb5d50 Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sat, 25 Apr 2020 18:00:27 +0200 Subject: [PATCH 4/6] refactor: address obecny suggestion --- .../opentelemetry-plugin-http/src/http.ts | 60 ++++++++----------- 1 file changed, 24 insertions(+), 36 deletions(-) diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts index fcc136df96..89e1d7520f 100644 --- a/packages/opentelemetry-plugin-http/src/http.ts +++ b/packages/opentelemetry-plugin-http/src/http.ts @@ -307,23 +307,10 @@ export class HttpPlugin extends BasePlugin { }; return context.with(propagation.extract(headers), () => { - let span: Span; - const hasParent = plugin._tracer.getCurrentSpan() !== undefined; - /* - * If a parent is required but not present, we use a `NoRecordingSpan` to still - * propagate context without recording it. - */ - if ( - plugin._config.requireParentforIncomingSpans === true && - hasParent === false - ) { - const spanContext = - getExtractedSpanContext(context.active()) ?? - plugin._emptySpanContext; - span = new NoRecordingSpan(spanContext); - } else { - span = plugin._startHttpSpan(`${method} ${pathname}`, spanOptions); - } + const span = plugin._startHttpSpan( + `${method} ${pathname}`, + spanOptions + ); return plugin._tracer.withSpan(span, () => { context.bind(request); @@ -421,22 +408,7 @@ export class HttpPlugin extends BasePlugin { const spanOptions: SpanOptions = { kind: SpanKind.CLIENT, }; - const hasParent = plugin._tracer.getCurrentSpan() !== undefined; - let span: Span; - /* - * If a parent is required but not present, we use a `NoRecordingSpan` to still - * propagate context without recording it. - */ - if ( - plugin._config.requireParentforOutgoingSpans === true && - hasParent === false - ) { - const spanContext = - getExtractedSpanContext(context.active()) ?? plugin._emptySpanContext; - span = new NoRecordingSpan(spanContext); - } else { - span = plugin._startHttpSpan(operationName, spanOptions); - } + const span = plugin._startHttpSpan(operationName, spanOptions); return plugin._tracer.withSpan(span, () => { if (!optionsParsed.headers) optionsParsed.headers = {}; @@ -456,9 +428,25 @@ export class HttpPlugin extends BasePlugin { } private _startHttpSpan(name: string, options: SpanOptions) { - const span = this._tracer - .startSpan(name, options) - .setAttribute(AttributeNames.COMPONENT, this.component); + /* + * If a parent is required but not present, we use a `NoRecordingSpan` to still + * propagate context without recording it. + */ + const hasParent = this._tracer.getCurrentSpan() !== undefined; + const requireParent = + options.kind === SpanKind.CLIENT + ? this._config.requireParentforOutgoingSpans + : this._config.requireParentforIncomingSpans; + let span: Span; + if (hasParent === false && requireParent === true) { + const spanContext = + getExtractedSpanContext(context.active()) ?? plugin._emptySpanContext; + span = new NoRecordingSpan(spanContext); + } else { + span = this._tracer + .startSpan(name, options) + .setAttribute(AttributeNames.COMPONENT, this.component); + } this._spanNotEnded.add(span); return span; } From 88a9788fbaa89bca5d88e44788715c2a16720e3f Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sat, 2 May 2020 15:17:31 +0200 Subject: [PATCH 5/6] chore: add TODO comment --- packages/opentelemetry-plugin-http/src/http.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts index 89e1d7520f..142ac5f689 100644 --- a/packages/opentelemetry-plugin-http/src/http.ts +++ b/packages/opentelemetry-plugin-http/src/http.ts @@ -441,6 +441,8 @@ export class HttpPlugin extends BasePlugin { if (hasParent === false && requireParent === true) { const spanContext = getExtractedSpanContext(context.active()) ?? plugin._emptySpanContext; + // TODO: Refactor this when a solution is found in + // https://github.com/open-telemetry/opentelemetry-specification/issues/530 span = new NoRecordingSpan(spanContext); } else { span = this._tracer From 4cd48d1672b637af717a8134b6e182debeb2c95c Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Wed, 6 May 2020 17:50:40 +0200 Subject: [PATCH 6/6] chore: rewrite requireParent condition to avoid unnecessary work --- packages/opentelemetry-plugin-http/src/http.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts index 142ac5f689..c6c9aaa933 100644 --- a/packages/opentelemetry-plugin-http/src/http.ts +++ b/packages/opentelemetry-plugin-http/src/http.ts @@ -432,13 +432,12 @@ export class HttpPlugin extends BasePlugin { * If a parent is required but not present, we use a `NoRecordingSpan` to still * propagate context without recording it. */ - const hasParent = this._tracer.getCurrentSpan() !== undefined; const requireParent = options.kind === SpanKind.CLIENT ? this._config.requireParentforOutgoingSpans : this._config.requireParentforIncomingSpans; let span: Span; - if (hasParent === false && requireParent === true) { + if (requireParent === true && this._tracer.getCurrentSpan() === undefined) { const spanContext = getExtractedSpanContext(context.active()) ?? plugin._emptySpanContext; // TODO: Refactor this when a solution is found in