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

trentm · 2021-11-03T00:09:44Z

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

Checklist

…erviceName 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

trentm

Add review notes.

trentm · 2021-11-03T00:10:41Z

lib/agent.js

  global[symbols.agentInitialized] = null

-  if (Error.stackTraceLimit === this._conf.stackTraceLimit) {
+  if (this._origStackTraceLimit && Error.stackTraceLimit !== this._origStackTraceLimit) {


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

trentm · 2021-11-03T00:10:54Z

lib/agent.js

    frameCache: frameCacheStats
  }
-  if (typeof this._transport._getStats === 'function') {
+  if (this._transport && typeof this._transport._getStats === 'function') {


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

trentm · 2021-11-03T00:11:56Z

lib/agent.js

+    // 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) {


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

trentm · 2021-11-03T00:12:25Z

lib/config.js

-  const { name, version } = readPkgUp.sync().packageJson
-  serviceName = name
-  serviceVersion = version
-} catch (err) {}


REVIEW NOTE: The equivalent of this was moved to getPkg() below.

trentm · 2021-11-03T00:13:45Z

lib/instrumentation/index.js

  this._runCtxMgr = null
-
-  this._log = agent.logger.child({ 'event.module': 'instrumentation' })
+  this._log = agent.logger


REVIEW NOTE: There was no value in the event.module field in the log records, so I dropped it rather than having to reproduce it in the Instrumentation.prototype.start change below.

trentm · 2021-11-03T00:14:21Z

lib/logging.js

+  } else {
+    logger.level = pinoLevel
  }
-  logger.level = pinoLevel


REVIEW NOTE: This was an existing small bug that was exposed by testing the configLogger() addition.

trentm · 2021-11-03T00:14:58Z

test/agent.test.js

  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)


REVIEW NOTE: This is the semantic change that was mentioned in the PR description and CHANGELOG message.

trentm · 2021-11-03T00:15:31Z

test/config.test.js

-  t.strictEqual(agent._conf.serviceName, 'elastic-apm-node')
-  agent.destroy()
-  t.end()
-})


REVIEW NOTE: This test is a duplicate of "serviceName/serviceVersion zero-conf: valid" below.

trentm · 2021-11-03T00:16:45Z

test/config.test.js

  t.end()
 })

-test('serviceName defaults to package name', function (t) {


REVIEW NOTE: This suite of test cases was replaced by the 'serviceName/serviceVersion zero-conf: ' test cases below. Instead of having all this goop to create directory trees and packages in which to test, I just created the various test scenarios in "test/fixtures/pkg-zero-conf-/" dirs.

trentm · 2021-11-03T00:18:35Z

test/filters.test.js

+require('..').start({
+  disableSend: true
+})
+


REVIEW NOTE: This file and a few test files below implicitly depended on a configured agent because this code reaches into the agent singleton to get its config:

apm-agent-nodejs/lib/redact-secrets/is-secret.js

Line 26 in ba570ba

const agent = require('../..')

A "configured agent" now means a "started agent", hence my starting an agent for these test files.

ghost · 2021-11-03T00:31:04Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-11-03T15:15:53.358+0000
Duration: 21 min 40 sec
Commit: 7d1d689

Test stats 🧪

Test	Results
Failed	0
Passed	22
Skipped	0
Total	22

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules
run benchmark tests : Run the benchmark test only.

astorm

A couple of questions about potential improvments, but I'd be OK with this going in as is.

astorm · 2021-11-03T19:08:06Z

lib/agent.js

-
-  // Early configuration to ensure `agent.logger` works before `.start()`
-  // is called.
-  this._config()


+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?

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.

lib/config.js

trentm self-assigned this Nov 3, 2021

github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Nov 3, 2021

trentm commented Nov 3, 2021

View reviewed changes

trentm requested a review from astorm November 3, 2021 00:19

a guess at why the no-async-hooks tests all failed in Jenkins CI

7d1d689

astorm approved these changes Nov 4, 2021

View reviewed changes

trentm mentioned this pull request Nov 4, 2021

use the main JS file's directory to search for package.json for serviceName inference #2420

Closed

Merge branch 'master' into trentm/zero-conf-issue1944

a61c911

trentm merged commit 5b833e6 into master Nov 4, 2021

trentm deleted the trentm/zero-conf-issue1944 branch November 4, 2021 23:29

This was referenced Nov 8, 2021

apm.startTransaction crash if agent not yet started #2425

Closed

fix: apm.startTransaction() crash if agent not yet started #2428

Merged

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

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

Uh oh!

Conversation

trentm commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

trentm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 3, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

astorm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

trentm commented Nov 3, 2021 •

edited

Loading

ghost commented Nov 3, 2021 •

edited by ghost

Loading