Is a context manager really necessary?

[carlpett]

In the readme, there's a comment in the example claiming that wrap routes requires having a context manager:

fastify-opentelemetry/README.md

Lines 72 to 76 in 15f12eb

	provider.register({
	// A contextManager must be provided when using the wrapRoutes option.
	contextManager: new AsyncHooksContextManager(),
	propagator: new HttpTraceContext()
	})

Now, it seems there's a bug here, you need to call enable() on the context manager. Otherwise the parent span got lost after await:ing in handlers. But after finding that out, I saw references in upstream release notes that another context manager is automatically used if not manually installed. And simply removing AsyncHooksContextManager altogether seems to also get things working...

Is the comment outdated, or is there something I'm not seeing yet?

[HW13]

Hi @carlpett, thanks for opening an issue!

I can confirm that a context manager is still required in order for wrapRoutes to function properly. When running the example app, if you comment out this line and make a request - the 'reply.doingWork' span will have an undefined parentId. With the line not commented out the parentId will be correct.

If using the NodeTracerProvider, it will automatically provide a context manager (either AsyncHooksContextManager or AsyncLocalStorageContextManager depending on your node version - see here). However, the BasicTracerProvider used in the example will not provide a default context manager, and you'll end up with the context API's noop default.

Good catch with the missing enable() in the examples, would you like to create a PR to fix them?

[carlpett]

Ah, I am indeed using the NodeTraceProvider, that explains why it is working :)
I'm out of office for the rest of the week, but can try to have a look at it later.

[HW13]

I've gone ahead and updated our examples, thanks for bringing this to our attention @carlpett! 😎 👍