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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
feat: change in approach
Alan Storm committed Sep 6, 2022

Verified

This commit was signed with the committer’s verified signature.
mcollina Matteo Collina
commit ccc92e58ddb4346ca1ea39f53dac55d72910e53d
6 changes: 3 additions & 3 deletions lib/agent.js
Original file line number Diff line number Diff line change
@@ -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')

@@ -36,7 +36,7 @@ module.exports = Agent
function Agent () {
// Early configuration to ensure `agent.logger` works before `agent.start()`.
this.logger = config.configLogger()

initStackTraceCollection()
// Get an initial pre-.start() configuration of agent defaults. This is a
// crutch for Agent APIs that depend on `agent._conf`.
this._conf = config.initialConfig(this.logger)
@@ -219,7 +219,7 @@ Agent.prototype.start = function (opts) {
}
// load error-callsites on start
// https://github.com/elastic/apm-agent-nodejs/issues/2833
require('error-callsites')

global[symbols.agentInitialized] = true

this._config(opts)
14 changes: 8 additions & 6 deletions lib/stacktraces.js
Original file line number Diff line number Diff line change
@@ -19,11 +19,12 @@ var path = require('path')
const asyncCache = require('async-cache')
const afterAllResults = require('after-all-results')

// lazy load error-callsites to avoid Error.prepareStackTrace
// side-effects until agent's been started
// avoid loading error-callsites until needed to avoid
// Error.prepareStackTrace side-effects
// https://github.com/elastic/apm-agent-nodejs/issues/2833
const errorCallsites = function () {
return require('error-callsites').apply(this, arguments)
let errorCallsites
function initStackTraceCollection () {
errorCallsites = require('error-callsites')
}
const errorStackParser = require('error-stack-parser')
const loadSourceMap = require('./load-source-map')
@@ -414,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.
@@ -430,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) {
@@ -453,6 +454,7 @@ function gatherStackTrace (log, err, sourceLinesAppFrames, sourceLinesLibraryFra
module.exports = {
gatherStackTrace,
frameCacheStats,
initStackTraceCollection,

// Exported for testing only.
stackTraceFromErrStackString
25 changes: 2 additions & 23 deletions test/stacktraces/stacktraces.test.js
Original file line number Diff line number Diff line change
@@ -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')

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

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

tape.test('delayed error-callsite loading', function (t) {
// const server = new MockAPMServer()
// server.start(function (serverUrl) {
execFile(
process.execPath,
['fixtures/throw-an-error-no-agent.js'],
{
cwd: __dirname,
timeout: 10000 // sanity stop, 3s is sometimes too short for CI
},
function done (err, _stdout, _stderr) {
t.ok(err, 'throw-an-error-no-agent.js errored out')
t.ok(/Error: boom/.test(err.message),
'err.message includes /Error: boom/: ' + JSON.stringify(err.message))
t.end()
console.log('----------')
console.log(_stderr)
console.log('----------')
}
)
})

suite.end()
})