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

Allow Logical Grouping of Instrumentation by Name #2992

Closed
astorm opened this issue Oct 18, 2022 · 0 comments · Fixed by #3658
Closed

Allow Logical Grouping of Instrumentation by Name #2992

astorm opened this issue Oct 18, 2022 · 0 comments · Fixed by #3658
Labels
8.7-candidate agent-nodejs Make available for APM Agents project planning.

Comments

@astorm
Copy link
Contributor

astorm commented Oct 18, 2022

Is your feature request related to a problem? Please describe.

Instrumentation increasingly requires us to hook multiple modules, which means it's no longer easy/intuitive to enable/disable a single module's instrumentation.

Describe the solution you'd like

Modify module wrapping and disabling to allow for logic grouping of instrumentations.

Describe alternatives you've considered

Passing complexity on to users and being sad :(

Additional context

Traditionally, there's been a 1:1 relationship between the module being instrumented and the individual instrumentation-module we write. Put another way, for each entry in the MODULES array, there's been a single instrumentation file in the instrumentation/modules folder. Traditionally, access to the object retured by requiring a module has been enough for us to replace the methods we need.

However, modern javascript practices are moving towards modules having more private members. This is both a cultural change, and a technical change brought about by TypeScript. Additionally -- many projects are moving towards having multiple packages.

One example of this is Node Redis. Where older versions packaged everything under a single redis package

require('redis')

Modern versions have many packages

require('@redis/client')
require('@redis/bloom')
require('@redis/graph')
require('@redis/json')
require('@redis/search')
require('@redis/time-series')

Additionally -- the redis instrumentation requires that we instrument multiple individual exports to get at the methods and objects we need.

var MODULES = [
  [
  '@node-redis/client/dist/lib/client',
  '@node-redis/client/dist/lib/client/commands-queue',
  '@redis/client/dist/lib/client',
  '@redis/client/dist/lib/client/commands-queue',

We should modify our module loading code to allow for a logical grouping of names. Something like

var MODULES = {
    'redis': [
      'redis',
      '@node-redis/client/dist/lib/client',
      '@node-redis/client/dist/lib/client/commands-queue',
      '@redis/client/dist/lib/client',
      '@redis/client/dist/lib/client/commands-queue',
    ]
    // ...
}

The intent is to allow end-users to enable/disable instrumentation by this logic name without having to know each individual package that would need to be disabled.

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Oct 18, 2022
trentm added a commit that referenced this issue 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
trentm added a commit that referenced this issue Oct 11, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.7-candidate agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant