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

feat: delay loading error-callsites module #2906

Merged
merged 9 commits into from
Sep 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ paths are considered for body capture. ({pull}2873[#2873])
For example `require('node:http')` will now be instrumented.
({issues}2816[#2816])

- Agent will delay loading of the `error-callsites` module until agent start time,
and will not load the module if the agent is disabled/inactive. This prevents the
setting of an `Error.prepareStackTrace` handler until necessary for stacktrace
collection. ({issues}2833[#2833] {pull}2906[#2906])

[float]
===== Bug fixes

Expand Down
3 changes: 2 additions & 1 deletion lib/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ var { elasticApmAwsLambda } = require('./lambda')
var Metrics = require('./metrics')
var parsers = require('./parsers')
var symbols = require('./symbols')
const { frameCacheStats } = require('./stacktraces')
const { frameCacheStats, initStackTraceCollection } = require('./stacktraces')
const Span = require('./instrumentation/span')
const Transaction = require('./instrumentation/transaction')

Expand Down Expand Up @@ -261,6 +261,7 @@ Agent.prototype.start = function (opts) {
}, 'agent configured correctly')
}

initStackTraceCollection()
this._transport = this._conf.transport(this._conf, this)

let runContextClass
Expand Down
14 changes: 11 additions & 3 deletions lib/stacktraces.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,14 @@ var path = require('path')

const asyncCache = require('async-cache')
const afterAllResults = require('after-all-results')
const errorCallsites = require('error-callsites')

// avoid loading error-callsites until needed to avoid
// Error.prepareStackTrace side-effects
// https://github.com/elastic/apm-agent-nodejs/issues/2833
let errorCallsites
function initStackTraceCollection () {
errorCallsites = require('error-callsites')
}
const errorStackParser = require('error-stack-parser')
const loadSourceMap = require('./load-source-map')
const LRU = require('lru-cache')
Expand Down Expand Up @@ -408,7 +415,7 @@ function frameFromCallSite (log, callsite, cwd, sourceLinesAppFrames, sourceLine
function gatherStackTrace (log, err, sourceLinesAppFrames, sourceLinesLibraryFrames, filterCallSite, cb) {
// errorCallsites returns an array of v8 CallSite objects.
// https://v8.dev/docs/stack-trace-api#customizing-stack-traces
let callsites = errorCallsites(err)
let callsites = errorCallsites ? errorCallsites(err) : null

const next = afterAllResults(function finish (_err, stacktrace) {
// _err is always null from frameFromCallSite.
Expand All @@ -424,7 +431,7 @@ function gatherStackTrace (log, err, sourceLinesAppFrames, sourceLinesLibraryFra

if (!isValidCallsites(callsites)) {
// When can this happen? Another competing Error.prepareStackTrace breaking
// error-callsites?
// error-callsites? Also initStackTraceCollection not having been called.
log.debug('could not get valid callsites from error "%s"', err)
} else if (callsites) {
if (filterCallSite) {
Expand All @@ -447,6 +454,7 @@ function gatherStackTrace (log, err, sourceLinesAppFrames, sourceLinesLibraryFra
module.exports = {
gatherStackTrace,
frameCacheStats,
initStackTraceCollection,

// Exported for testing only.
stackTraceFromErrStackString
Expand Down
20 changes: 20 additions & 0 deletions test/stacktraces/fixtures/get-prepare-stacktrace.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright Elasticsearch B.V. and other contributors where applicable.
* Licensed under the BSD 2-Clause License; you may not use this file except in
* compliance with the BSD 2-Clause License.
*/

// print name of error.prepareStackTrace function to STDOUT
require('../../../').start({
serviceName: 'test-get-prepare-stacktrace',
logUncaughtExceptions: true,
metricsInterval: 0,
centralConfig: false,
logLevel: 'off'
})
function main () {
const name = Error.prepareStackTrace ? Error.prepareStackTrace.name : undefined
console.log(name)
}

main()
49 changes: 48 additions & 1 deletion test/stacktraces/stacktraces.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const tape = require('tape')

const logging = require('../../lib/logging')
const { MockAPMServer } = require('../_mock_apm_server')
const { gatherStackTrace, stackTraceFromErrStackString } = require('../../lib/stacktraces')
const { gatherStackTrace, initStackTraceCollection, stackTraceFromErrStackString } = require('../../lib/stacktraces')

const log = logging.createLogger('off')

Expand Down Expand Up @@ -304,6 +304,7 @@ tape.test('stackTraceFromErrStackString()', function (t) {
})

tape.test('gatherStackTrace()', function (suite) {
initStackTraceCollection()
function thisIsMyFunction () {
// before 2
// before 1
Expand Down Expand Up @@ -398,5 +399,51 @@ tape.test('gatherStackTrace()', function (suite) {
})
})

tape.test('Error.prepareStackTrace is set', function (t) {
const server = new MockAPMServer()
server.start(function (serverUrl) {
execFile(
process.execPath,
['fixtures/get-prepare-stacktrace.js'],
{
cwd: __dirname,
timeout: 3000,
env: Object.assign({}, process.env, {
ELASTIC_APM_ACTIVE: true
})
},
function done (err, _stdout, _stderr) {
t.ok(!err)
t.equals(_stdout.trim(), 'csPrepareStackTrace', 'Error.prepareStackTrace is set')
server.close()
t.end()
}
)
})
})

tape.test('Error.prepareStackTrace is not set', function (t) {
const server = new MockAPMServer()
server.start(function (serverUrl) {
execFile(
process.execPath,
['fixtures/get-prepare-stacktrace.js'],
{
cwd: __dirname,
timeout: 3000,
env: Object.assign({}, process.env, {
ELASTIC_APM_ACTIVE: false
})
},
function done (err, _stdout, _stderr) {
t.ok(!err)
t.equals(_stdout.trim(), 'undefined', 'Error.prepareStackTrace is set')
server.close()
t.end()
}
)
})
})

suite.end()
})