From 18f1601e13aa041e8c448840df70aacfb3ba5791 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 24 Jan 2022 15:09:43 -0800 Subject: [PATCH 1/7] feat: Drop unsampled transactions when connected to APM Server 8.0+ This updates to an elastic-apm-http-client that calls the APM Server Information API on creation to get the APM Server version. Closes: #2455 --- CHANGELOG.asciidoc | 2 + lib/instrumentation/index.js | 5 + lib/noop-transport.js | 4 + package.json | 3 +- test/_capturing_transport.js | 4 + test/_mock_apm_server.js | 19 ++- test/_mock_http_client.js | 3 + test/_mock_http_client_states.js | 3 + test/config.test.js | 4 + test/instrumentation/index.test.js | 83 ------------- test/outcome.test.js | 3 +- test/transaction-sampling.test.js | 189 +++++++++++++++++++++++++++++ 12 files changed, 234 insertions(+), 88 deletions(-) create mode 100644 test/transaction-sampling.test.js diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 3e6fbe1c06..8c867a43fd 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -37,6 +37,8 @@ Notes: [float] ===== Features +* Drop unsampled transactions when sending to APM Server v8.0+. ({issues}2455[#2455]) + * The default <> string (when it is not configured and cannot be inferred from a "package.json" file) has been changed from "nodejs_service" to "unknown-nodejs-service". This is a standardized pattern diff --git a/lib/instrumentation/index.js b/lib/instrumentation/index.js index 766a72cbbf..a95a16d889 100644 --- a/lib/instrumentation/index.js +++ b/lib/instrumentation/index.js @@ -281,6 +281,11 @@ Instrumentation.prototype.addEndedTransaction = function (transaction) { return } + // https://github.com/elastic/apm/blob/main/specs/agents/tracing-sampling.md#non-sampled-transactions + if (!transaction.sampled && !agent._transport.supportsKeepingUnsampledTransaction()) { + return + } + var payload = agent._transactionFilters.process(transaction._encode()) if (!payload) { agent.logger.debug('transaction ignored by filter %o', { trans: transaction.id, trace: transaction.traceId }) diff --git a/lib/noop-transport.js b/lib/noop-transport.js index dd02c049b8..baf341d92c 100644 --- a/lib/noop-transport.js +++ b/lib/noop-transport.js @@ -43,6 +43,10 @@ class NoopTransport { } } + supportsKeepingUnsampledTransaction () { + return false // Default to the behavior for APM Server >=8.0. + } + // Inherited from Writable, called in agent.js. destroy () {} } diff --git a/package.json b/package.json index dedb0813c6..fd0ff77d8e 100644 --- a/package.json +++ b/package.json @@ -78,6 +78,7 @@ "url": "https://github.com/elastic/apm-agent-nodejs/issues" }, "homepage": "https://github.com/elastic/apm-agent-nodejs", + "XXX elastic-apm-http-client": "^10.4.0", "dependencies": { "@elastic/ecs-pino-format": "^1.2.0", "after-all-results": "^2.0.0", @@ -86,7 +87,7 @@ "basic-auth": "^2.0.1", "cookie": "^0.4.0", "core-util-is": "^1.0.2", - "elastic-apm-http-client": "^10.3.0", + "elastic-apm-http-client": "file:../apm-nodejs-http-client4", "end-of-stream": "^1.4.4", "error-callsites": "^2.0.4", "error-stack-parser": "^2.0.6", diff --git a/test/_capturing_transport.js b/test/_capturing_transport.js index 56715fcd91..6ddd957d3f 100644 --- a/test/_capturing_transport.js +++ b/test/_capturing_transport.js @@ -77,6 +77,10 @@ class CapturingTransport { } } + supportsKeepingUnsampledTransaction () { + return false + } + // Inherited from Writable, called in agent.js. destroy () {} } diff --git a/test/_mock_apm_server.js b/test/_mock_apm_server.js index 0fe0b4ca8e..5ddcf8e6b2 100644 --- a/test/_mock_apm_server.js +++ b/test/_mock_apm_server.js @@ -1,7 +1,7 @@ // A mock APM server to use in tests. // // Usage: -// const server = new MockAPMServer() +// const server = new MockAPMServer(opts) // server.start(function (serverUrl) { // // Test code using `serverUrl`... // // - Events received on the intake API will be on `server.events`. @@ -16,9 +16,14 @@ const { URL } = require('url') const zlib = require('zlib') class MockAPMServer { - constructor () { + // - @param {Object} opts + // - {String} opts.apmServerVersion - The version to report in the + // "GET /" response body. Defaults to "8.0.0". + constructor (opts) { + opts = opts || {} this.clear() this.serverUrl = null // set in .start() + this.apmServerVersion = opts.apmServerVersion || '8.0.0' this._http = http.createServer(this._onRequest.bind(this)) } @@ -43,7 +48,15 @@ class MockAPMServer { instream.on('end', () => { let resBody = '' - if (parsedUrl.pathname === '/config/v1/agents') { + if (req.method === 'GET' && parsedUrl.pathname === '/') { + // https://www.elastic.co/guide/en/apm/server/current/server-info.html#server-info-endpoint + res.writeHead(200) + resBody = JSON.stringify({ + build_date: '2021-09-16T02:05:39Z', + build_sha: 'a183f675ecd03fca4a897cbe85fda3511bc3ca43', + version: this.apmServerVersion + }) + } else if (parsedUrl.pathname === '/config/v1/agents') { // Central config mocking. res.writeHead(200) resBody = '{}' diff --git a/test/_mock_http_client.js b/test/_mock_http_client.js index bfed7fe1a1..93ba4c06e5 100644 --- a/test/_mock_http_client.js +++ b/test/_mock_http_client.js @@ -54,6 +54,9 @@ module.exports = function (expected, done) { }, flush (cb) { if (cb) process.nextTick(cb) + }, + supportsKeepingUnsampledTransaction () { + return false } } diff --git a/test/_mock_http_client_states.js b/test/_mock_http_client_states.js index c2ffda4fd4..1aa9c69982 100644 --- a/test/_mock_http_client_states.js +++ b/test/_mock_http_client_states.js @@ -35,6 +35,9 @@ module.exports = function (expectations = [], done) { }, flush (cb) { if (cb) process.nextTick(cb) + }, + supportsKeepingUnsampledTransaction () { + return false } } } diff --git a/test/config.test.js b/test/config.test.js index f14d25f93c..fb7715d92f 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -934,6 +934,10 @@ test('custom transport', function (t) { flush (cb) { if (cb) setImmediate(cb) } + + supportsKeepingUnsampledTransaction () { + return false + } } const myTransport = new MyTransport() diff --git a/test/instrumentation/index.test.js b/test/instrumentation/index.test.js index e5738f1190..fcb2376e65 100644 --- a/test/instrumentation/index.test.js +++ b/test/instrumentation/index.test.js @@ -15,9 +15,7 @@ var http = require('http') var test = require('tape') -const logging = require('../../lib/logging') var mockClient = require('../_mock_http_client') -var Instrumentation = require('../../lib/instrumentation') var findObjInArray = require('../_utils').findObjInArray var origCaptureError = agent.captureError @@ -250,87 +248,6 @@ test('captureError should handle opts.captureAttributes', function (t) { agent.captureError(ex2, { captureAttributes: false }) }) -test('sampling', function (t) { - function generateSamples (rate, count) { - count = count || 1000 - var agent = { - _conf: { - transactionSampleRate: rate - }, - logger: logging.createLogger('off') - } - var ins = new Instrumentation(agent) - agent._instrumentation = ins - - var results = { - count: count, - sampled: 0, - unsampled: 0 - } - for (var i = 0; i < count; i++) { - var trans = ins.startTransaction() - if (trans && trans.sampled) { - results.sampled++ - } else { - results.unsampled++ - } - } - - return results - } - - function toRatios (samples) { - return { - count: samples.count, - sampled: samples.sampled / samples.count, - unsampled: samples.unsampled / samples.count - } - } - - var high = generateSamples(1.0) - t.ok(high.sampled > high.unsampled) - - var low = generateSamples(0.1) - t.ok(low.sampled < low.unsampled) - - var mid = toRatios(generateSamples(0.5)) - t.ok(mid.sampled > 0.4 && mid.sampled < 0.6) - t.ok(mid.unsampled > 0.4 && mid.unsampled < 0.6) - - t.end() -}) - -test('unsampled transactions do not include spans', function (t) { - resetAgent(1, function (data, cb) { - t.strictEqual(data.transactions.length, 1) - - data.transactions.forEach(function (trans) { - t.ok(/^[\da-f]{16}$/.test(trans.id)) - t.ok(/^[\da-f]{32}$/.test(trans.trace_id)) - t.ok(trans.duration > 0, 'duration should be >0ms') - t.ok(trans.duration < 100, 'duration should be <100ms') - t.notOk(Number.isNaN((new Date(trans.timestamp)).getTime())) - t.strictEqual(trans.sampled, false) - }) - - t.end() - }) - - agent._conf.transactionSampleRate = 0.0 - var ins = agent._instrumentation - - var trans = ins.startTransaction() - var span = ins.startSpan('span 0', 'type') - process.nextTick(function () { - if (span) span.end() - span = ins.startSpan('span 1', 'type') - process.nextTick(function () { - if (span) span.end() - trans.end() - }) - }) -}) - test('unsampled request transactions should have the correct result', function (t) { resetAgent(1, function (data) { t.strictEqual(data.transactions.length, 1) diff --git a/test/outcome.test.js b/test/outcome.test.js index 3368d1a4b4..98b211ae7b 100644 --- a/test/outcome.test.js +++ b/test/outcome.test.js @@ -13,7 +13,8 @@ const noOpClient = { sendTransaction () {}, sendError () {}, sendMetricSet () {}, - flush () {} + flush () {}, + supportsKeepingUnsampledTransaction () { return false } } agent._transport = noOpClient diff --git a/test/transaction-sampling.test.js b/test/transaction-sampling.test.js new file mode 100644 index 0000000000..258c326703 --- /dev/null +++ b/test/transaction-sampling.test.js @@ -0,0 +1,189 @@ +'use strict' + +// Test agent behavior with Transaction sampling. + +const tape = require('tape') + +const Agent = require('../lib/agent') +const { MockAPMServer } = require('./_mock_apm_server') + +const testAgentOpts = { + serviceName: 'test-transaction-sampling', + cloudProvider: 'none', + centralConfig: false, + captureExceptions: false, + captureSpanStackTraces: false, + metricsInterval: '0s', + logLevel: 'off' +} + +// ---- tests + +tape.test('various transactionSampleRate values', function (t) { + function startNTransactions (rate, count) { + const agent = new Agent().start(Object.assign( + {}, + testAgentOpts, + { + disableSend: true, + transactionSampleRate: rate + } + )) + + var results = { + count: count, + numSampled: 0, + numUnsampled: 0 + } + for (var i = 0; i < count; i++) { + var trans = agent.startTransaction('myTrans') + if (trans && trans.sampled) { + results.numSampled++ + } else { + results.numUnsampled++ + } + trans.end() + } + + agent.destroy() + return results + } + + let results = startNTransactions(1.0, 1000) + t.equal(results.numSampled, results.count, + 'with transactionSampleRate=1.0, all transactions were sampled') + t.equal(results.numUnsampled, 0) + + results = startNTransactions(0.1, 1000) + t.ok(Math.abs(results.numSampled / results.count - 0.1) < 0.1, // within 10% of expected 10% + 'with transactionSampleRate=0.1, ~10% transactions were sampled: ' + JSON.stringify(results)) + + results = startNTransactions(0.5, 1000) + t.ok(Math.abs(results.numSampled / results.count - 0.5) < 0.1, // within 10% of expected 50% + 'with transactionSampleRate=0.5, ~50% of transactions were sampled: ' + JSON.stringify(results)) + + t.end() +}) + +tape.test('APM Server =v8.0 (which does not want unsampled transactions)', function (suite) { + let agent + let apmServer + let serverUrl + + suite.test('setup mock APM server', function (t) { + apmServer = new MockAPMServer({ apmServerVersion: '8.0.0' }) + apmServer.start(function (serverUrl_) { + serverUrl = serverUrl_ + t.comment('mock APM serverUrl: ' + serverUrl) + t.end() + }) + }) + + suite.test('setup agent and wait for APM Server version fetch', function (t) { + agent = new Agent().start(Object.assign( + {}, + testAgentOpts, + { + serverUrl, + transactionSampleRate: 0 + } + )) + + // The agent's internal usage of client.supportsKeepingUnsampledTransaction() + // is the behavior being tested. That depends on the APM client having time + // to fetch the APM Server version before processing our test transaction. + // There isn't a mechanism exposed to wait for this, so we just wait a short + // while and poke into the internal APM client props. + setTimeout(function () { + t.ok(agent._transport._apmServerVersion, 'the agent APM client has fetched the APM Server version') + t.end() + }, 1000) + }) + + suite.test('unsampled transactions are not sent', function (t) { + // Start and end a transaction with some spans. + var t0 = agent.startTransaction('t0') + var s0 = agent.startSpan('s0', 'type') + process.nextTick(function () { + if (s0) s0.end() + var s1 = agent.startSpan('s1', 'type') + t.ok(s1 === null, 'no span should be started for an unsampled transaction') + process.nextTick(function () { + if (s1) s1.end() + t0.end() + + agent.flush(function () { + t.equal(apmServer.events.length, 0, 'no events were set to APM Server') + t.end() + }) + }) + }) + }) + + suite.test('teardown mock APM server', function (t) { + agent.destroy() + apmServer.close() + t.end() + }) + + suite.end() +}) From f636dc53610800a563c415dff43e02db7fb460ba Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 24 Jan 2022 15:13:57 -0800 Subject: [PATCH 2/7] use the feature-branch dep for now --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index fd0ff77d8e..64779d5601 100644 --- a/package.json +++ b/package.json @@ -87,7 +87,7 @@ "basic-auth": "^2.0.1", "cookie": "^0.4.0", "core-util-is": "^1.0.2", - "elastic-apm-http-client": "file:../apm-nodejs-http-client4", + "elastic-apm-http-client": "elastic/apm-nodejs-http-client#trentm/issue2455-drop-unsampled-trans", "end-of-stream": "^1.4.4", "error-callsites": "^2.0.4", "error-stack-parser": "^2.0.6", From dd9b0bfc6f74aa2affe8ef983c3ebbd424b0eed4 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 24 Jan 2022 15:51:58 -0800 Subject: [PATCH 3/7] get tests passing with node 12 on my mac --- lib/config.js | 3 ++- lib/noop-transport.js | 2 +- test/_capturing_transport.js | 2 +- test/_mock_http_client.js | 2 +- test/_mock_http_client_states.js | 2 +- test/agent.test.js | 7 ++++++- test/central-config-enabled.test.js | 3 +++ test/config.test.js | 2 +- test/fixtures/do-not-trace-self.js | 1 + test/instrumentation/index.test.js | 2 ++ test/integration/api-schema/basic.test.js | 1 + .../api-schema/capture-error-log-stack-traces.test.js | 1 + .../api-schema/capture-span-stack-traces.test.js | 1 + .../api-schema/source-lines-error-frames.test.js | 1 + .../api-schema/source-lines-span-frames.test.js | 1 + test/outcome.test.js | 2 +- 16 files changed, 25 insertions(+), 8 deletions(-) diff --git a/lib/config.js b/lib/config.js index 5b2fb14a01..9b490f4348 100644 --- a/lib/config.js +++ b/lib/config.js @@ -925,9 +925,10 @@ function getBaseClientConfig (conf, agent) { time: conf.apiRequestTime * 1000, maxQueueSize: conf.maxQueueSize, - // Debugging + // Debugging/testing options logger: clientLogger, payloadLogFile: conf.payloadLogFile, + apmServerVersion: conf.apmServerVersion, // Container conf containerId: conf.containerId, diff --git a/lib/noop-transport.js b/lib/noop-transport.js index baf341d92c..94b7bebb1c 100644 --- a/lib/noop-transport.js +++ b/lib/noop-transport.js @@ -44,7 +44,7 @@ class NoopTransport { } supportsKeepingUnsampledTransaction () { - return false // Default to the behavior for APM Server >=8.0. + return true } // Inherited from Writable, called in agent.js. diff --git a/test/_capturing_transport.js b/test/_capturing_transport.js index 6ddd957d3f..b21f430a35 100644 --- a/test/_capturing_transport.js +++ b/test/_capturing_transport.js @@ -78,7 +78,7 @@ class CapturingTransport { } supportsKeepingUnsampledTransaction () { - return false + return true } // Inherited from Writable, called in agent.js. diff --git a/test/_mock_http_client.js b/test/_mock_http_client.js index 93ba4c06e5..55e553f746 100644 --- a/test/_mock_http_client.js +++ b/test/_mock_http_client.js @@ -56,7 +56,7 @@ module.exports = function (expected, done) { if (cb) process.nextTick(cb) }, supportsKeepingUnsampledTransaction () { - return false + return true } } diff --git a/test/_mock_http_client_states.js b/test/_mock_http_client_states.js index 1aa9c69982..68f71d1992 100644 --- a/test/_mock_http_client_states.js +++ b/test/_mock_http_client_states.js @@ -37,7 +37,7 @@ module.exports = function (expectations = [], done) { if (cb) process.nextTick(cb) }, supportsKeepingUnsampledTransaction () { - return false + return true } } } diff --git a/test/agent.test.js b/test/agent.test.js index e01df120c1..52fab66588 100644 --- a/test/agent.test.js +++ b/test/agent.test.js @@ -553,7 +553,12 @@ test('filters', function (t) { filterAgentOpts = Object.assign( {}, agentOpts, - { serverUrl } + { + serverUrl, + // Ensure the APM client's `GET /` requests do not get in the way of + // the test asserts below. + apmServerVersion: '8.0.0' + } ) t.end() }) diff --git a/test/central-config-enabled.test.js b/test/central-config-enabled.test.js index a5f4358ac7..92af8e4aee 100644 --- a/test/central-config-enabled.test.js +++ b/test/central-config-enabled.test.js @@ -46,6 +46,7 @@ const runTestsWithServer = (t, updates, expect) => { serviceName: 'test', logLevel: 'off', // silence for cleaner test output captureExceptions: false, + apmServerVersion: '8.0.0', metricsInterval: 0, centralConfig: true }) @@ -159,6 +160,7 @@ test('agent.logger updates for central config `log_level` change', { timeout: 10 serverUrl: 'http://localhost:' + server.address().port, serviceName: 'test', captureExceptions: false, + apmServerVersion: '8.0.0', metricsInterval: 0, centralConfig: true, logLevel: 'warn' @@ -207,6 +209,7 @@ test('central config change does not erroneously update cloudProvider', { timeou cloudProvider: 'aws', // These settings to reduce some agent activity: captureExceptions: false, + apmServerVersion: '8.0.0', metricsInterval: 0 }) diff --git a/test/config.test.js b/test/config.test.js index fb7715d92f..ec4276e917 100644 --- a/test/config.test.js +++ b/test/config.test.js @@ -936,7 +936,7 @@ test('custom transport', function (t) { } supportsKeepingUnsampledTransaction () { - return false + return true } } const myTransport = new MyTransport() diff --git a/test/fixtures/do-not-trace-self.js b/test/fixtures/do-not-trace-self.js index a1a35322fb..a99a5314ab 100644 --- a/test/fixtures/do-not-trace-self.js +++ b/test/fixtures/do-not-trace-self.js @@ -4,6 +4,7 @@ var apm = require('../../').start({ // elastic-apm-node serviceName: 'test-do-not-trace-self', metricsInterval: 0, + apmServerVersion: '8.0.0', cloudProvider: 'none', centralConfig: false }) diff --git a/test/instrumentation/index.test.js b/test/instrumentation/index.test.js index fcb2376e65..4e68c4df12 100644 --- a/test/instrumentation/index.test.js +++ b/test/instrumentation/index.test.js @@ -249,6 +249,8 @@ test('captureError should handle opts.captureAttributes', function (t) { }) test('unsampled request transactions should have the correct result', function (t) { + // This test is relying on `resetAgent` creating an `agent._transaction` + // that returns true from `.supportsKeepingUnsampledTransaction()`. resetAgent(1, function (data) { t.strictEqual(data.transactions.length, 1) diff --git a/test/integration/api-schema/basic.test.js b/test/integration/api-schema/basic.test.js index 21977fc69c..d415ea8343 100644 --- a/test/integration/api-schema/basic.test.js +++ b/test/integration/api-schema/basic.test.js @@ -244,6 +244,7 @@ function newAgent (server) { serverUrl: 'http://localhost:' + server.address().port, captureExceptions: false, disableInstrumentations: ['http'], + apmServerVersion: '8.0.0', metricsInterval: 0, centralConfig: false }) diff --git a/test/integration/api-schema/capture-error-log-stack-traces.test.js b/test/integration/api-schema/capture-error-log-stack-traces.test.js index 37b95f2476..b3a31fe0cf 100644 --- a/test/integration/api-schema/capture-error-log-stack-traces.test.js +++ b/test/integration/api-schema/capture-error-log-stack-traces.test.js @@ -73,6 +73,7 @@ function newAgent (server) { disableInstrumentations: ['http'], captureErrorLogStackTraces: true, metricsInterval: 0, + apmServerVersion: '8.0.0', centralConfig: false }) } diff --git a/test/integration/api-schema/capture-span-stack-traces.test.js b/test/integration/api-schema/capture-span-stack-traces.test.js index abb438a885..7964027545 100644 --- a/test/integration/api-schema/capture-span-stack-traces.test.js +++ b/test/integration/api-schema/capture-span-stack-traces.test.js @@ -69,6 +69,7 @@ function newAgent (server) { captureExceptions: false, disableInstrumentations: ['http'], captureSpanStackTraces: false, + apmServerVersion: '8.0.0', metricsInterval: 0, centralConfig: false }) diff --git a/test/integration/api-schema/source-lines-error-frames.test.js b/test/integration/api-schema/source-lines-error-frames.test.js index 27cdaea198..a645587774 100644 --- a/test/integration/api-schema/source-lines-error-frames.test.js +++ b/test/integration/api-schema/source-lines-error-frames.test.js @@ -73,6 +73,7 @@ function newAgent (server) { disableInstrumentations: ['http'], sourceLinesErrorAppFrames: 0, sourceLinesErrorLibraryFrames: 0, + apmServerVersion: '8.0.0', metricsInterval: 0, centralConfig: false }) diff --git a/test/integration/api-schema/source-lines-span-frames.test.js b/test/integration/api-schema/source-lines-span-frames.test.js index d80abda420..42b5768d7d 100644 --- a/test/integration/api-schema/source-lines-span-frames.test.js +++ b/test/integration/api-schema/source-lines-span-frames.test.js @@ -70,6 +70,7 @@ function newAgent (server) { disableInstrumentations: ['http'], sourceLinesSpanAppFrames: 5, sourceLinesSpanLibraryFrames: 5, + apmServerVersion: '8.0.0', metricsInterval: 0, centralConfig: false }) diff --git a/test/outcome.test.js b/test/outcome.test.js index 98b211ae7b..aa6d0102f4 100644 --- a/test/outcome.test.js +++ b/test/outcome.test.js @@ -14,7 +14,7 @@ const noOpClient = { sendError () {}, sendMetricSet () {}, flush () {}, - supportsKeepingUnsampledTransaction () { return false } + supportsKeepingUnsampledTransaction () { return true } } agent._transport = noOpClient From 5d1572105e9d8678aaf2cadacbb00ab42abd52b1 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Mon, 24 Jan 2022 16:33:09 -0800 Subject: [PATCH 4/7] fix a possible cause of this test failing on Windows It was failing because there were two asserts for "server received client request": ``` [2022-01-25T00:10:48.113Z] running test: cd . && node --unhandled-rejections=strict test\integration\allow-invalid-cert.test.js [2022-01-25T00:10:48.690Z] TAP version 13 [2022-01-25T00:10:48.690Z] # should allow self signed certificate [2022-01-25T00:10:48.956Z] ok 1 server received client request [2022-01-25T00:10:48.956Z] {"log.level":"error","@timestamp":"2022-01-25T00:10:49.113Z","log":{"logger":"elastic-apm-node"},"ecs":{"version":"1.6.0"},"message":"APM Server transport error: APM Server information endpoint returned no body, often this indicates authentication (\"apiKey\" or \"secretToken\") is incorrect"} [2022-01-25T00:10:48.956Z] {"log.level":"info","@timestamp":"2022-01-25T00:10:49.118Z","log":{"logger":"elastic-apm-node"},"ecs":{"version":"1.6.0"},"message":"Sending error to Elastic APM: {\"id\":\"737eadd48ee233f960b8fa5f58c5b234\"}"} [2022-01-25T00:10:48.956Z] ok 2 server received client request [2022-01-25T00:10:48.956Z] ok 3 undefined [2022-01-25T00:10:48.956Z] ok 4 agent.captureError callback called [2022-01-25T00:10:48.956Z] not ok 5 plan != count ``` I'm guessing the extra one is from the APM Server version fetch. --- test/integration/allow-invalid-cert.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/allow-invalid-cert.test.js b/test/integration/allow-invalid-cert.test.js index 630133757a..f377b0e828 100644 --- a/test/integration/allow-invalid-cert.test.js +++ b/test/integration/allow-invalid-cert.test.js @@ -4,12 +4,12 @@ var getPort = require('get-port') getPort().then(function (port) { var agent = require('../../').start({ - serviceName: 'test', + serviceName: 'test-allow-invalid-cert', serverUrl: 'https://localhost:' + port, captureExceptions: false, metricsInterval: 0, centralConfig: false, - disableInstrumentations: ['https'], // avoid the agent instrumenting the mock APM Server + apmServerVersion: '8.0.0', verifyServerCert: false }) @@ -27,7 +27,7 @@ getPort().then(function (port) { server.listen(port, function () { agent.captureError(new Error('boom!'), function (err) { - t.error(err) + t.error(err, 'no error in captureError') t.pass('agent.captureError callback called') server.close() agent.destroy() From 085891031a9408ad2c623af7058c08887502b9d9 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 25 Jan 2022 10:56:31 -0800 Subject: [PATCH 5/7] fix test failure on (slower) Windows CI where the ver-fetch request surprised the test asserts --- test/integration/no-sampling.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/no-sampling.test.js b/test/integration/no-sampling.test.js index 16032a9027..fe8ebd83e0 100644 --- a/test/integration/no-sampling.test.js +++ b/test/integration/no-sampling.test.js @@ -5,12 +5,12 @@ var ndjson = require('ndjson') getPort().then(function (port) { var agent = require('../../').start({ - serviceName: 'test', + serviceName: 'test-no-sampling', serverUrl: 'http://localhost:' + port, captureExceptions: false, metricsInterval: 0, centralConfig: false, - disableInstrumentations: ['http'], // avoid the agent instrumenting the mock APM Server + apmServerVersion: '8.0.0', apiRequestTime: '1s' }) From 31796315f21161e24f3143bba5d08497941bbb4c Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 25 Jan 2022 11:20:37 -0800 Subject: [PATCH 6/7] more test updates to ensure the ver-fetch request doesn't cause a race in the testing --- test/integration/allow-invalid-cert.test.js | 1 + test/integration/no-sampling.test.js | 1 + test/integration/server-url-path.test.js | 1 + test/integration/skip-internal-http-spans.test.js | 1 + test/integration/socket-close.test.js | 1 + test/integration/verify-server-ca-cert.test.js | 1 + 6 files changed, 6 insertions(+) diff --git a/test/integration/allow-invalid-cert.test.js b/test/integration/allow-invalid-cert.test.js index f377b0e828..0c57608200 100644 --- a/test/integration/allow-invalid-cert.test.js +++ b/test/integration/allow-invalid-cert.test.js @@ -10,6 +10,7 @@ getPort().then(function (port) { metricsInterval: 0, centralConfig: false, apmServerVersion: '8.0.0', + disableInstrumentations: ['https'], // avoid the agent instrumenting the mock APM Server verifyServerCert: false }) diff --git a/test/integration/no-sampling.test.js b/test/integration/no-sampling.test.js index fe8ebd83e0..502ae1b0b6 100644 --- a/test/integration/no-sampling.test.js +++ b/test/integration/no-sampling.test.js @@ -11,6 +11,7 @@ getPort().then(function (port) { metricsInterval: 0, centralConfig: false, apmServerVersion: '8.0.0', + disableInstrumentations: ['http'], // avoid the agent instrumenting the mock APM Server apiRequestTime: '1s' }) diff --git a/test/integration/server-url-path.test.js b/test/integration/server-url-path.test.js index 70ffe4c75d..fb9ba60d04 100644 --- a/test/integration/server-url-path.test.js +++ b/test/integration/server-url-path.test.js @@ -9,6 +9,7 @@ getPort().then(function (port) { captureExceptions: false, metricsInterval: 0, centralConfig: false, + apmServerVersion: '8.0.0', disableInstrumentations: ['http'] // avoid the agent instrumenting the mock APM Server }) diff --git a/test/integration/skip-internal-http-spans.test.js b/test/integration/skip-internal-http-spans.test.js index bd6ac7667b..cc425b96c8 100644 --- a/test/integration/skip-internal-http-spans.test.js +++ b/test/integration/skip-internal-http-spans.test.js @@ -10,6 +10,7 @@ getPort().then(port => { serverUrl: 'http://localhost:' + port, captureExceptions: false, metricsInterval: 0, + apmServerVersion: '8.0.0', centralConfig: false }) diff --git a/test/integration/socket-close.test.js b/test/integration/socket-close.test.js index 2349c825a8..a4ea5c837f 100644 --- a/test/integration/socket-close.test.js +++ b/test/integration/socket-close.test.js @@ -9,6 +9,7 @@ getPort().then(function (port) { captureExceptions: false, metricsInterval: 0, centralConfig: false, + apmServerVersion: '8.0.0', disableInstrumentations: ['http'] // avoid the agent instrumenting the mock APM Server }) diff --git a/test/integration/verify-server-ca-cert.test.js b/test/integration/verify-server-ca-cert.test.js index df36331ef6..8bc9f0ca05 100644 --- a/test/integration/verify-server-ca-cert.test.js +++ b/test/integration/verify-server-ca-cert.test.js @@ -12,6 +12,7 @@ getPort().then(function (port) { captureExceptions: false, metricsInterval: 0, centralConfig: false, + apmServerVersion: '8.0.0', disableInstrumentations: ['https'], // avoid the agent instrumenting the mock APM Server serverCaCertFile: path.join(__dirname, 'cert.pem') // self-signed certificate }) From 653995911d615669b659cf09275bbde10acb37b7 Mon Sep 17 00:00:00 2001 From: Trent Mick Date: Tue, 25 Jan 2022 15:49:05 -0800 Subject: [PATCH 7/7] switch over to now-published elastic-apm-http-client@10.4.0 --- package.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/package.json b/package.json index 64779d5601..fb40715081 100644 --- a/package.json +++ b/package.json @@ -78,7 +78,6 @@ "url": "https://github.com/elastic/apm-agent-nodejs/issues" }, "homepage": "https://github.com/elastic/apm-agent-nodejs", - "XXX elastic-apm-http-client": "^10.4.0", "dependencies": { "@elastic/ecs-pino-format": "^1.2.0", "after-all-results": "^2.0.0", @@ -87,7 +86,7 @@ "basic-auth": "^2.0.1", "cookie": "^0.4.0", "core-util-is": "^1.0.2", - "elastic-apm-http-client": "elastic/apm-nodejs-http-client#trentm/issue2455-drop-unsampled-trans", + "elastic-apm-http-client": "^10.4.0", "end-of-stream": "^1.4.4", "error-callsites": "^2.0.4", "error-stack-parser": "^2.0.6",