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

chore: refactor module patch handling in Instrumentation class #3658

Merged
merged 6 commits into from
Oct 11, 2023

Conversation

trentm
Copy link
Member

@trentm trentm commented Oct 6, 2023

This refactors how module patch handlers are loaded and the Instrumentation class file. There is now a PatcherRegistry that is a little smarter than the _patches object before it.

The main change is that we now use RITM's internals:true option so that our onRequire hook is called with module sub-paths. This will allow #3657 to hook both "mongodb" and "mongodb/lib/cmap/connection_pool.js", and avoid a RITM bug where "mongodb" &
"mongodb/lib/cmap/connection_pool" (no extension, indicating a "sub-module") don't work together.

Benefits and other changes:

  • With the smarter "PatcherRegistry" the module name can be used in disableInstrumentations to disable any patchers for that module. E.g. disableInstrumentations=next will disable all four next-related patchers.
  • A custom "lambda" name is now usable to disable the Lambda instr, FWIW.
  • (Note: we could also add custom disableInstrumentations keys for other modules, e.g. perhaps "aws-sdk" for all those.)
  • Some edge cases with the (rare, and undocumented) config.addPatch config var have been cleaned up.
  • Loading of the lambda-handler has been refactored a bit to no longer need to know internal details of the instrumentation module.

Refs: #3657
Closes: #2992

This refactors how module patch handlers are loaded and the
Instrumentation class file. There is now a PatcherRegistry that is
a little smarter than the `_patches` object before it.

The main change is that we now use RITM's internals:true option
so that our `onRequire` hook is called with module sub-paths. This will
allow #3657 to hook both "mongodb" and "mongodb/lib/cmap/connection_pool.js",
and avoid a RITM bug where "mongodb" &
"mongodb/lib/cmap/connection_pool" (no extension, indicating a
"sub-module") don't work together.

Benefits and other changes:
- With the smarter "PatcherRegistry" the module name can be used in
  `disableInstrumentations` to disable any patchers for that module.
  E.g. `disableInstrumentations=next` will disable all four
  `next`-related patchers.
- A custom "lambda" name is now usable to disable the Lambda instr,
  FWIW.
- (Note: we *could* also add custom disableInstrumentations keys for
  other modules, e.g. perhaps "aws-sdk" for all those.)
- Some edge cases with the (rare, and undocumented) `config.addPatch`
  config var have been cleaned up.
- Loading of the lambda-handler has been refactored a bit to no longer
  need to know internal details of the instrumentation module.

Refs: #3657
Closes: #2992
@trentm trentm requested a review from david-luna October 6, 2023 23:24
@trentm trentm self-assigned this Oct 6, 2023
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Oct 6, 2023
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Oct 6, 2023
david-luna
david-luna previously approved these changes Oct 9, 2023
test/config.test.js Show resolved Hide resolved
@david-luna
Copy link
Member

Looks good. I'm leaving already the approval so you can merge when you finish the the caching of module versions

@trentm trentm merged commit 1eae1b5 into main Oct 11, 2023
18 checks passed
APM-Agents (OLD) automation moved this from In Progress to Done Oct 11, 2023
@trentm trentm deleted the trentm/instr-refactor branch October 11, 2023 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
Development

Successfully merging this pull request may close these issues.

Allow Logical Grouping of Instrumentation by Name
2 participants