Skip to content

Commit

Permalink
chore: address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
vmarchaud committed May 10, 2020
1 parent 3ce40ab commit b794a08
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 37 deletions.
12 changes: 6 additions & 6 deletions packages/opentelemetry-api/src/api/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -70,7 +70,7 @@ export class ContextAPI {
* Get the currently active context
*/
public active(): Context {
return this._getContextManager().active();
return this.getContextManager().active();
}

/**
Expand All @@ -83,7 +83,7 @@ export class ContextAPI {
context: Context,
fn: T
): ReturnType<T> {
return this._getContextManager().with(context, fn);
return this.getContextManager().with(context, fn);
}

/**
Expand All @@ -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<T>(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
Expand All @@ -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];
}
}
22 changes: 11 additions & 11 deletions packages/opentelemetry-api/test/api/global.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand All @@ -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);

Expand Down
8 changes: 8 additions & 0 deletions packages/opentelemetry-node/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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: <https://opentelemetry.io/>
Expand Down
23 changes: 3 additions & 20 deletions packages/opentelemetry-node/src/NodeTracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -54,18 +38,17 @@ export class NodeTracer extends Tracer {
T extends Promise<any>,
U extends (...args: unknown[]) => T
>(span: api.Span, fn: U): Promise<T> {
// @ts-ignore
const contextManager = api.context._getContextManager();
const contextManager = api.context.getContextManager();
if (contextManager instanceof AsyncHooksContextManager) {
return contextManager.withAsync(
setActiveSpan(api.context.active(), span),
fn
);
} 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();
}
}
}

0 comments on commit b794a08

Please sign in to comment.