From 0b98274ae7559e7c6189cfc4047c2f433ee3f0a9 Mon Sep 17 00:00:00 2001 From: Olivier Albertini Date: Mon, 5 Aug 2019 18:32:06 -0400 Subject: [PATCH] fix: wip revert - gts fix issue on all packages Signed-off-by: Olivier Albertini --- .../src/BasicTracer.ts | 27 +++---- .../src/NodeTracer.ts | 17 ++++- .../test/NodeTracer.test.ts | 74 ++++++++++++++----- .../opentelemetry-plugin-http/src/http.ts | 45 +++++------ 4 files changed, 101 insertions(+), 62 deletions(-) diff --git a/packages/opentelemetry-basic-tracer/src/BasicTracer.ts b/packages/opentelemetry-basic-tracer/src/BasicTracer.ts index 783779b0f86..821c6d08939 100644 --- a/packages/opentelemetry-basic-tracer/src/BasicTracer.ts +++ b/packages/opentelemetry-basic-tracer/src/BasicTracer.ts @@ -15,12 +15,7 @@ */ import * as types from '@opentelemetry/types'; -import { - ALWAYS_SAMPLER, - BinaryTraceContext, - HttpTraceContext, - NOOP_SPAN, -} from '@opentelemetry/core'; +import { ALWAYS_SAMPLER, BinaryTraceContext, HttpTraceContext, NOOP_SPAN } from '@opentelemetry/core'; import { BinaryFormat, HttpTextFormat } from '@opentelemetry/types'; import { BasicTracerConfig } from '../src/types'; import { ScopeManager } from '@opentelemetry/scope-base'; @@ -34,7 +29,8 @@ export class BasicTracer implements types.Tracer { private readonly _binaryFormat: types.BinaryFormat; private readonly _httpTextFormat: types.HttpTextFormat; private readonly _sampler: types.Sampler; - readonly scopeManager: ScopeManager; + + protected readonly _scopeManager: ScopeManager; /** * Constructs a new Tracer instance. @@ -44,7 +40,7 @@ export class BasicTracer implements types.Tracer { this._defaultAttributes = config.defaultAttributes || {}; this._httpTextFormat = config.httpTextFormat || new HttpTraceContext(); this._sampler = config.sampler || ALWAYS_SAMPLER; - this.scopeManager = config.scopeManager; + this._scopeManager = config.scopeManager; } /** @@ -61,7 +57,7 @@ export class BasicTracer implements types.Tracer { } const spanOptions = Object.assign({}, options, { - parent: parentSpanContext, + parent: parentSpanContext }); const span = new Span(this, name, spanOptions); @@ -75,18 +71,15 @@ export class BasicTracer implements types.Tracer { */ getCurrentSpan(): types.Span { // Get the current Span from the context. - return this.scopeManager.active() as types.Span; + return this._scopeManager.active() as types.Span; } /** * Enters the scope of code where the given Span is in the current context. */ - withSpan ReturnType>( - span: types.Span, - fn: T - ): ReturnType { + withSpan ReturnType>(span: types.Span, fn: T): ReturnType { // Set given span to context. - return this.scopeManager.with(span, fn); + return this._scopeManager.with(span, fn); } /** @@ -110,9 +103,7 @@ export class BasicTracer implements types.Tracer { return this._httpTextFormat; } - private _getParentSpanContext( - parent: types.Span | types.SpanContext | undefined - ): types.SpanContext | undefined { + private _getParentSpanContext(parent: types.Span | types.SpanContext | undefined): types.SpanContext | undefined { if (!parent) return undefined; // parent is a SpanContext diff --git a/packages/opentelemetry-node-tracer/src/NodeTracer.ts b/packages/opentelemetry-node-tracer/src/NodeTracer.ts index ea138470e5c..4b4f504f929 100644 --- a/packages/opentelemetry-node-tracer/src/NodeTracer.ts +++ b/packages/opentelemetry-node-tracer/src/NodeTracer.ts @@ -25,10 +25,21 @@ export class NodeTracer extends BasicTracer { * Constructs a new Tracer instance. */ constructor(config: BasicTracerConfig) { - super( - Object.assign({}, { scopeManager: new AsyncHooksScopeManager() }, config) - ); + super(Object.assign({}, { scopeManager: new AsyncHooksScopeManager() }, config)); // @todo: Integrate Plugin Loader (pull/126). } + /** + * Binds the trace context to the given event emitter. + * This is necessary in order to create child spans correctly in event + * handlers. + * @param emitter An event emitter whose handlers should have + * the trace context binded to them. + */ + wrapEmitter(emitter: NodeJS.EventEmitter): void { + if (!this._scopeManager.active()) { + return; + } + this._scopeManager.bind(emitter); + } } diff --git a/packages/opentelemetry-node-tracer/test/NodeTracer.test.ts b/packages/opentelemetry-node-tracer/test/NodeTracer.test.ts index d27b112708d..3a516d39455 100644 --- a/packages/opentelemetry-node-tracer/test/NodeTracer.test.ts +++ b/packages/opentelemetry-node-tracer/test/NodeTracer.test.ts @@ -21,16 +21,17 @@ import { HttpTraceContext, NEVER_SAMPLER, NoopLogger, - NOOP_SPAN, + NOOP_SPAN } from '@opentelemetry/core'; import { AsyncHooksScopeManager } from '@opentelemetry/scope-async-hooks'; import { NodeTracer } from '../src/NodeTracer'; +import { EventEmitter } from 'events'; describe('NodeTracer', () => { describe('constructor', () => { it('should construct an instance with required only options', () => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.ok(tracer instanceof NodeTracer); }); @@ -38,7 +39,7 @@ describe('NodeTracer', () => { it('should construct an instance with binary format', () => { const tracer = new NodeTracer({ binaryFormat: new BinaryTraceContext(), - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.ok(tracer instanceof NodeTracer); }); @@ -46,7 +47,7 @@ describe('NodeTracer', () => { it('should construct an instance with http text format', () => { const tracer = new NodeTracer({ httpTextFormat: new HttpTraceContext(), - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.ok(tracer instanceof NodeTracer); }); @@ -54,7 +55,7 @@ describe('NodeTracer', () => { it('should construct an instance with logger', () => { const tracer = new NodeTracer({ logger: new NoopLogger(), - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.ok(tracer instanceof NodeTracer); }); @@ -62,7 +63,7 @@ describe('NodeTracer', () => { it('should construct an instance with sampler', () => { const tracer = new NodeTracer({ scopeManager: new AsyncHooksScopeManager(), - sampler: ALWAYS_SAMPLER, + sampler: ALWAYS_SAMPLER }); assert.ok(tracer instanceof NodeTracer); }); @@ -71,9 +72,9 @@ describe('NodeTracer', () => { const tracer = new NodeTracer({ defaultAttributes: { region: 'eu-west', - asg: 'my-asg', + asg: 'my-asg' }, - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.ok(tracer instanceof NodeTracer); }); @@ -82,7 +83,7 @@ describe('NodeTracer', () => { describe('.startSpan()', () => { it('should start a span with name only', () => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); const span = tracer.startSpan('my-span'); assert.ok(span); @@ -90,7 +91,7 @@ describe('NodeTracer', () => { it('should start a span with name and options', () => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); const span = tracer.startSpan('my-span', {}); assert.ok(span); @@ -99,7 +100,7 @@ describe('NodeTracer', () => { it('should return a default span with no sampling', () => { const tracer = new NodeTracer({ sampler: NEVER_SAMPLER, - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); const span = tracer.startSpan('my-span'); assert.deepStrictEqual(span, NOOP_SPAN); @@ -115,7 +116,7 @@ describe('NodeTracer', () => { describe('.getCurrentSpan()', () => { it('should return null with AsyncHooksScopeManager when no span started', () => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.deepStrictEqual(tracer.getCurrentSpan(), null); }); @@ -124,7 +125,7 @@ describe('NodeTracer', () => { describe('.withSpan()', () => { it('should run scope with AsyncHooksScopeManager scope manager', done => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); const span = tracer.startSpan('my-span'); tracer.withSpan(span, () => { @@ -136,7 +137,7 @@ describe('NodeTracer', () => { it('should run scope with AsyncHooksScopeManager scope manager with multiple spans', done => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); const span = tracer.startSpan('my-span'); tracer.withSpan(span, () => { @@ -145,10 +146,7 @@ describe('NodeTracer', () => { const span1 = tracer.startSpan('my-span1', { parent: span }); tracer.withSpan(span1, () => { assert.deepStrictEqual(tracer.getCurrentSpan(), span1); - assert.deepStrictEqual( - span1.context().traceId, - span.context().traceId - ); + assert.deepStrictEqual(span1.context().traceId, span.context().traceId); return done(); }); }); @@ -166,7 +164,7 @@ describe('NodeTracer', () => { describe('getBinaryFormat', () => { it('should get default binary formatter', () => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.ok(tracer.getBinaryFormat() instanceof BinaryTraceContext); }); @@ -175,9 +173,45 @@ describe('NodeTracer', () => { describe('.getHttpTextFormat()', () => { it('should get default HTTP text formatter', () => { const tracer = new NodeTracer({ - scopeManager: new AsyncHooksScopeManager(), + scopeManager: new AsyncHooksScopeManager() }); assert.ok(tracer.getHttpTextFormat() instanceof HttpTraceContext); }); }); + describe('.wrapEmitter()', () => { + it('should not throw', () => { + const tracer = new NodeTracer({ + scopeManager: new AsyncHooksScopeManager() + }); + tracer.wrapEmitter({} as EventEmitter); + }); + // TODO: uncomment once https://github.com/open-telemetry/opentelemetry-js/pull/146 is merged + // it('should get current Span', (done) => { + // const tracer = new NodeTracer({ + // scopeManager: new AsyncHooksScopeManager(), + // }); + + // const span = tracer.startSpan('my-span'); + // class FakeEventEmitter extends EventEmitter { + // constructor() { + // super() + // } + // test() { + // this.emit('event'); + // } + // } + + // tracer.withSpan(span, () => { + // const fake = new FakeEventEmitter(); + // tracer.wrapEmitter(fake); + // fake.on('event', () => { + // setTimeout(() => { + // assert.deepStrictEqual(tracer.getCurrentSpan(), span); + // done(); + // }, 100); + // }); + // fake.test(); + // }); + // }); + }); }); diff --git a/packages/opentelemetry-plugin-http/src/http.ts b/packages/opentelemetry-plugin-http/src/http.ts index 02c49f8ee0b..1d2773d8103 100644 --- a/packages/opentelemetry-plugin-http/src/http.ts +++ b/packages/opentelemetry-plugin-http/src/http.ts @@ -151,16 +151,16 @@ export class HttpPlugin extends BasePlugin { protected patch() { this._logger.debug('applying patch to %s@%s', this.moduleName, this.version); - shimmer.wrap(this._moduleExports, 'request', this.getPatchOutgoingRequestFunction()); + shimmer.wrap(this._moduleExports, 'request', this._getPatchOutgoingRequestFunction()); // In Node 8-10, http.get calls a private request method, therefore we patch it // here too. if (semver.satisfies(this.version, '>=8.0.0')) { - shimmer.wrap(this._moduleExports, 'get', this.getPatchOutgoingGetFunction()); + shimmer.wrap(this._moduleExports, 'get', this._getPatchOutgoingGetFunction()); } if (this._moduleExports && this._moduleExports.Server && this._moduleExports.Server.prototype) { - shimmer.wrap(this._moduleExports.Server.prototype, 'emit', this.getPatchIncomingRequestFunction()); + shimmer.wrap(this._moduleExports.Server.prototype, 'emit', this._getPatchIncomingRequestFunction()); } else { this._logger.error('Could not apply patch to %s.emit. Interface is not as expected.', this.moduleName); } @@ -182,7 +182,7 @@ export class HttpPlugin extends BasePlugin { /** * Creates spans for incoming requests, restoring spans' context if applied. */ - protected getPatchIncomingRequestFunction() { + protected _getPatchIncomingRequestFunction() { return (original: (event: string) => boolean) => { return this.incomingRequestFunction(original, this); }; @@ -192,13 +192,13 @@ export class HttpPlugin extends BasePlugin { * Creates spans for outgoing requests, sending spans' context for distributed * tracing. */ - protected getPatchOutgoingRequestFunction() { + protected _getPatchOutgoingRequestFunction() { return (original: Func): Func => { return this.outgoingRequestFunction(original, this); }; } - protected getPatchOutgoingGetFunction() { + protected _getPatchOutgoingGetFunction() { return (original: Func): Func => { // Re-implement http.get. This needs to be done (instead of using // getPatchOutgoingRequestFunction to patch it) because we need to @@ -249,7 +249,7 @@ export class HttpPlugin extends BasePlugin { propagation.inject(span.context(), HttpPlugin.PROPAGATION_FORMAT, options.headers); request.on('response', (response: IncomingMessage) => { - plugin._tracer.scopeManager.bind(response); + plugin._tracer.wrapEmitter(response); plugin._logger.debug('outgoingRequest on response()'); response.on('end', () => { plugin._logger.debug('outgoingRequest on end()'); @@ -271,8 +271,9 @@ export class HttpPlugin extends BasePlugin { span.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_USER_AGENT, userAgent.toString()); } if (response.statusCode) { - span.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_STATUS_CODE, response.statusCode.toString()); - span.setStatus(HttpPlugin.parseResponseStatus(response.statusCode)); + span + .setAttribute(HttpPlugin.ATTRIBUTE_HTTP_STATUS_CODE, response.statusCode.toString()) + .setStatus(HttpPlugin.parseResponseStatus(response.statusCode)); } if (plugin.options.applyCustomAttributesOnSpan) { @@ -322,34 +323,36 @@ export class HttpPlugin extends BasePlugin { const rootSpan = plugin._tracer.startSpan(path, spanOptions); return plugin._tracer.withSpan(rootSpan, () => { - plugin._tracer.scopeManager.bind(request); - plugin._tracer.scopeManager.bind(response); + plugin._tracer.wrapEmitter(request); + plugin._tracer.wrapEmitter(response); // Wraps end (inspired by: // https://github.com/GoogleCloudPlatform/cloud-trace-nodejs/blob/master/src/plugins/plugin-connect.ts#L75) const originalEnd = response.end; response.end = function(this: ServerResponse, ...args: ResponseEndArgs) { response.end = originalEnd; - // Cannot pass args of type ResponseEndArgs, Expected 1-2 arguments, but got 1 or more. + // Cannot pass args of type ResponseEndArgs, + // tslint complains "Expected 1-2 arguments, but got 1 or more.", it does not make sense to me // tslint:disable-next-line:no-any const returned = response.end.apply(this, arguments as any); const requestUrl = request.url ? url.parse(request.url) : null; const host = headers.host || 'localhost'; const userAgent = (headers['user-agent'] || headers['User-Agent']) as string; - rootSpan.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_HOST, host.replace(/^(.*)(\:[0-9]{1,5})/, '$1')); - - rootSpan.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_METHOD, method); + rootSpan + .setAttribute(HttpPlugin.ATTRIBUTE_HTTP_HOST, host.replace(/^(.*)(\:[0-9]{1,5})/, '$1')) + .setAttribute(HttpPlugin.ATTRIBUTE_HTTP_METHOD, method); if (requestUrl) { - rootSpan.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_PATH, requestUrl.pathname || ''); - rootSpan.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_ROUTE, requestUrl.path || ''); + rootSpan + .setAttribute(HttpPlugin.ATTRIBUTE_HTTP_PATH, requestUrl.pathname || '') + .setAttribute(HttpPlugin.ATTRIBUTE_HTTP_ROUTE, requestUrl.path || ''); } if (userAgent) { rootSpan.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_USER_AGENT, userAgent); } - rootSpan.setAttribute(HttpPlugin.ATTRIBUTE_HTTP_STATUS_CODE, response.statusCode.toString()); - - rootSpan.setStatus(HttpPlugin.parseResponseStatus(response.statusCode)); + rootSpan + .setAttribute(HttpPlugin.ATTRIBUTE_HTTP_STATUS_CODE, response.statusCode.toString()) + .setStatus(HttpPlugin.parseResponseStatus(response.statusCode)); if (plugin.options.applyCustomAttributesOnSpan) { plugin.options.applyCustomAttributesOnSpan(rootSpan, request, response); @@ -398,7 +401,7 @@ export class HttpPlugin extends BasePlugin { return request; } - plugin._tracer.scopeManager.bind(request); + plugin._tracer.wrapEmitter(request); if (!method) { method = 'GET';