feat(instrumentation): apply unwrap before wrap in base class

[blumamir]

This PR aims to cleanup code from instrumentations, that unwraps a patched function before attempting to patch it again.

For instrumentation implementations, there will be no need to _unwrap before calling _wrap, refactoring this:

if (isWrapped(moduleExports.myFunc)) {
    this._unwrap(moduleExports, 'myFunc');
}
this._wrap(moduleExports, 'myFunc', this._getMyFuncFunctionPatch());

to this shorter cleaner version:

this._wrap(moduleExports, 'myFunc', this._getMyFuncFunctionPatch());

Improvement Opportunity

A common implementation of instrumnetation patch hook looks like this:

        if (isWrapped(moduleExports.request)) {
          this._unwrap(moduleExports, 'request');
        }
        this._wrap(
          moduleExports,
          'request',
          this._getPatchOutgoingRequestFunction('http')
        );
        if (isWrapped(moduleExports.get)) {
          this._unwrap(moduleExports, 'get');
        }
        this._wrap(
          moduleExports,
          'get',
          this._getPatchOutgoingGetFunction(moduleExports.request)
        );
        if (isWrapped(moduleExports.Server.prototype.emit)) {
          this._unwrap(moduleExports.Server.prototype, 'emit');
        }
        this._wrap(
          moduleExports.Server.prototype,
          'emit',
          this._getPatchIncomingRequestFunction('http')
        );
        return moduleExports;

Instrumentation first checks if the function it attempts to patch is already wrapped, and if so, unwrap it before it is wrapped again. This is needed, I believe, for test to function correctly, where the tests are enable()d and disabled()d again and again to clear their state and start fresh.

Change

The _wrap function is implemented in the InstrumentationBase class for platform node. I moved the common check isWrapped and _unwraping it if needed, into the _wrap implementation, and removed it from the 2 core instrumentation. you can already see how many lines of code are cleared from existing instrumentations, making the codebase a bit shorter and removing lines that add no real value and decrease readability in instrumentation implementation.

Benefits

This PR moves the unwrap call to happen as part of the wrap for nodejs base instrumentation class:

• hoist common code so instrumentations are a bit lighter with shorter patch implementations
• makes sure instrumentation authors will not forget to call unwrap in implementations, thus making tests more robust.
• guarantee consistency as the usage pattern we settled upon is now done the same everywhere regardless of the caller implementation.

Change Analysis

• For calls that are unwraped to begin with (end users, and tests where the check is already ), this addition will not do any extra work since the isWrapped is false.
• The other case is when we call wrap on an already wraped function. I think this case doesn't make sense and it will only happen if the implementation forgot to run the unwraping. In this case, applying the unwrap in base class would guarantee that we only have one active patch per function. This should never happen in end users' code, only in tests, as end users are not expected to enable and disable instrumentations back and forte in their apps.

We can release this feature without breaking any existing instrumentation, and then create followup PRs to cleanup contrib instrumentations and take advantage of this service.

[blumamir]

I didn't add tests, as this functionality exists to support instrumentation tests like http and grpc, and those tests pass after the refactor.

[dyladan]

i wonder if we should make unwrap private. Private methods can still be called from tests using obj["_somePrivateProperty"] and it really shouldn't be used by users.

[blumamir]

i wonder if we should make unwrap private. Private methods can still be called from tests using obj["_somePrivateProperty"] and it really shouldn't be used by users.

That's a good point. Adding @Flarna comment from here:

By the way, the handling of enabled flag is not done consistent as of now, see #2257

I think a single enable / disable flag is not enough in general. I would expect 2 flags:

enable (or disable) - only evaluated at construction time to decide patch yes/no
capture - runtime changeable, evaluated whenever some monitored/pached API is called

I think there are still a lot of opportunities for improvements which we can address.

This PR will allow us to clean dozens of lines of codes from contrib instrumentations.

BTW - I wonder if we can introduce something to record which patches where made, and then automate the unpatch which is trivial in most cases. Perhaps I will research it as followup PR.

[Flarna]

Sorry, I'm a bit late in this because of vacation.

Do we know why unwrap() before wrap() is done/needed?

We rely on simmer and others might also use shimmer - so OTel might undo wrappings done by others. Fine in tests but in real world?

[trentm]

We rely on simmer and others might also use shimmer - so OTel might undo wrappings done by others. Fine in tests but in real world?

FWIW, the Elastic APM agent has a light fork of shimmer.js to have it use a private symbol for the "is wrapped" and "unwrap" properties, so that it won't conflict with other uses of shimmer.