options to fail fast on module instrumentation and to override module version guards

[trentm]

It might be useful to support some config options for control over instrumented module version guards. Specifically:

1. a config option to have the APM agent fail fast/hard if a particular module cannot be instrumented
2. a config option to allow the user to override a version guard

background

Typically a particular package instrumentation (i.e. each file under https://github.com/elastic/apm-agent-nodejs/tree/master/lib/instrumentation/modules/) will begin something like this:

module.exports = function (cassandra, agent, { version, enabled }) {
  if (!enabled) return cassandra
  if (!semver.satisfies(version, '>=3 <5')) {
    agent.logger.debug('cassandra-driver version %s not supported - aborting...', version)
    return cassandra
  }
  // ...

The if (!enabled) ... is how the disableInstrumentations config var works.

The if (!semver.satisfies(version, '...')) ... I'm calling the "module version guard". The current considered best practice for this Node.js APM agent's module instrumentations is to be explicit about the supported version range. This means that typically the module version guard will explicitly have a "< $next_major_version" condition. (This "best practice" isn't currently written down, nor universal to all currently instrumented modules, and is still debatable. See #745 and #514.)

Using the cassandra example above, this practice means that we explicitly will instrument "cassandra-driver" versions 3.x and 4.x, but not 5.x or later. Should a [email protected] be released, then the current Node.js APM won't instrument it.

Pros:

• Theoretically if the APM agent says it will instrument a module, then it will do so properly.

Cons:

• When a user upgrades to a new major release of a particular module, then APM instrumentation will likely stop.
• Perhaps worse, this change will happen silently, because the only noticeable changes are a log message at debug-level, a level that is likely not enabled by most users (the default level is "info").

proposal

1. A config option that would allow a user of the Node.js APM agent to say "I know I want my package FOO to be instrumented, please fail if APM thinks it cannot instrument it." Something like requireInstrumentations:

require('elastic-apm-node').start({
    disableInstrumentations: ['bluebird'],
    requireInstrumentations: ['redis', 'cassandra-driver']
})

2. A config option that would allow a user of the Node.js APM agent to say "I know package FOO version $new_major isn't explicitly supported by this version, I'd like you to try anyway because it seems to work for me." Something like:

require('elastic-apm-node').start({
    disableInstrumentations: ['bluebird'],
    requireInstrumentations: ['redis', 'cassandra-driver'],
    experimentalOverrideInstrumentationVersionGuard: ['cassandra-driver']
})

I'm not married to the particular names. I prefixed "experimental" here because I think it is a feature that shouldn't persist in user code for very long. It is a bridge to be able to use an existing version of the Node.js APM agent until it updates to provide support for a $new_major release of a particular package. Whenever an override is used, the APM agent should always emit a log message at the "warning" level.

Note: I think my "2." might be mostly a dupe of #745

todos

• [ ] Discuss
• [ ] What, if anything, do the other Elastic APM agents provide around this?
• [ ] Ditto for other APM vendors?

[trentm]

Note: See this discuss topic where a config option for explicit pre-requireing of modules to instrument is suggested as a solution (or perhaps at best a work around) for automatic detection of ESM import 'foo' not currently being supported by the APM agent.

[HonzaMac]

We have currently have test-case which check those "satisfies" from instrument code. It would be great to expose "satisfies" from each instrumentation.

We need these checks because of Renovate Bot doing upgrades and we may loose APM tracing.

Using requireInstrumentations would be also great.

const commonPackageJson = require('./../../common/package.json');
const desPackageJson = require('./../../digital_evidence_storage/package.json');
const serverPackageJson = require('./../../server/package.json');

const moduleMap = {
  common: commonPackageJson,
  server: serverPackageJson,
  des: desPackageJson,
};

describe('elastic-apm-node', () => {
  it.each`
    packageName  | module      | dependencyType       | satisfies
    ${'ioredis'} | ${'common'} | ${'dependencies'}    | ${'>=2.0.0 <6.0.0'}
    ${'express'} | ${'common'} | ${'devDependencies'} | ${'^4.0.0'}
    ${'pg'}      | ${'server'} | ${'dependencies'}    | ${'>=4.0.0 <9.0.0'}
    ${'pg'}      | ${'des'}    | ${'dependencies'}    | ${'>=4.0.0 <9.0.0'}
  `('should support $packageName from $module', ({module, dependencyType, packageName, satisfies}) => {
    const {version} = semver.coerce(moduleMap[module][dependencyType][packageName]);

    expect(version).to.be.not.empty;
    expect(semver.satisfies(version, satisfies)).to.equal(
      true,
      `current repository version ${version} of ${packageName} is not supported "${satisfies}"`,
    );
  });
});

[trentm]

@HonzaMac Thanks. I like the idea of us having the "satisfies" version range for each supported module instrumentation in an accessible format. Perhaps something like:

const apm = require('elastic-apm-node')
console.log(apm.instrumentedModules) // an array of "module info" objects that have the require/import name, the supported version range and perhaps other fields

[HonzaMac]

I would probably prefer more special function:

const commonPackageJson = require('./../../common/package.json');
const apm = require('elastic-apm-node')
console.log(apm.isSupported(commonPackageJson['dependencies']['pg'])

In this case, I don't need to have dependency on my side on semver package and internal way of checking is on elastic-apm-node instead of user. elastic-apm-node already have dependency on semver.