From fba2e35c5de67a772798e36774796e828c9c3fa3 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 2 Nov 2021 17:02:23 -0700 Subject: [PATCH 1/2] feat: zero-configuration support by always successfully inferring a serviceName If a bogus serviceName is explicitly configured, the agent will still log.error and de-activate. However, if the agent is inferring a serviceName, then with this change it will always succeed. This change improves inference from a package.json by dealing with some cases where a package.json "name" was an invalid serviceName. And then ultimately it falls back to "nodejs_service" if a name cannot otherwise be inferred. This changes Agent configuration somewhat. Before this change the agent would `this._config()` twice: once at Agent creation (resulting in a agent._conf that is incorrect if options are later passed to agent.start()), and once at agent.start(). This commit changes to only configure at agent.start(). A separate `config.configLogger()` handles setting `agent.logger` to a valid early logger when the Agent is created. All Agent methods have been updated as necessary to guard against usage before the agent is started. One visible semantic change is that `agent.getServiceName()` will now always return `undefined` before the agent is started. Arguably the earlier behaviour was a subtle bug, but the returned serviceName could be *wrong* if the agent was later started with a given serviceName. Closes: #1944 --- CHANGELOG.asciidoc | 11 + docs/agent-api.asciidoc | 6 +- docs/configuration.asciidoc | 12 +- lib/agent.js | 48 ++-- lib/config.js | 186 ++++++++++--- lib/instrumentation/index.js | 6 +- lib/logging.js | 3 +- package.json | 1 - test/agent.test.js | 8 +- test/config.test.js | 247 ++++++++---------- test/filters.test.js | 6 + test/fixtures/pkg-zero-conf-noname/index.js | 4 + .../pkg-zero-conf-noname/package.json | 4 + test/fixtures/pkg-zero-conf-nsname/index.js | 4 + .../pkg-zero-conf-nsname/package.json | 5 + test/fixtures/pkg-zero-conf-sanitize/index.js | 4 + .../pkg-zero-conf-sanitize/package.json | 5 + test/fixtures/pkg-zero-conf-valid/index.js | 4 + .../fixtures/pkg-zero-conf-valid/package.json | 5 + test/fixtures/pkg-zero-conf-weird/index.js | 4 + .../fixtures/pkg-zero-conf-weird/package.json | 5 + test/redact-secrets/is-secret.test.js | 6 + test/redact-secrets/redact-secrets.test.js | 6 + test/start/env/test.test.js | 4 +- test/start/file/test.test.js | 4 +- 25 files changed, 380 insertions(+), 218 deletions(-) create mode 100644 test/fixtures/pkg-zero-conf-noname/index.js create mode 100644 test/fixtures/pkg-zero-conf-noname/package.json create mode 100644 test/fixtures/pkg-zero-conf-nsname/index.js create mode 100644 test/fixtures/pkg-zero-conf-nsname/package.json create mode 100644 test/fixtures/pkg-zero-conf-sanitize/index.js create mode 100644 test/fixtures/pkg-zero-conf-sanitize/package.json create mode 100644 test/fixtures/pkg-zero-conf-valid/index.js create mode 100644 test/fixtures/pkg-zero-conf-valid/package.json create mode 100644 test/fixtures/pkg-zero-conf-weird/index.js create mode 100644 test/fixtures/pkg-zero-conf-weird/package.json diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 79886da6a8..4ac45e44a8 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -60,6 +60,17 @@ This is similar to <>, but differs in that https://github.com/elastic/apm/issues/509[helpful for APM Server log analysis]. ({issues}2364[#2364]) +* Zero configuration support. The only required agent configuration option + is <>. Normally the agent will attempt to + infer `serviceName` for the "name" field in a package.json file. However, + that could fail. With this version, the agent will cope with: a scoped + package name (`@scope/name` is normalized to `scope-name`), a "name" that + isn't a valid `serviceName`, not being able to find a "package.json" file, + etc. Ultimately it will fallback to "nodejs_service". ({issues}1944[#1944]) ++ +One consequence of this change is that `apm.getServiceName()` will return +`undefined` until the agent is started (check with `apm.isStarted()`). + [float] ===== Bug fixes diff --git a/docs/agent-api.asciidoc b/docs/agent-api.asciidoc index 123f12b866..252bd92731 100644 --- a/docs/agent-api.asciidoc +++ b/docs/agent-api.asciidoc @@ -67,10 +67,8 @@ otherwise returns `false`. Get the configured <>. If a service name was not explicitly configured, this value may have been automatically determined. - -This may be called before `agent.start()`, but the values before and after -might differ as config is reloaded during and can be set from options given to -`.start()`. A misconfigured agent can have an `undefined` service name. +The service name is not determined until `agent.start()`, so will be `undefined` +until then. A misconfigured agent can have a `null` service name. [[apm-set-framework]] ==== `apm.setFramework(options)` diff --git a/docs/configuration.asciidoc b/docs/configuration.asciidoc index 29dc81eade..2c48d6d63c 100644 --- a/docs/configuration.asciidoc +++ b/docs/configuration.asciidoc @@ -20,13 +20,19 @@ the agent will use the `name` from `package.json` by default if available. ==== `serviceName` * *Type:* String -* *Default:* `name` field of `package.json` +* *Default:* `name` field of `package.json`, or "nodejs_service" * *Env:* `ELASTIC_APM_SERVICE_NAME` -Your Elastic APM service name. - +The name to identify this service in Elastic APM. Multiple instances of the +same service should use the same name. Allowed characters: `a-z`, `A-Z`, `0-9`, `-`, `_`, and space. +If serviceName is not provided, the agent will attempt to use the "name" field +from "package.json" -- looking up from the current working directory. The name +will be normalized to the allowed characters. If the name cannot be inferred +from package.json, then a fallback value of "nodejs_service" is used. + + [float] [[service-node-name]] ==== `serviceNodeName` diff --git a/lib/agent.js b/lib/agent.js index 58ebf99339..bfde1b219a 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -26,17 +26,12 @@ var version = require('../package').version module.exports = Agent function Agent () { - this.middleware = { connect: connect.bind(this) } + // Early configuration to ensure `agent.logger` works before `agent.start()`. + this.logger = config.configLogger() - this.logger = null this._conf = null this._httpClient = null this._uncaughtExceptionListener = null - - // Early configuration to ensure `agent.logger` works before `.start()` - // is called. - this._config() - this._inflightEvents = new InflightEventSet() this._instrumentation = new Instrumentation(this) this._metrics = new Metrics(this) @@ -46,6 +41,7 @@ function Agent () { this._transport = null this.lambda = elasticApmAwsLambda(this) + this.middleware = { connect: connect.bind(this) } } Object.defineProperty(Agent.prototype, 'currentTransaction', { @@ -113,7 +109,7 @@ Agent.prototype.destroy = function () { // for tests that may use multiple Agent instances in a single test process. global[symbols.agentInitialized] = null - if (Error.stackTraceLimit === this._conf.stackTraceLimit) { + if (this._origStackTraceLimit && Error.stackTraceLimit !== this._origStackTraceLimit) { Error.stackTraceLimit = this._origStackTraceLimit } } @@ -127,7 +123,7 @@ Agent.prototype._getStats = function () { const stats = { frameCache: frameCacheStats } - if (typeof this._transport._getStats === 'function') { + if (this._transport && typeof this._transport._getStats === 'function') { stats.apmclient = this._transport._getStats() } return stats @@ -186,7 +182,7 @@ Agent.prototype.setSpanOutcome = function (outcome) { } Agent.prototype._config = function (opts) { - this._conf = config(opts) + this._conf = config.createConfig(opts, this.logger) this.logger = this._conf.logger const { host, port, protocol } = this._conf.serverUrl @@ -204,7 +200,9 @@ Agent.prototype.isStarted = function () { } Agent.prototype.start = function (opts) { - if (this.isStarted()) throw new Error('Do not call .start() more than once') + if (this.isStarted()) { + throw new Error('Do not call .start() more than once') + } global[symbols.agentInitialized] = true this._config(opts) @@ -217,15 +215,11 @@ Agent.prototype.start = function (opts) { this.logger.debug('Elastic APM agent disabled (`active` is false)') return this } else if (!this._conf.serviceName) { - this.logger.error('Elastic APM isn\'t correctly configured: Missing serviceName') - this._conf.active = false - return this - } else if (!/^[a-zA-Z0-9 _-]+$/.test(this._conf.serviceName)) { - this.logger.error('Elastic APM isn\'t correctly configured: serviceName "%s" contains invalid characters! (allowed: a-z, A-Z, 0-9, _, -, )', this._conf.serviceName) + this.logger.error('Elastic APM is incorrectly configured: Missing serviceName (APM will be disabled)') this._conf.active = false return this } else if (!(this._conf.serverPort >= 1 && this._conf.serverPort <= 65535)) { - this.logger.error('Elastic APM isn\'t correctly configured: serverUrl "%s" contains an invalid port! (allowed: 1-65535)', this._conf.serverUrl) + this.logger.error('Elastic APM is incorrectly configured: serverUrl "%s" contains an invalid port! (allowed: 1-65535)', this._conf.serverUrl) this._conf.active = false return this } else if (this._conf.logLevel === 'trace') { @@ -266,11 +260,13 @@ Agent.prototype.start = function (opts) { } Agent.prototype.getServiceName = function () { - return this._conf.serviceName + return this._conf ? this._conf.serviceName : undefined } Agent.prototype.setFramework = function ({ name, version, overwrite = true }) { - if (!this._transport || !this._conf) return + if (!this._transport || !this._conf) { + return + } const conf = {} if (name && (overwrite || !this._conf.frameworkName)) this._conf.frameworkName = conf.frameworkName = name if (version && (overwrite || !this._conf.frameworkVersion)) this._conf.frameworkVersion = conf.frameworkVersion = version @@ -408,6 +404,13 @@ Agent.prototype.captureError = function (err, opts, cb) { const id = errors.generateErrorId() + if (!this.isStarted()) { + if (cb) { + cb(new Error('cannot capture error before agent is started'), id) + } + return + } + // Avoid unneeded error/stack processing if only propagating trace-context. if (this._conf.contextPropagationOnly) { if (cb) { @@ -534,8 +537,7 @@ Agent.prototype.captureError = function (err, opts, cb) { } else { inflightEvents.delete(id) if (cb) { - // TODO: Swallow this error just as it's done in agent.flush()? - cb(new Error('cannot capture error before agent is started'), id) + cb(new Error('cannot send error: missing transport'), id) } } }) @@ -556,9 +558,9 @@ Agent.prototype.handleUncaughtExceptions = function (cb) { this._uncaughtExceptionListener = function (err) { agent.logger.debug({ err }, 'Elastic APM caught unhandled exception') // The stack trace of uncaught exceptions are normally written to STDERR. - // The `uncaughtException` listener inhibits this behavor, and it's + // The `uncaughtException` listener inhibits this behavior, and it's // therefore necessary to manually do this to not break expectations. - if (agent._conf.logUncaughtExceptions === true) { + if (agent._conf && agent._conf.logUncaughtExceptions === true) { console.error(err) } diff --git a/lib/config.js b/lib/config.js index 0c9e18bd23..5a5a14fa9f 100644 --- a/lib/config.js +++ b/lib/config.js @@ -18,13 +18,6 @@ const { isLambdaExecutionEnviornment } = require('./lambda') let confFile = loadConfigFile() -let serviceName, serviceVersion -try { - const { name, version } = readPkgUp.sync().packageJson - serviceName = name - serviceVersion = version -} catch (err) {} - const INTAKE_STRING_MAX_SIZE = 1024 const CAPTURE_ERROR_LOG_STACK_TRACES_NEVER = 'never' const CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES = 'messages' @@ -226,12 +219,39 @@ var KEY_VALUE_OPTS = [ 'globalLabels' ] -function config (opts) { - return new Config(opts) +// Configure a logger for the agent. +// +// This is separate from `createConfig` to allow the agent to have an early +// logger before `agent.start()` is called. +function configLogger (opts) { + const logLevel = ( + process.env[ENV_TABLE.logLevel] || + (opts && opts.logLevel) || + (confFile && confFile.logLevel) || + DEFAULTS.logLevel + ) + + // `ELASTIC_APM_LOGGER=false` is provided as a mechanism to *disable* a + // custom logger for troubleshooting because a wrapped custom logger does + // not get structured log data. + // https://www.elastic.co/guide/en/apm/agent/nodejs/current/troubleshooting.html#debug-mode + let customLogger = null + if (process.env.ELASTIC_APM_LOGGER !== 'false') { + customLogger = ( + (opts && opts.logger) || + (confFile && confFile.logger) + ) + } + + return logging.createLogger(logLevel, customLogger) +} + +function createConfig (opts, logger) { + return new Config(opts, logger) } class Config { - constructor (opts) { + constructor (opts, logger) { this.ignoreUrlStr = [] this.ignoreUrlRegExp = [] this.ignoreUserAgentStr = [] @@ -239,6 +259,7 @@ class Config { this.transactionIgnoreUrlRegExp = [] this.sanitizeFieldNamesRegExp = [] this.ignoreMessageQueuesRegExp = [] + // If we didn't find a config file on process boot, but a path to one is // provided as a config option, let's instead try to load that if (confFile === null && opts && opts.configFile) { @@ -253,19 +274,45 @@ class Config { readEnv() // options read from environment variables ) - // Custom logic for setting serviceName so that an empty string in the config - // doesn't overwrite the serviceName read from package.json - if (!this.serviceName) this.serviceName = serviceName - if (!this.serviceVersion) this.serviceVersion = serviceVersion - - // The logger is used in config process, so create it first. - // - // `ELASTIC_APM_LOGGER=false` is provided as a mechanism to *disable* a - // custom logger for troubleshooting because a wrapped custom logger does - // not get structured log data. - // https://www.elastic.co/guide/en/apm/agent/nodejs/current/troubleshooting.html#debug-mode + // The logger is used later in this function, so create/update it first. + // Unless a new custom `logger` was provided, we use the one created earlier + // in `configLogger()`. const customLogger = (process.env.ELASTIC_APM_LOGGER === 'false' ? null : this.logger) - this.logger = logging.createLogger(this.logLevel, customLogger) + if (!customLogger && logger) { + logging.setLogLevel(logger, this.logLevel) + this.logger = logger + } else { + this.logger = logging.createLogger(this.logLevel, customLogger) + } + + // Fallback and validation handling for `serviceName` and `serviceVersion`. + if (this.serviceName) { + // A value here means an explicit value was given. Error out if invalid. + try { + validateServiceName(this.serviceName) + } catch (err) { + this.logger.error('serviceName "%s" is invalid: %s', this.serviceName, err.message) + this.serviceName = null + } + } else { + // Zero-conf support: use package.json#name, else // "nodejs_service". + try { + this.serviceName = serviceNameFromPackageJson() + } catch (err) { + this.logger.warn(err.message) + } + if (!this.serviceName) { + this.serviceName = 'nodejs_service' + } + } + if (!this.serviceVersion) { + // Zero-conf support: use package.json#version, if possible. + try { + this.serviceVersion = serviceVersionFromPackageJson() + } catch (err) { + // pass + } + } normalize(this, this.logger) @@ -409,6 +456,78 @@ function readEnv () { return opts } +function validateServiceName (s) { + if (typeof s !== 'string') { + throw new Error('not a string') + } else if (!/^[a-zA-Z0-9 _-]+$/.test(s)) { + throw new Error('contains invalid characters (allowed: a-z, A-Z, 0-9, _, -, )') + } +} + +// getPkg() attempts to find a "package.json" file (looking up from the current +// working directory), read it, and return an object of the form: +// { +// path: "/the/full/path/to/package.json", +// packageJson: { /* the loaded package.json object */ } +// } +let pkgCache +function getPkg () { + if (pkgCache === undefined) { + pkgCache = readPkgUp.sync() || null + } + return pkgCache +} + +function serviceNameFromPackageJson () { + const pkg = getPkg() + if (!pkg) { + throw new Error(`could not infer serviceName: could not find package.json up from ${process.cwd()}`) + } + if (!pkg.packageJson.name) { + throw new Error(`could not infer serviceName: "${pkg.path}" does not contain a "name"`) + } + if (typeof pkg.packageJson.name !== 'string') { + throw new Error(`could not infer serviceName: "name" in "${pkg.path}" is not a string`) + } + let serviceName = pkg.packageJson.name + + // Normalize a namespaced npm package name, '@ns/name', to 'ns-name'. + const match = /^@([^/]+)\/([^/]+)$/.exec(serviceName) + if (match) { + serviceName = match[1] + '-' + match[2] + } + + // Sanitize, by replacing invalid service name chars with an underscore. + const SERVICE_NAME_BAD_CHARS = /[^a-zA-Z0-9 _-]/g + serviceName = serviceName.replace(SERVICE_NAME_BAD_CHARS, '_') + + // Disallow some weird sanitized values. For example, it is better to + // have the fallback "nodejs_service" than "_" or "____" or " ". + const ALL_NON_ALPHANUMERIC = /^[ _-]*$/ + if (ALL_NON_ALPHANUMERIC.test(serviceName)) { + serviceName = null + } + if (!serviceName) { + throw new Error(`could not infer serviceName from name="${pkg.packageJson.name}" in "${pkg.path}"`) + } + + return serviceName +} + +function serviceVersionFromPackageJson () { + const pkg = getPkg() + if (!pkg) { + throw new Error(`could not infer serviceVersion: could not find package.json up from ${process.cwd()}`) + } + if (!pkg.packageJson.version) { + throw new Error(`could not infer serviceName: "${pkg.path}" does not contain a "version"`) + } + if (typeof pkg.packageJson.version !== 'string') { + throw new Error(`could not infer serviceName: "version" in "${pkg.path}" is not a string`) + } + return pkg.packageJson.version +} + function normalize (opts, logger) { normalizeKeyValuePairs(opts) normalizeNumbers(opts) @@ -807,13 +926,16 @@ function getBaseClientConfig (conf, agent) { } // Exports. -module.exports = config -module.exports.INTAKE_STRING_MAX_SIZE = INTAKE_STRING_MAX_SIZE -module.exports.CAPTURE_ERROR_LOG_STACK_TRACES_NEVER = CAPTURE_ERROR_LOG_STACK_TRACES_NEVER -module.exports.CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES = CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES -module.exports.CAPTURE_ERROR_LOG_STACK_TRACES_ALWAYS = CAPTURE_ERROR_LOG_STACK_TRACES_ALWAYS - -// The following are exported for tests. -module.exports.DEFAULTS = DEFAULTS -module.exports.secondsFromTimeStr = secondsFromTimeStr -module.exports.userAgentFromConf = userAgentFromConf +module.exports = { + configLogger, + createConfig, + INTAKE_STRING_MAX_SIZE, + CAPTURE_ERROR_LOG_STACK_TRACES_NEVER, + CAPTURE_ERROR_LOG_STACK_TRACES_MESSAGES, + CAPTURE_ERROR_LOG_STACK_TRACES_ALWAYS, + + // The following are exported for tests. + DEFAULTS, + secondsFromTimeStr, + userAgentFromConf +} diff --git a/lib/instrumentation/index.js b/lib/instrumentation/index.js index a6d6f12d23..0f2967ce06 100644 --- a/lib/instrumentation/index.js +++ b/lib/instrumentation/index.js @@ -59,8 +59,7 @@ function Instrumentation (agent) { this._hook = null // this._hook is only exposed for testing purposes this._started = false this._runCtxMgr = null - - this._log = agent.logger.child({ 'event.module': 'instrumentation' }) + this._log = agent.logger // NOTE: we need to track module names for patches // in a separate array rather than using Object.keys() @@ -148,6 +147,9 @@ Instrumentation.prototype.start = function () { if (this._started) return this._started = true + // Could have changed in Agent.start(). + this._log = this._agent.logger + if (this._agent._conf.asyncHooks) { this._runCtxMgr = new AsyncHooksRunContextManager(this._log) } else { diff --git a/lib/logging.js b/lib/logging.js index b3605ccee8..77109ad65c 100644 --- a/lib/logging.js +++ b/lib/logging.js @@ -163,8 +163,9 @@ function setLogLevel (logger, levelName) { const pinoLevel = PINO_LEVEL_FROM_LEVEL_NAME[levelName] if (!pinoLevel) { logger.warn('unknown log levelName "%s": cannot setLogLevel', levelName) + } else { + logger.level = pinoLevel } - logger.level = pinoLevel } module.exports = { diff --git a/package.json b/package.json index 4697185ac3..9297f97dc2 100644 --- a/package.json +++ b/package.json @@ -172,7 +172,6 @@ "numeral": "^2.0.6", "nyc": "^15.0.0", "once": "^1.4.0", - "p-finally": "^2.0.1", "pg": "^8.7.1", "pug": "^2.0.4", "redis": "^3.0.2", diff --git a/test/agent.test.js b/test/agent.test.js index 33de6e6e4c..12b0e1ff6f 100644 --- a/test/agent.test.js +++ b/test/agent.test.js @@ -91,14 +91,10 @@ function deep (depth, n) { test('#getServiceName()', function (t) { const agent = new Agent() - // Before agent.start(), config will have already been loaded once, which - // typically means a `serviceName` determined from package.json. + // Before agent.start() the agent hasn't configured yet. t.ok(!agent.isStarted(), 'agent should not have been started yet') - t.strictEqual(agent.getServiceName(), packageJson.name) - t.strictEqual(agent.getServiceName(), agent._conf.serviceName) + t.strictEqual(agent.getServiceName(), undefined) - // After agent.start() config will be loaded again, this time with possible - // provided config. agent.start(Object.assign( {}, agentOptsNoopTransport, diff --git a/test/config.test.js b/test/config.test.js index 1247e02c3a..914ff0a74e 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -9,7 +9,6 @@ var util = require('util') var isRegExp = require('core-util-is').isRegExp var mkdirp = require('mkdirp') -var pFinally = require('p-finally') var rimraf = require('rimraf') var semver = require('semver') var test = require('tape') @@ -18,7 +17,7 @@ const Agent = require('../lib/agent') const { MockAPMServer } = require('./_mock_apm_server') const { NoopTransport } = require('../lib/noop-transport') const { safeGetPackageVersion } = require('./_utils') -var config = require('../lib/config') +const config = require('../lib/config') var Instrumentation = require('../lib/instrumentation') var apmVersion = require('../package').version var apmName = require('../package').name @@ -538,14 +537,6 @@ test('should overwrite option property active by ELASTIC_APM_ACTIVE', function ( t.end() }) -test('should default serviceName to package name', function (t) { - var agent = new Agent() - agent.start() - t.strictEqual(agent._conf.serviceName, 'elastic-apm-node') - agent.destroy() - t.end() -}) - test('should default to empty request blacklist arrays', function (t) { var agent = new Agent() agent.start(agentOptsNoopTransport) @@ -605,9 +596,16 @@ test('should compile wildcards from string', function (t) { }) test('invalid serviceName => inactive', function (t) { + var logger = new CaptureLogger() var agent = new Agent() - agent.start(Object.assign({}, agentOptsNoopTransport, { serviceName: 'foo&bar' })) - t.strictEqual(agent._conf.active, false) + agent.start(Object.assign( + {}, + agentOptsNoopTransport, + { serviceName: 'foo&bar', logger } + )) + t.ok(logger.calls[0].type === 'error' && logger.calls[0].message.indexOf('serviceName') !== -1, + 'there was a log.error mentioning "serviceName"') + t.strictEqual(agent._conf.active, false, 'active is false') agent.destroy() t.end() }) @@ -620,144 +618,105 @@ test('valid serviceName => active', function (t) { t.end() }) -test('serviceName defaults to package name', function (t) { - var mkdirpPromise = util.promisify(mkdirp) - var rimrafPromise = util.promisify(rimraf) - var writeFile = util.promisify(fs.writeFile) - var symlink = util.promisify(fs.symlink) - var exec = util.promisify(cp.exec) - - function testServiceConfig (pkg, handle) { - var tmp = path.join(os.tmpdir(), 'elastic-apm-node-test', String(Date.now())) - var files = [ - { - action: 'mkdirp', - dir: tmp - }, - { - action: 'create', - path: path.join(tmp, 'package.json'), - contents: JSON.stringify(pkg) - }, - { - action: 'create', - path: path.join(tmp, 'index.js'), - contents: ` - var apm = require('elastic-apm-node').start({ - centralConfig: false, - metricsInterval: '0s', - logLevel: 'off' - }) - console.log(JSON.stringify(apm._conf)) - ` - }, - { - action: 'mkdirp', - dir: path.join(tmp, 'node_modules') - } - ] - - if (process.platform === 'win32') { - files.push({ - action: 'npm link', - from: path.resolve(__dirname, '..'), - to: tmp - }) - } else { - files.push({ - action: 'symlink', - from: path.resolve(__dirname, '..'), - to: path.join(tmp, 'node_modules/elastic-apm-node') - }) - } - - // NOTE: Reduce the sequence to a promise chain rather - // than using Promise.all(), as the tasks are dependent. - let promise = files.reduce((p, file) => { - return p.then(() => { - switch (file.action) { - case 'create': { - return writeFile(file.path, file.contents) - } - case 'mkdirp': { - return mkdirpPromise(file.dir) - } - case 'symlink': { - return symlink(file.from, file.to) - } - case 'npm link': { - return exec('npm link', { - cwd: file.from - }).then(() => { - return exec('npm link elastic-apm-node', { - cwd: file.to - }) - }) - } - } - }) - }, Promise.resolve()) - - promise = promise - .then(() => { - return exec('node index.js', { - cwd: tmp - }) - }) - .then(result => { - return JSON.parse(result.stdout) - }) - .catch(err => { - t.error(err) - }) - - return pFinally(promise, () => { - return rimrafPromise(tmp) - }) - } - - t.test('should be active when valid', function (t) { - var pkg = { - name: 'valid' - } - - return testServiceConfig(pkg).then(conf => { - t.strictEqual(conf.active, true) - t.strictEqual(conf.serviceName, pkg.name) - t.end() - }) +test('serviceName/serviceVersion zero-conf: valid', function (t) { + cp.execFile(process.execPath, ['index.js'], { + timeout: 1000, + cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-valid') + }, function (err, stdout, stderr) { + t.error(err, 'no error running index.js') + t.equal(stderr, '', 'no stderr') + const lines = stdout.trim().split('\n') + const conf = JSON.parse(lines[lines.length - 1]) + t.equal(conf.serviceName, 'validName', 'serviceName was inferred from package.json') + t.equal(conf.serviceVersion, '1.2.3', 'serviceVersion was inferred from package.json') + t.end() }) +}) - t.test('should be inactive when blank', function (t) { - var pkg = { - name: '' - } - - return testServiceConfig(pkg).then(conf => { - t.strictEqual(conf.active, false) - t.strictEqual(conf.serviceName, pkg.name) - t.end() - }) +test('serviceName/serviceVersion zero-conf: no package.json to find', function (t) { + const indexJs = path.join(__dirname, 'fixtures', 'pkg-zero-conf-valid', 'index.js') + cp.execFile(process.execPath, [indexJs], { + timeout: 1000, + // Set CWD to top-level to ensure the agent cannot find a package.json file. + cwd: '/' + }, function (err, stdout, stderr) { + t.error(err, 'no error running index.js') + t.equal(stderr, '', 'no stderr') + const lines = stdout.trim().split('\n') + const conf = JSON.parse(lines[lines.length - 1]) + t.equal(conf.serviceName, 'nodejs_service', 'serviceName is the "nodejs_service" zero-conf fallback') + t.equal(conf.serviceVersion, undefined, 'serviceVersion is undefined') + t.end() }) +}) - t.test('should be inactive when missing', function (t) { - var pkg = {} +test('serviceName/serviceVersion zero-conf: no "name" in package.json', function (t) { + cp.execFile(process.execPath, ['index.js'], { + timeout: 1000, + cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-noname') + }, function (err, stdout, stderr) { + t.error(err, 'no error running index.js') + t.equal(stderr, '', 'no stderr') + const lines = stdout.trim().split('\n') + const conf = JSON.parse(lines[lines.length - 1]) + t.equal(conf.serviceName, 'nodejs_service', 'serviceName is the "nodejs_service" zero-conf fallback') + t.equal(conf.serviceVersion, '1.2.3', 'serviceVersion was inferred from package.json') + t.end() + }) +}) - return testServiceConfig(pkg).then(conf => { - t.strictEqual(conf.active, false) - t.end() - }) +// A package.json#name that uses a scoped npm name, e.g. @ns/name, should get +// a normalized serviceName='ns-name'. +test('serviceName/serviceVersion zero-conf: namespaced package name', function (t) { + cp.execFile(process.execPath, ['index.js'], { + timeout: 1000, + cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-nsname') + }, function (err, stdout, stderr) { + t.error(err, 'no error running index.js') + t.equal(stderr, '', 'no stderr') + const lines = stdout.trim().split('\n') + const conf = JSON.parse(lines[lines.length - 1]) + t.equal(conf.serviceName, 'ns-name', 'serviceName was inferred and normalized from package.json') + t.equal(conf.serviceVersion, '1.2.3', 'serviceVersion was inferred from package.json') + t.end() }) +}) - t.test('serviceVersion should default to package version', function (t) { - var pkg = { - version: '1.2.3' - } +test('serviceName/serviceVersion zero-conf: a package name that requires sanitization', function (t) { + cp.execFile(process.execPath, ['index.js'], { + timeout: 1000, + cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-sanitize') + }, function (err, stdout, stderr) { + t.error(err, 'no error running index.js') + t.equal(stderr, '', 'no stderr') + const lines = stdout.trim().split('\n') + const conf = JSON.parse(lines[lines.length - 1]) + // serviceName sanitization changes any disallowed char to an underscore. + // The pkg-zero-conf-sanitize/package.json has a name starting with the + // 7 characters that an npm package name can have, but a serviceName + // cannot. + // "name": "~*.!'()validNpmName" + t.equal(conf.serviceName, '_______validNpmName', 'serviceName was inferred and sanitized from package.json') + t.equal(conf.serviceVersion, '1.2.3', 'serviceVersion was inferred from package.json') + t.end() + }) +}) - return testServiceConfig(pkg).then(conf => { - t.strictEqual(conf.serviceVersion, pkg.version) - t.end() - }) +test('serviceName/serviceVersion zero-conf: weird "name" in package.json', function (t) { + cp.execFile(process.execPath, ['index.js'], { + timeout: 1000, + cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-weird') + }, function (err, stdout, stderr) { + t.error(err, 'no error running index.js') + t.equal(stderr, '', 'no stderr') + const lines = stdout.trim().split('\n') + const logWarn = JSON.parse(lines[0]) + t.ok(logWarn['log.level'] === 'warn' && logWarn.message.indexOf('serviceName') !== -1, + 'there is a log.warn about "serviceName"') + const conf = JSON.parse(lines[lines.length - 1]) + t.equal(conf.serviceName, 'nodejs_service', 'serviceName is the "nodejs_service" zero-conf fallback') + t.equal(conf.serviceVersion, '1.2.3', 'serviceVersion was inferred from package.json') + t.end() }) }) @@ -1125,7 +1084,7 @@ test('parsing of ARRAY and KEY_VALUE opts', function (t) { if (env) { process.env = Object.assign({}, origEnv, env) } - var cfg = config(opts) + var cfg = config.createConfig(opts) for (var field in expect) { t.deepEqual(cfg[field], expect[field], util.format('opts=%j env=%j -> %j', opts, env, expect)) @@ -1184,7 +1143,7 @@ test('transactionSampleRate precision', function (t) { if (env) { process.env = Object.assign({}, origEnv, env) } - var cfg = config(opts) + var cfg = config.createConfig(opts) for (var field in expect) { t.deepEqual(cfg[field], expect[field], util.format('opts=%j env=%j -> %j', opts, env, expect)) diff --git a/test/filters.test.js b/test/filters.test.js index 08612c5ed2..a53a4c3b05 100644 --- a/test/filters.test.js +++ b/test/filters.test.js @@ -1,5 +1,11 @@ 'use strict' +// `filterHttpHeaders` below indirectly (*very* indirectly) depends on there +// being a configured agent, because "is-secret.js" reaches into `agent._conf`. +require('..').start({ + disableSend: true +}) + const test = require('tape') const Filters = require('object-filter-sequence') diff --git a/test/fixtures/pkg-zero-conf-noname/index.js b/test/fixtures/pkg-zero-conf-noname/index.js new file mode 100644 index 0000000000..b4e5c4cd02 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-noname/index.js @@ -0,0 +1,4 @@ +const apm = require('../../..').start({ + disableSend: true +}) +console.log(JSON.stringify(apm._conf)) diff --git a/test/fixtures/pkg-zero-conf-noname/package.json b/test/fixtures/pkg-zero-conf-noname/package.json new file mode 100644 index 0000000000..a8d7893ce7 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-noname/package.json @@ -0,0 +1,4 @@ +{ + "version": "1.2.3", + "description": "a package without a 'name' field for testing serviceName and serviceVersion inference" +} diff --git a/test/fixtures/pkg-zero-conf-nsname/index.js b/test/fixtures/pkg-zero-conf-nsname/index.js new file mode 100644 index 0000000000..b4e5c4cd02 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-nsname/index.js @@ -0,0 +1,4 @@ +const apm = require('../../..').start({ + disableSend: true +}) +console.log(JSON.stringify(apm._conf)) diff --git a/test/fixtures/pkg-zero-conf-nsname/package.json b/test/fixtures/pkg-zero-conf-nsname/package.json new file mode 100644 index 0000000000..c2981f2ab8 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-nsname/package.json @@ -0,0 +1,5 @@ +{ + "name": "@ns/name", + "version": "1.2.3", + "description": "a package with a scoped npm name for testing serviceName and serviceVersion inference" +} diff --git a/test/fixtures/pkg-zero-conf-sanitize/index.js b/test/fixtures/pkg-zero-conf-sanitize/index.js new file mode 100644 index 0000000000..b4e5c4cd02 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-sanitize/index.js @@ -0,0 +1,4 @@ +const apm = require('../../..').start({ + disableSend: true +}) +console.log(JSON.stringify(apm._conf)) diff --git a/test/fixtures/pkg-zero-conf-sanitize/package.json b/test/fixtures/pkg-zero-conf-sanitize/package.json new file mode 100644 index 0000000000..c89018c2ab --- /dev/null +++ b/test/fixtures/pkg-zero-conf-sanitize/package.json @@ -0,0 +1,5 @@ +{ + "name": "~*.!'()validNpmName", + "version": "1.2.3", + "description": "a package with a valid npm name, but invalid for serviceName, to test serviceName inference" +} diff --git a/test/fixtures/pkg-zero-conf-valid/index.js b/test/fixtures/pkg-zero-conf-valid/index.js new file mode 100644 index 0000000000..b4e5c4cd02 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-valid/index.js @@ -0,0 +1,4 @@ +const apm = require('../../..').start({ + disableSend: true +}) +console.log(JSON.stringify(apm._conf)) diff --git a/test/fixtures/pkg-zero-conf-valid/package.json b/test/fixtures/pkg-zero-conf-valid/package.json new file mode 100644 index 0000000000..29b26065de --- /dev/null +++ b/test/fixtures/pkg-zero-conf-valid/package.json @@ -0,0 +1,5 @@ +{ + "name": "validName", + "version": "1.2.3", + "description": "a package with valid values for serviceName and serviceVersion inference" +} diff --git a/test/fixtures/pkg-zero-conf-weird/index.js b/test/fixtures/pkg-zero-conf-weird/index.js new file mode 100644 index 0000000000..b4e5c4cd02 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-weird/index.js @@ -0,0 +1,4 @@ +const apm = require('../../..').start({ + disableSend: true +}) +console.log(JSON.stringify(apm._conf)) diff --git a/test/fixtures/pkg-zero-conf-weird/package.json b/test/fixtures/pkg-zero-conf-weird/package.json new file mode 100644 index 0000000000..5a49efa9d8 --- /dev/null +++ b/test/fixtures/pkg-zero-conf-weird/package.json @@ -0,0 +1,5 @@ +{ + "name": "~~-~~", + "version": "1.2.3", + "description": "a package with a valid npm name that sanitizes to all non-alphanumeric for testing serviceName inference" +} diff --git a/test/redact-secrets/is-secret.test.js b/test/redact-secrets/is-secret.test.js index 026cec2936..165b055d05 100644 --- a/test/redact-secrets/is-secret.test.js +++ b/test/redact-secrets/is-secret.test.js @@ -22,6 +22,12 @@ // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. + +// `isSecret` below assumes there is a configured agent. +require('../..').start({ + disableSend: true +}) + var test = require('tape') var isSecret = require('../../lib/redact-secrets/is-secret') diff --git a/test/redact-secrets/redact-secrets.test.js b/test/redact-secrets/redact-secrets.test.js index 8c4caf2f20..17e2371245 100644 --- a/test/redact-secrets/redact-secrets.test.js +++ b/test/redact-secrets/redact-secrets.test.js @@ -22,6 +22,12 @@ // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE // SOFTWARE. + +// `redact` below assumes there is a configured agent. +require('../..').start({ + disableSend: true +}) + var test = require('tape') var clone = require('clone') var redact = require('../../lib/redact-secrets') diff --git a/test/start/env/test.test.js b/test/start/env/test.test.js index 39aa994576..b0513f3933 100644 --- a/test/start/env/test.test.js +++ b/test/start/env/test.test.js @@ -1,6 +1,8 @@ 'use strict' -var agent = require('../../..') +var agent = require('../../..').start({ + disableSend: true +}) const tape = require('tape') tape('from-env service name test', function (t) { diff --git a/test/start/file/test.test.js b/test/start/file/test.test.js index ef3b835cdd..86f072cee0 100644 --- a/test/start/file/test.test.js +++ b/test/start/file/test.test.js @@ -1,6 +1,8 @@ 'use strict' -var agent = require('../../..') +var agent = require('../../..').start({ + disableSend: true +}) const tape = require('tape') From 7d1d6892886a1ed1df10c01a86ad77b5801894ed Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Wed, 3 Nov 2021 08:15:28 -0700 Subject: [PATCH 2/2] a guess at why the no-async-hooks tests all failed in Jenkins CI --- test/config.test.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/config.test.js b/test/config.test.js index 914ff0a74e..287a781dd2 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -620,10 +620,10 @@ test('valid serviceName => active', function (t) { test('serviceName/serviceVersion zero-conf: valid', function (t) { cp.execFile(process.execPath, ['index.js'], { - timeout: 1000, + timeout: 3000, cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-valid') }, function (err, stdout, stderr) { - t.error(err, 'no error running index.js') + t.error(err, 'no error running index.js: ' + err) t.equal(stderr, '', 'no stderr') const lines = stdout.trim().split('\n') const conf = JSON.parse(lines[lines.length - 1]) @@ -636,11 +636,11 @@ test('serviceName/serviceVersion zero-conf: valid', function (t) { test('serviceName/serviceVersion zero-conf: no package.json to find', function (t) { const indexJs = path.join(__dirname, 'fixtures', 'pkg-zero-conf-valid', 'index.js') cp.execFile(process.execPath, [indexJs], { - timeout: 1000, + timeout: 3000, // Set CWD to top-level to ensure the agent cannot find a package.json file. cwd: '/' }, function (err, stdout, stderr) { - t.error(err, 'no error running index.js') + t.error(err, 'no error running index.js: ' + err) t.equal(stderr, '', 'no stderr') const lines = stdout.trim().split('\n') const conf = JSON.parse(lines[lines.length - 1]) @@ -652,10 +652,10 @@ test('serviceName/serviceVersion zero-conf: no package.json to find', function ( test('serviceName/serviceVersion zero-conf: no "name" in package.json', function (t) { cp.execFile(process.execPath, ['index.js'], { - timeout: 1000, + timeout: 3000, cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-noname') }, function (err, stdout, stderr) { - t.error(err, 'no error running index.js') + t.error(err, 'no error running index.js: ' + err) t.equal(stderr, '', 'no stderr') const lines = stdout.trim().split('\n') const conf = JSON.parse(lines[lines.length - 1]) @@ -669,10 +669,10 @@ test('serviceName/serviceVersion zero-conf: no "name" in package.json', function // a normalized serviceName='ns-name'. test('serviceName/serviceVersion zero-conf: namespaced package name', function (t) { cp.execFile(process.execPath, ['index.js'], { - timeout: 1000, + timeout: 3000, cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-nsname') }, function (err, stdout, stderr) { - t.error(err, 'no error running index.js') + t.error(err, 'no error running index.js: ' + err) t.equal(stderr, '', 'no stderr') const lines = stdout.trim().split('\n') const conf = JSON.parse(lines[lines.length - 1]) @@ -684,10 +684,10 @@ test('serviceName/serviceVersion zero-conf: namespaced package name', function ( test('serviceName/serviceVersion zero-conf: a package name that requires sanitization', function (t) { cp.execFile(process.execPath, ['index.js'], { - timeout: 1000, + timeout: 3000, cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-sanitize') }, function (err, stdout, stderr) { - t.error(err, 'no error running index.js') + t.error(err, 'no error running index.js: ' + err) t.equal(stderr, '', 'no stderr') const lines = stdout.trim().split('\n') const conf = JSON.parse(lines[lines.length - 1]) @@ -704,10 +704,10 @@ test('serviceName/serviceVersion zero-conf: a package name that requires sanitiz test('serviceName/serviceVersion zero-conf: weird "name" in package.json', function (t) { cp.execFile(process.execPath, ['index.js'], { - timeout: 1000, + timeout: 3000, cwd: path.join(__dirname, 'fixtures', 'pkg-zero-conf-weird') }, function (err, stdout, stderr) { - t.error(err, 'no error running index.js') + t.error(err, 'no error running index.js: ' + err) t.equal(stderr, '', 'no stderr') const lines = stdout.trim().split('\n') const logWarn = JSON.parse(lines[0])