feat(instrumentation): apply unwrap before wrap in base class #4692
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:to this shorter cleaner version:
Improvement Opportunity
A common implementation of instrumnetation
patch
hook looks like this: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 anddisabled()
d again and again to clear their state and start fresh.Change
The
_wrap
function is implemented in theInstrumentationBase
class for platform node. I moved the common checkisWrapped
and_unwrap
ing 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 thewrap
for nodejs base instrumentation class:patch
implementationsunwrap
in implementations, thus making tests more robust.Change Analysis
unwrap
ed to begin with (end users, and tests where the check is already ), this addition will not do any extra work since theisWrapped
is false.wrap
on an alreadywrap
ed function. I think this case doesn't make sense and it will only happen if the implementation forgot to run theunwraping
. In this case, applying theunwrap
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 toenable
anddisable
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.