Skip to content

feat: zero-configuration support by always successfully inferring a serviceName #2410

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 4, 2021
Merged

feat: zero-configuration support by always successfully inferring a serviceName #2410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fba2e35
feat: zero-configuration support by always successfully inferring a s…
trentm Nov 3, 2021
7d1d689
a guess at why the no-async-hooks tests all failed in Jenkins CI
trentm Nov 3, 2021
a61c911
Merge branch 'master' into trentm/zero-conf-issue1944
trentm Nov 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ This is similar to <<disable-send, `disableSend`>>, 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 <<service-name, `serviceName`>>. 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

Expand Down
6 changes: 2 additions & 4 deletions docs/agent-api.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,8 @@ otherwise returns `false`.

Get the configured <<service-name,`serviceName`>>. 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)`
Expand Down
12 changes: 9 additions & 3 deletions docs/configuration.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
48 changes: 25 additions & 23 deletions lib/agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to getting the agent a logger immediately upon instantiation. However, without this call to this._conf the agent will no longer have a configuration object between its initial module loading and the agent starting.

    const agent = require('elastic-apm-node');
    
    // will not be set
    console.log(agent._conf);
    
    agent.start({});
    
    // only now is it set
    console.log(agent._conf);

Are we OK breaking this not-explicit-but-a-bit-implicit API?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am. ;)

My first justification is that this is an internal/undocumented property. We cannot, IMHO, in general have internal code changes be disallowed by possible/unknown external usage.

The main justification is that agent._conf between Agent creation and agent.start(...) is incomplete -- a stronger word is that this _conf is wrong and misleading. External code should not be making behaviour decisions based on a config value that will be different after agent.start(...). It could lead to a more subtle bug. (The obvious exception here is the logLevel, which we need for internal logging before agent.start(). If a good use case for another exception comes up, I think having the specific use case would much better inform how we support it.)

The last justification is that I would be surprised if there is any meaningful usage of the Agent instance in customer code before agent.start(...) is called.


this._inflightEvents = new InflightEventSet()
this._instrumentation = new Instrumentation(this)
this._metrics = new Metrics(this)
Expand All @@ -46,6 +41,7 @@ function Agent () {
this._transport = null

this.lambda = elasticApmAwsLambda(this)
this.middleware = { connect: connect.bind(this) }
}

Object.defineProperty(Agent.prototype, 'currentTransaction', {
Expand Down Expand Up @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REVIEW NOTE: This is a guard against agent.destroy() being called before agent.start()

Error.stackTraceLimit = this._origStackTraceLimit
}
}
Expand All @@ -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') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REVIEW NOTE: This is a guard against agent._getStats() being called before agent.start()

stats.apmclient = this._transport._getStats()
}
return stats
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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, _, -, <space>)', 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') {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
}
})
Expand All @@ -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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REVIEW NOTE: This is a guard against calling agent.handleUncaughtExceptions() and then capturing an uncaught exception before agent.start()

console.error(err)
}

Expand Down
Loading