Migrate from AsyncResource to TracingChannel for Hapi.js plugin

[cristunaranjo]

What does this PR do?

This PR addresses issue #4557, where dd-trace's use of AsyncResource in the Hapi.js handler wrapper was causing conflicts with existing AsyncLocalStorage contexts in applications. By removing the AsyncResource usage, the PR ensures that user-defined async contexts are respected and not overridden.

The original implementation used AsyncResource to ensure that tracing was correctly applied within the Hapi.js handler context. However, this was found to interfere with applications that already define their own async contexts using AsyncLocalStorage. In this PR, I removed the AsyncResource wrapping to prevent conflicts with these user-defined contexts. The handler function is now directly invoked within the existing async scope.

Potential Side Effects

Removing AsyncResource may potentially affect the isolation of the tracing context in environments where AsyncLocalStorage is not in use. There might be cases where this change could lead to the leakage of async contexts if not properly managed by the application itself. Specifically, applications that rely on the isolation provided by AsyncResource might need to take additional steps to ensure context integrity.

Testing

I have tested this change in a Hapi.js application using AsyncLocalStorage and confirmed that the context is preserved across requests without conflicts. Additionally, I ran the existing dd-trace test suite to ensure that no other functionality was inadvertently affected. If necessary, I am happy to assist with further testing across different Node.js versions or specific scenarios.

[uurien]

Hi there,
Thank you so much for your contribution to the repo. We really appreciate the time you've put into this PR. We'll be giving it a detailed review and will get back to you as soon as we can.
Best regards!

[rochdev]

While this addresses the issue of altering the async stack, it also has the effect of breaking tracing. The proper fix here would be to switch from AsyncResource to TracingChannel which is able to track individual stores instead of having to alter the current async context for all code in the process.

[cristunaranjo]

Thanks for the review @rochdev

I'm not too familiar with this library (or tracing libraries in general 😅 ). Where can I find an example of how to use the TracingChannel? Would it be similar to the apollo instrumentation?

I'm happy to give it a shot and try to fix it, but this is out of my expertise.

[rochdev]

Where can I find an example of how to use the TracingChannel? Would it be similar to the apollo instrumentation?

Generally speaking yes, but for Hapi it's a bit different as its events (at least in the current code) don't really map directly to a TracingChannel. However, I don't think a full conversion is needed to address the immediate issue, so only the extension could be done to start with, and it doesn't even need to be complete. In theory, by looking at the code quickly, is seems like the following changes should be made:

• In the instrumentation, replace the asyncResource.runInAsyncScope() with a tracingChannel.traceSync(). The channel could be called apm:hapi:extension which will automatically publish events, so publishing apm:hapi:extension:enter manually will no longer be needed. By convention, the event will now be apm:hapi:extension:start instead. (note: this approach is not entirely correct as extensions are asynchronous, but since we don't care about when it returns for our purpose, it's fine to use traceSync for now).
• In the plugin, replace the call to this.addSub('apm:hapi:extension:enter' with instead this.addBind(apm:hapi:extension:start', and instead of calling this.enterin the callback, returnthis._requestSpans.get(req)`.

I may be forgetting something, but generally speaking those are the minimum steps to migrate from AsyncResource to TracingChannel. More work would be needed for a complete transition, but the above should be enough to at least fix the issue of altering the async context of the entire process without impacting tracing.

[cristunaranjo]

I've made the requested changes and confirmed that the storage is functioning correctly. However, I’m not entirely sure how to validate that the traces are working as expected. I'm not familiar with what to look for, and with my previous change the traces seemed fine to me 😅 .
Could you guide me on how to test the tracing @rochdev, or if there's a specific way to verify that everything is working correctly?

[rochdev]

Could you guide me on how to test the tracing @rochdev, or if there's a specific way to verify that everything is working correctly?

Sorry for the delay, I just saw this. You can validate that this works by running the tests for Hapi locally with PLUGINS=hapi yarn test:plugins:ci which will install supported versions of Hapi and run our tests for all of them.

As for the change, it looks good to me for the most part, and if anything is missing the tests will catch it. Of course it might be worth it to also add a test for the incorrect behaviour you were seeing to avoid a regression in the future.

The only thing I'm not sure I understand in the PR is the tracing channel map. Why is that needed?

[cristunaranjo]

I have added a test to the datadog-plugin-hapi package. The tests look good on my end.

Regarding the tracing map, I replicated the approach used with the channels map so that it can be reused in other plugins. However, I am happy to remove it if you feel it is unnecessary @rochdev

[rochdev]

I have added a test to the datadog-plugin-hapi package. The tests look good on my end.

Perfect, thanks!

Regarding the tracing map, I replicated the approach used with the channels map so that it can be reused in other plugins. However, I am happy to remove it if you feel it is unnecessary

I guess I don't understand what is the purpose of the map in the first place 😅 If it can be removed, then let's definitely remove it. It should be possible to use tracingChannel on its own as it already keeps its own internal map.

[cristunaranjo]

Thanks for the feedback again @rochdev. Really appreciate your help with this.
I have removed the tracingChannel map, wasn't aware you already track this internally. Please, let me know if there's something else missing.

[tlhunter]

Thanks for this work, @cristunaranjo! I'm going to make a copy of your PR so that all of our CI tasks can properly run.

[tlhunter]

These changes have merged and should be available in the next release. Thanks for the contribution!