From b794a08ff56befbaeb9138f06823df4fb144b89c Mon Sep 17 00:00:00 2001 From: vmarchaud Date: Sun, 10 May 2020 11:31:35 +0200 Subject: [PATCH] chore: address PR comments --- packages/opentelemetry-api/src/api/context.ts | 12 +++++----- .../opentelemetry-api/test/api/global.test.ts | 22 +++++++++--------- packages/opentelemetry-node/README.md | 8 +++++++ packages/opentelemetry-node/src/NodeTracer.ts | 23 +++---------------- 4 files changed, 28 insertions(+), 37 deletions(-) diff --git a/packages/opentelemetry-api/src/api/context.ts b/packages/opentelemetry-api/src/api/context.ts index b127917990b..d799cfe077b 100644 --- a/packages/opentelemetry-api/src/api/context.ts +++ b/packages/opentelemetry-api/src/api/context.ts @@ -54,7 +54,7 @@ export class ContextAPI { ): ContextManager { if (_global[GLOBAL_CONTEXT_MANAGER_API_KEY]) { // global context manager has already been set - return this._getContextManager(); + return this.getContextManager(); } _global[GLOBAL_CONTEXT_MANAGER_API_KEY] = makeGetter( @@ -70,7 +70,7 @@ export class ContextAPI { * Get the currently active context */ public active(): Context { - return this._getContextManager().active(); + return this.getContextManager().active(); } /** @@ -83,7 +83,7 @@ export class ContextAPI { context: Context, fn: T ): ReturnType { - return this._getContextManager().with(context, fn); + return this.getContextManager().with(context, fn); } /** @@ -93,10 +93,10 @@ export class ContextAPI { * @param context context to bind to the event emitter or function. Defaults to the currently active context */ public bind(target: T, context: Context = this.active()): T { - return this._getContextManager().bind(target, context); + return this.getContextManager().bind(target, context); } - private _getContextManager(): ContextManager { + public getContextManager(): ContextManager { return ( _global[GLOBAL_CONTEXT_MANAGER_API_KEY]?.( API_BACKWARDS_COMPATIBILITY_VERSION @@ -106,7 +106,7 @@ export class ContextAPI { /** Disable and remove the global context manager */ public disable() { - this._getContextManager().disable(); + this.getContextManager().disable(); delete _global[GLOBAL_CONTEXT_MANAGER_API_KEY]; } } diff --git a/packages/opentelemetry-api/test/api/global.test.ts b/packages/opentelemetry-api/test/api/global.test.ts index 5ff4e25a2bf..8a9a67f752d 100644 --- a/packages/opentelemetry-api/test/api/global.test.ts +++ b/packages/opentelemetry-api/test/api/global.test.ts @@ -34,8 +34,8 @@ describe('Global Utils', () => { assert.notEqual(api1, api2); // that return separate noop instances to start assert.notStrictEqual( - api1.context['_getContextManager'](), - api2.context['_getContextManager']() + api1.context['getContextManager'](), + api2.context['getContextManager']() ); beforeEach(() => { @@ -46,33 +46,33 @@ describe('Global Utils', () => { }); it('should change the global context manager', () => { - const original = api1.context['_getContextManager'](); + const original = api1.context['getContextManager'](); const newContextManager = new NoopContextManager(); api1.context.setGlobalContextManager(newContextManager); - assert.notStrictEqual(api1.context['_getContextManager'](), original); - assert.strictEqual(api1.context['_getContextManager'](), newContextManager); + assert.notStrictEqual(api1.context['getContextManager'](), original); + assert.strictEqual(api1.context['getContextManager'](), newContextManager); }); it('should load an instance from one which was set in the other', () => { api1.context.setGlobalContextManager(new NoopContextManager()); assert.strictEqual( - api1.context['_getContextManager'](), - api2.context['_getContextManager']() + api1.context['getContextManager'](), + api2.context['getContextManager']() ); }); it('should disable both if one is disabled', () => { - const original = api1.context['_getContextManager'](); + const original = api1.context['getContextManager'](); api1.context.setGlobalContextManager(new NoopContextManager()); - assert.notStrictEqual(original, api1.context['_getContextManager']()); + assert.notStrictEqual(original, api1.context['getContextManager']()); api2.context.disable(); - assert.strictEqual(original, api1.context['_getContextManager']()); + assert.strictEqual(original, api1.context['getContextManager']()); }); it('should return the module NoOp implementation if the version is a mismatch', () => { - const original = api1.context['_getContextManager'](); + const original = api1.context['getContextManager'](); api1.context.setGlobalContextManager(new NoopContextManager()); const afterSet = _global[GLOBAL_CONTEXT_MANAGER_API_KEY]!(-1); diff --git a/packages/opentelemetry-node/README.md b/packages/opentelemetry-node/README.md index 4dd8d84a741..3acbef8ada8 100644 --- a/packages/opentelemetry-node/README.md +++ b/packages/opentelemetry-node/README.md @@ -92,6 +92,14 @@ provider.register() ## Examples See how to automatically instrument [http](https://github.com/open-telemetry/opentelemetry-js/tree/master/examples/http) and [gRPC](https://github.com/open-telemetry/opentelemetry-js/tree/master/examples/grpc) using node-sdk. +## Experimental API methods + +#### withSpanAsync + +The `NodeTracer` expose a method called `withSpanAsync` that allows you to set a current span in the context of a `async` function. This previously was a problem with `withSpan` because its synchronous behavior, read about that in [this issue](https://github.com/open-telemetry/opentelemetry-js/issues/752). +This method have two drawback: + - strict requirement of the `AsyncHooksScopeManager`, you **cannot** use it without. + - the underlying implementation inside `AsyncHooksScopeManager` is experimental, so is this method, please open an issue if you have a problem with it. ## Useful links - For more information on OpenTelemetry, visit: diff --git a/packages/opentelemetry-node/src/NodeTracer.ts b/packages/opentelemetry-node/src/NodeTracer.ts index 96d48bfb09b..c838e306860 100644 --- a/packages/opentelemetry-node/src/NodeTracer.ts +++ b/packages/opentelemetry-node/src/NodeTracer.ts @@ -14,22 +14,6 @@ * limitations under the License. */ -/*! - * Copyright 2019, 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 api from '@opentelemetry/api'; import { setActiveSpan } from '@opentelemetry/core'; import { @@ -54,8 +38,7 @@ export class NodeTracer extends Tracer { T extends Promise, U extends (...args: unknown[]) => T >(span: api.Span, fn: U): Promise { - // @ts-ignore - const contextManager = api.context._getContextManager(); + const contextManager = api.context.getContextManager(); if (contextManager instanceof AsyncHooksContextManager) { return contextManager.withAsync( setActiveSpan(api.context.active(), span), @@ -63,9 +46,9 @@ export class NodeTracer extends Tracer { ); } else { this.logger.warn( - `Using withAsync without AsyncHookContextManager doesn't work, please refer to` + `Using withAsync without AsyncHookContextManager doesn't work, please refer to https://github.com/open-telemetry/opentelemetry-js/tree/master/packages/opentelemetry-node#withspanasync` ); - return fn(); + return await fn(); } } }