Consider auto-enabling or warning if non-enabled context manager is used

[dyladan]

When a user creates an AsyncHooksContextManager, it is easy to forget to call enable, which can cause issues when used with promises. We should consider enabling the context manager automatically, or at least warning if a context manager is used without a call to enable.

See an example of this confusion in the original issue description below

I have a simple use case which loses context when using AsyncHooksContextManager:

const api = require("@opentelemetry/api");
const tracing = require("@opentelemetry/tracing");
const contextAsyncHooks = require("@opentelemetry/context-async-hooks");

api.context.setGlobalContextManager(new contextAsyncHooks.AsyncHooksContextManager());
api.trace.setGlobalTracerProvider(new tracing.BasicTracerProvider());

const tracer = api.trace.getTracer("context-loss-test");
const span = tracer.startSpan("test");
tracer.withSpan(span, () => contextLossFunction())

async function contextLossFunction() {
    let currentSpan = tracer.getCurrentSpan();
    if (currentSpan) {
        console.log("before await", currentSpan.context().traceId);
    } else {
        console.log("context lost")
    }
    await (async () => {})()
    currentSpan = tracer.getCurrentSpan();
    if (currentSpan) {
        console.log("after await", currentSpan.context().traceId);
    } else {
        console.log("context lost")
    }
}

output:

before await 12305e9b40d445d997f5da1e420bcc70
context lost

Seems very similar to #752 and #1101

I thought this case should have been solved by #1099

/cc @vmarchaud

[vmarchaud]

@dyladan With which version you reproduced the issue ? (both node and ot)

[dyladan]

Latest ot (0.10.2)

Multiple node versions including the latest of both 12 and 14.

The AsyncLocalStorageContextManager does not have this bug, but the version of node available in our target environment is running a version of node 12 that is not late enough to be able to use it.

[vmarchaud]

I can't reproduce with the following test (node v14.7.0, master branch):

it('should not loose the context (async/await)', done => {
  const scope1 = '1' as any;

  contextManager.with(scope1, async () => {
    assert.strictEqual(contextManager.active(), scope1);
    await (async () => {})()
    assert.strictEqual(contextManager.active(), scope1);
    return done();
  });
  assert.strictEqual(contextManager.active(), Context.ROOT_CONTEXT);
});

And i believe this is tested with this test

EDIT: I didnt mention it but i reproduce with your exact code though

[dyladan]

Following your example, another simple reproduction:

const { AsyncHooksContextManager } = require("@opentelemetry/context-async-hooks");

const contextManager = new AsyncHooksContextManager();
const scope1 = '1';

contextManager.with(scope1, async () => {
    console.log(contextManager.active()) // 1
    await (async () => { })()
    console.log(contextManager.active()) // Context { _currentContext: Map(0) {} }
});

[dyladan]

I don't know what is different about it in the test environment vs in the reproduction.

[dyladan]

Fixed with one line:

const { AsyncHooksContextManager } = require("@opentelemetry/context-async-hooks");

const contextManager = new AsyncHooksContextManager();
contextManager.enable()
const scope1 = '1';

contextManager.with(scope1, async () => {
    console.log(contextManager.active())
    await (async () => { })()
    console.log(contextManager.active())
});

I feel silly now

[vmarchaud]

Ahah no worries i already spent few hours trying to debug issue when i wasn't enabling the it. Wondering if we could warn at some point that it is used without being enabled

[dyladan]

Is there some reason we don't automatically enable?

[vmarchaud]

@dyladan At the root i believe we wanted to choose whent to enable it (because the performance hit only is there after the enable), but i believe it don't make much sense with our current internal usage. Would be a breaking change though so we might want to take a decision before GA

[dyladan]

I think if the user creates an async hooks context manager, the expectation that async hooks is enabled is a reasonable one.

[Flarna]

Sounds related: #758

FWIW the AsyncLocalStorage in node uses autoenable - even after disable was called.
disable is present mostly to have some emergency tool in case something goes wrong. And for the rare case it's really not needed anymore to allow GC to collect it.
Frequently disable/enable of a context manager will likely cause problems than solving ones.

[dyladan]

Maybe instead we should remove enable/disable from the interface and always auto-enable?

[pichlermarc]

Closing this for the following reasons:

• enable() as it's already part of the stable API and we won't be able to change this for the foreseeable future (that would be a breaking change) - we may revisit the decision if API 2.0 ever happens
• AsyncLocalStorageContextManager does already auto-enable
• AsyncHooksContextManager will be removed in SDK 2.0, so also it's enable() function will be gone 🙂 Remove unused AsyncHooksContextManager #4364