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/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/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..94b7bebb1c 100644 --- a/lib/noop-transport.js +++ b/lib/noop-transport.js @@ -43,6 +43,10 @@ class NoopTransport { } } + supportsKeepingUnsampledTransaction () { + return true + } + // Inherited from Writable, called in agent.js. destroy () {} } diff --git a/package.json b/package.json index dedb0813c6..fb40715081 100644 --- a/package.json +++ b/package.json @@ -86,7 +86,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": "^10.4.0", "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..b21f430a35 100644 --- a/test/_capturing_transport.js +++ b/test/_capturing_transport.js @@ -77,6 +77,10 @@ class CapturingTransport { } } + supportsKeepingUnsampledTransaction () { + return true + } + // 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..55e553f746 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 true } } diff --git a/test/_mock_http_client_states.js b/test/_mock_http_client_states.js index c2ffda4fd4..68f71d1992 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 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 f14d25f93c..ec4276e917 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 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 e5738f1190..4e68c4df12 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,88 +248,9 @@ 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) { + // 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/allow-invalid-cert.test.js b/test/integration/allow-invalid-cert.test.js index 630133757a..0c57608200 100644 --- a/test/integration/allow-invalid-cert.test.js +++ b/test/integration/allow-invalid-cert.test.js @@ -4,11 +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, + apmServerVersion: '8.0.0', disableInstrumentations: ['https'], // avoid the agent instrumenting the mock APM Server verifyServerCert: false }) @@ -27,7 +28,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() 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/integration/no-sampling.test.js b/test/integration/no-sampling.test.js index 16032a9027..502ae1b0b6 100644 --- a/test/integration/no-sampling.test.js +++ b/test/integration/no-sampling.test.js @@ -5,11 +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, + 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 }) diff --git a/test/outcome.test.js b/test/outcome.test.js index 3368d1a4b4..aa6d0102f4 100644 --- a/test/outcome.test.js +++ b/test/outcome.test.js @@ -13,7 +13,8 @@ const noOpClient = { sendTransaction () {}, sendError () {}, sendMetricSet () {}, - flush () {} + flush () {}, + supportsKeepingUnsampledTransaction () { return true } } 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() +})