Skip to content

Commit

Permalink
fix: drop 'transaction.sync' field, it was never used by apm-server
Browse files Browse the repository at this point in the history
Also add a test for span.sync=false for ES and HTTP spans from
@elastic/elasticsearch instrumentation for #1996.

Move the span.sync test from async-hooks.test.js over to span.test.js
because its impl no longer deps on lib/instrumentation/async-hooks.js.

Fixes: #2292
  • Loading branch information
trentm committed Sep 15, 2021
1 parent 4936dd8 commit 39e3854
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 47 deletions.
1 change: 0 additions & 1 deletion lib/instrumentation/generic-span.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ function GenericSpan (agent, ...args) {

this.timestamp = this._timer.start
this.ended = false
this.sync = true

this.outcome = constants.OUTCOME_UNKNOWN

Expand Down
1 change: 1 addition & 0 deletions lib/instrumentation/span.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ function Span (transaction, name, ...args) {
this._message = null
this._stackObj = null
this._capturedStackTrace = null
this.sync = true
this._startXid = executionAsyncId()

this.transaction = transaction
Expand Down
6 changes: 0 additions & 6 deletions lib/instrumentation/transaction.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use strict'

const { executionAsyncId } = require('async_hooks')
var util = require('util')

var ObjectIdentityMap = require('object-identity-map')
Expand Down Expand Up @@ -36,7 +35,6 @@ function Transaction (agent, name, ...args) {
this._contextLost = false // TODO: Send this up to the server some how
this._abortTime = 0
this._breakdownTimings = new ObjectIdentityMap()
this._startXid = executionAsyncId()

this.outcome = constants.OUTCOME_UNKNOWN
}
Expand Down Expand Up @@ -131,7 +129,6 @@ Transaction.prototype.toJSON = function () {
result: String(this.result),
sampled: this.sampled,
context: undefined,
sync: this.sync,
span_count: {
started: this._builtSpans
},
Expand Down Expand Up @@ -234,9 +231,6 @@ Transaction.prototype.end = function (result, endTime) {
this._timer.end(endTime)
this._captureBreakdown(this)
this.ended = true
if (executionAsyncId() !== this._startXid) {
this.sync = false
}

var trans = this._agent._instrumentation.currentTransaction

Expand Down
24 changes: 0 additions & 24 deletions test/instrumentation/async-hooks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,30 +121,6 @@ test('post-defined, post-resolved promise', function (t) {
})
})

test('sync.sync', function (t) {
var trans = agent.startTransaction()
t.strictEqual(trans.sync, true)

var span1 = agent.startSpan('span1')
t.strictEqual(span1.sync, true)

// This span will be *ended* synchronously. It should stay `span.sync=true`.
var span2 = agent.startSpan('span2')
t.strictEqual(span2.sync, true, 'span2.sync=true immediately after creation')
span2.end()
t.strictEqual(span2.sync, true, 'span2.sync=true immediately after end')

setImmediate(() => {
span1.end()
t.strictEqual(span1.sync, false)
trans.end()
t.strictEqual(trans.sync, false)
t.strictEqual(span2.sync, true,
'span2.sync=true later after having ended sync')
t.end()
})
})

function twice (fn) {
setImmediate(fn)
setImmediate(fn)
Expand Down
2 changes: 2 additions & 0 deletions test/instrumentation/modules/@elastic/elasticsearch.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -586,12 +586,14 @@ function checkDataAndEnd (t, method, path, dbStatement) {
t.strictEqual(esSpan.type, 'db')
t.strictEqual(esSpan.subtype, 'elasticsearch')
t.strictEqual(esSpan.action, 'request')
t.strictEqual(esSpan.sync, false, 'span.sync=false')

const httpSpan = findObjInArray(data.spans, 'subtype', 'http')
t.ok(httpSpan, 'have an http span')
t.strictEqual(httpSpan.type, 'external')
t.strictEqual(httpSpan.subtype, 'http')
t.strictEqual(httpSpan.action, method)
t.strictEqual(httpSpan.sync, false, 'span.sync=false')

t.equal(httpSpan.name, method + ' ' + host, 'http span should have expected name')
t.equal(esSpan.name, 'Elasticsearch: ' + method + ' ' + path, 'elasticsearch span should have expected name')
Expand Down
23 changes: 17 additions & 6 deletions test/instrumentation/span.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,13 +161,24 @@ test('#addLabels', function (t) {
t.end()
})

test('sync/async tracking', function (t) {
var trans = new Transaction(agent)
var span = new Span(trans)
t.strictEqual(span.sync, true)
test('span.sync', function (t) {
var trans = agent.startTransaction()

var span1 = agent.startSpan('span1')
t.strictEqual(span1.sync, true)

// This span will be *ended* synchronously. It should stay `span.sync=true`.
var span2 = agent.startSpan('span2')
t.strictEqual(span2.sync, true, 'span2.sync=true immediately after creation')
span2.end()
t.strictEqual(span2.sync, true, 'span2.sync=true immediately after end')

setImmediate(() => {
span.end()
t.strictEqual(span.sync, false)
span1.end()
t.strictEqual(span1.sync, false, 'span1.sync=false immediately after end')
trans.end()
t.strictEqual(span2.sync, true,
'span2.sync=true later after having ended sync')
t.end()
})
})
Expand Down
10 changes: 0 additions & 10 deletions test/instrumentation/transaction.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -291,16 +291,6 @@ test('parallel transactions', function (t) {
}, 25)
})

test('sync/async tracking', function (t) {
var trans = new Transaction(agent)
t.strictEqual(trans.sync, true)
setImmediate(() => {
trans.end()
t.strictEqual(trans.sync, false)
t.end()
})
})

test('#_encode() - un-ended', function (t) {
var trans = new Transaction(agent)
t.strictEqual(trans._encode(), null, 'cannot encode un-ended transaction')
Expand Down

0 comments on commit 39e3854

Please sign in to comment.