Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
May 13, 2024

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented May 10, 2024

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 blumamir requested a review from a team May 10, 2024 05:42
@blumamir
Copy link
Member Author

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

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

blumamir commented May 11, 2024

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.

@pichlermarc pichlermarc merged commit 3cfa783 into open-telemetry:main May 13, 2024
19 checks passed
@Flarna
Copy link
Member

Flarna commented May 13, 2024

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
Copy link
Contributor

trentm commented May 13, 2024

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.

Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…elemetry#4692)

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

* chore: CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants