diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index cbc944b53b3..29e06046368 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -49,6 +49,11 @@ Notes: * Add `transaction.name` to captured APM errors. This will allow the Kibana APM app to correlate error groups and transaction groups. ({issues}2456[#2456]) +* Mark S3 spans (from 'aws-sdk' instrumentation) as exit spans (per + https://github.com/elastic/apm/blob/main/specs/agents/tracing-spans.md#exit-spans). + The result is that HTTP child spans of S3 spans are no longer captured. + ({issues}2125[#2125]) + [float] ===== Bug fixes diff --git a/docs/agent-api.asciidoc b/docs/agent-api.asciidoc index 240d92e3ee3..01b6d7ff937 100644 --- a/docs/agent-api.asciidoc +++ b/docs/agent-api.asciidoc @@ -643,6 +643,12 @@ Sub-millisecond precision can be achieved using decimals. If not provided, the current time will be used +** `exitSpan` +{type-boolean}+ Make an "exit span". +Exit spans represent outgoing communication. They are used to create a node +in the {kibana-ref}/service-maps.html[Service Map] and a downstream service +in the {kibana-ref}/dependencies.html[Dependencies Table]. The provided subtype +will be used as the downstream service name. + Start and return a new custom span associated with the current active transaction. This is the same as getting the current transaction with `apm.currentTransaction` and, if a transaction was found, diff --git a/docs/transaction-api.asciidoc b/docs/transaction-api.asciidoc index 4ab26171bed..a48a03bc756 100644 --- a/docs/transaction-api.asciidoc +++ b/docs/transaction-api.asciidoc @@ -113,6 +113,12 @@ Sub-millisecond precision can be achieved using decimals. If not provided, the current time will be used +** `exitSpan` +{type-boolean}+ Make an "exit span". +Exit spans represent outgoing communication. They are used to create a node +in the {kibana-ref}/service-maps.html[Service Map] and a downstream service +in the {kibana-ref}/dependencies.html[Dependencies Table]. The provided subtype +will be used as the downstream service name. + Start and return a new custom span associated with this transaction. When a span is started it will measure the time until <> is called. diff --git a/index.d.ts b/index.d.ts index 2d938130114..7ba7323cece 100644 --- a/index.d.ts +++ b/index.d.ts @@ -347,6 +347,7 @@ declare namespace apm { export interface SpanOptions { startTime?: number; childOf?: Transaction | Span | string; + exitSpan?: boolean; } type CaptureBody = 'off' | 'errors' | 'transactions' | 'all'; diff --git a/lib/agent.js b/lib/agent.js index cfdfc7443bb..17be1161670 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -171,7 +171,7 @@ Agent.prototype.setTransactionOutcome = function (outcome) { return this._instrumentation.setTransactionOutcome.apply(this._instrumentation, arguments) } -Agent.prototype.startSpan = function (name, type, subtype, action, { childOf } = {}) { +Agent.prototype.startSpan = function (name, type, subtype, action, { startTime, childOf, exitSpan } = {}) { return this._instrumentation.startSpan.apply(this._instrumentation, arguments) } diff --git a/lib/instrumentation/http-shared.js b/lib/instrumentation/http-shared.js index 3177fe77065..ffbe3de32f4 100644 --- a/lib/instrumentation/http-shared.js +++ b/lib/instrumentation/http-shared.js @@ -146,12 +146,12 @@ exports.traceOutgoingRequest = function (agent, moduleName, method) { if (!options.headers) options.headers = {} - // Attempt to use the span context as a traceparent header. - // If the transaction is unsampled the span will not exist, - // however a traceparent header must still be propagated - // to indicate requested services should not be sampled. - // Use the transaction context as the parent, in this case. - var parent = span || ins.currTransaction() + // W3C trace-context propagation. + // There are a number of reasons why `span` might be null: child of an + // exit span, `transactionMaxSpans` was hit, unsampled transaction, etc. + // If so, then fallback to the current run context's span or transaction, + // if any. + var parent = span || parentRunContext.currSpan() || parentRunContext.currTransaction() if (parent && parent._context) { const headerValue = parent._context.toTraceParentString() const traceStateValue = parent._context.toTraceStateString() diff --git a/lib/instrumentation/index.js b/lib/instrumentation/index.js index a95a16d8891..19a0f798f7b 100644 --- a/lib/instrumentation/index.js +++ b/lib/instrumentation/index.js @@ -83,7 +83,7 @@ Instrumentation.prototype.currTransaction = function () { if (!this._started) { return null } - return this._runCtxMgr.active().currTransaction() || null + return this._runCtxMgr.active().currTransaction() } Instrumentation.prototype.currSpan = function () { diff --git a/lib/instrumentation/modules/aws-sdk/s3.js b/lib/instrumentation/modules/aws-sdk/s3.js index afa38d04d86..da78ceecd32 100644 --- a/lib/instrumentation/modules/aws-sdk/s3.js +++ b/lib/instrumentation/modules/aws-sdk/s3.js @@ -54,7 +54,7 @@ function instrumentationS3 (orig, origArguments, request, AWS, agent, { version, const ins = agent._instrumentation - const span = ins.createSpan(name, TYPE, SUBTYPE, opName) + const span = ins.createSpan(name, TYPE, SUBTYPE, opName, { exitSpan: true }) if (!span) { return orig.apply(request, origArguments) } diff --git a/lib/instrumentation/span.js b/lib/instrumentation/span.js index 54de190f3ea..f5222803943 100644 --- a/lib/instrumentation/span.js +++ b/lib/instrumentation/span.js @@ -17,18 +17,21 @@ module.exports = Span util.inherits(Span, GenericSpan) function Span (transaction, name, ...args) { - const defaultChildOf = transaction._agent._instrumentation.currSpan() || transaction const opts = typeof args[args.length - 1] === 'object' ? (args.pop() || {}) : {} if (!opts.childOf) { + const defaultChildOf = transaction._agent._instrumentation.currSpan() || transaction opts.childOf = defaultChildOf opts.timer = defaultChildOf._timer } else if (opts.childOf._timer) { opts.timer = opts.childOf._timer } + this._exitSpan = !!opts.exitSpan + delete opts.exitSpan + GenericSpan.call(this, transaction._agent, ...args, opts) this._db = null diff --git a/lib/instrumentation/transaction.js b/lib/instrumentation/transaction.js index 357d0f19ca1..a136659c54f 100644 --- a/lib/instrumentation/transaction.js +++ b/lib/instrumentation/transaction.js @@ -116,8 +116,8 @@ Transaction.prototype.setCloudContext = function (cloudContext) { } // Create a span on this transaction and make it the current span. -Transaction.prototype.startSpan = function (...spanArgs) { - const span = this.createSpan(...spanArgs) +Transaction.prototype.startSpan = function (...args) { + const span = this.createSpan(...args) if (span) { this._agent._instrumentation.supersedeWithSpanRunContext(span) } @@ -129,7 +129,7 @@ Transaction.prototype.startSpan = function (...spanArgs) { // This does *not* replace the current run context to make this span the // "current" one. This allows instrumentations to avoid impacting the run // context of the calling code. Compare to `startSpan`. -Transaction.prototype.createSpan = function (...spanArgs) { +Transaction.prototype.createSpan = function (...args) { if (!this.sampled) { return null } @@ -142,8 +142,23 @@ Transaction.prototype.createSpan = function (...spanArgs) { return null } + // Exit spans must not have child spans. The spec allows child spans "that + // have the same type and subtype", but this agent currently has no use for + // this case. + // https://github.com/elastic/apm/blob/master/specs/agents/tracing-spans.md#child-spans-of-exit-spans + const opts = typeof args[args.length - 1] === 'object' + ? (args.pop() || {}) + : {} + opts.childOf = opts.childOf || this._agent._instrumentation.currSpan() || this + const childOf = opts.childOf + if (childOf instanceof Span && childOf._exitSpan) { + this._agent.logger.trace({ exitSpanId: childOf.id, newSpanArgs: args }, + 'createSpan: drop child span of exit span') + return null + } + this._builtSpans++ - return new Span(this, ...spanArgs) + return new Span(this, ...args, opts) } // Note that this only returns a complete result when called *during* the call diff --git a/test/instrumentation/modules/aws-sdk/s3.test.js b/test/instrumentation/modules/aws-sdk/s3.test.js index b90d41b6ef2..036e190cd73 100644 --- a/test/instrumentation/modules/aws-sdk/s3.test.js +++ b/test/instrumentation/modules/aws-sdk/s3.test.js @@ -61,14 +61,9 @@ tape.test('simple S3 usage scenario', function (t) { const tx = events.shift().transaction const errors = events.filter(e => e.error).map(e => e.error) - // Currently HTTP spans under each S3 span are included. Eventually - // those will be excluded. Filter those out for now. - // https://github.com/elastic/apm-agent-nodejs/issues/2125 + // Compare some common fields across all spans. const spans = events.filter(e => e.span) .map(e => e.span) - .filter(e => e.subtype !== 'http') - - // Compare some common fields across all spans. t.equal(spans.filter(s => s.trace_id === tx.trace_id).length, spans.length, 'all spans have the same trace_id') t.equal(spans.filter(s => s.transaction_id === tx.id).length, diff --git a/test/types/index.ts b/test/types/index.ts index 60b462b5f71..837617ac6c6 100644 --- a/test/types/index.ts +++ b/test/types/index.ts @@ -106,6 +106,7 @@ apm.startSpan('foo', 'type', { childOf: 'baz' }) apm.startSpan('foo', 'type', 'subtype', { childOf: 'baz' }) apm.startSpan('foo', 'type', 'subtype', 'action', { childOf: 'baz' }) apm.startSpan('foo', 'type', 'subtype', 'action', { startTime: 42 }) +apm.startSpan('foo', 'type', 'subtype', { exitSpan: true }) apm.setLabel('foo', 'bar') apm.setLabel('foo', 1) @@ -180,10 +181,13 @@ apm.logger.fatal('') trans.startSpan('foo', 'type', { childOf: 'baz' }) trans.startSpan('foo', 'type', 'subtype', { childOf: 'baz' }) trans.startSpan('foo', 'type', 'subtype', 'action', { childOf: 'baz' }) + trans.startSpan('foo', 'type', 'subtype', { exitSpan: true }) function ensureParentId (id: string) {} ensureParentId(trans.ensureParentId()) + trans.setOutcome('failure') + trans.end() trans.end('foo') trans.end('foo', 42) @@ -205,6 +209,8 @@ apm.logger.fatal('') span.addLabels({ s: 'bar', n: 42, b: false }) + span.setOutcome('failure') + span.end() span.end(42) }