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.

Sign up for GitHub

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

Scope manager and exporter integration tests #1224

Closed
wants to merge 5 commits into from
Closed

Scope manager and exporter integration tests #1224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d720dd3
Scope manager integration tests
bengl Jan 26, 2021
64c0cc5
integration test for log exporter
bengl Jan 26, 2021
54e1050
decide scope based on platform support
bengl Jan 27, 2021
938eba1
move logger-specific testing to actual test file
bengl Jan 27, 2021
212717c
assert that internal context propagation actually happens
bengl Jan 27, 2021
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
6 changes: 6 additions & 0 deletions integration-tests/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const codec = msgpack.createCodec({ int64: true })
const EventEmitter = require('events')
const { fork } = require('child_process')
const http = require('http')
const rl = require('readline')

class FakeAgent extends EventEmitter {
constructor (port = 0) {
Expand Down Expand Up @@ -85,6 +86,11 @@ class FakeAgent extends EventEmitter {

function spawnProc (filename, options = {}) {
const proc = fork(filename, options)
if (proc.stdout) {
rl.createInterface(proc.stdout).on('line', line => {
proc.emit('logLine', line)
})
}
return new Promise((resolve, reject) => {
proc.on('message', ({ port }) => {
proc.url = `http://localhost:${port}`
Expand Down
45 changes: 40 additions & 5 deletions integration-tests/startup.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
const {
FakeAgent,
spawnProc,
curlAndAssertMessage
curlAndAssertMessage,
curl
} = require('./helpers')
const path = require('path')
const { assert } = require('chai')
Expand Down Expand Up @@ -37,13 +38,15 @@ describe('startup', () => {
assert.isArray(payload[0])
assert.strictEqual(payload[0].length, 1)
assert.propertyVal(payload[0][0], 'name', 'http.request')
assert.propertyVal(payload[0][0].meta, 'foo', 'bar')
})
})

it('works for options.url', async () => {
it('works for options.url and options.scope: async_hooks', async () => {
proc = await spawnProc(startupTestFile, {
env: {
AGENT_URL: `http://localhost:${agent.port}`
AGENT_URL: `http://localhost:${agent.port}`,
SCOPE: 'async_hooks'
}
})
return curlAndAssertMessage(agent, proc, ({ headers, payload }) => {
Expand All @@ -53,8 +56,36 @@ describe('startup', () => {
assert.isArray(payload[0])
assert.strictEqual(payload[0].length, 1)
assert.propertyVal(payload[0][0], 'name', 'http.request')
assert.propertyVal(payload[0][0].meta, 'foo', 'bar')
})
})

it('uses log exporter correctly', async () => {
proc = await spawnProc(startupTestFile, {
env: {
AWS_LAMBDA_FUNCTION_NAME: 'fake-lambda'
},
stdio: 'pipe'
})
const logPromise = new Promise((resolve, reject) => {
proc.once('logLine', line => {
try {
const { traces } = JSON.parse(line)
assert.isArray(traces)
assert.strictEqual(traces.length, 1)
assert.isArray(traces[0])
assert.strictEqual(traces[0].length, 1)
assert.propertyVal(traces[0][0], 'name', 'http.request')
assert.propertyVal(traces[0][0].meta, 'foo', 'bar')
resolve()
} catch (e) {
reject(e)
}
})
})
const curlPromise = curl(proc)
return Promise.all([logPromise, curlPromise])
})
})

context('env var', () => {
Expand All @@ -80,13 +111,15 @@ describe('startup', () => {
assert.isArray(payload[0])
assert.strictEqual(payload[0].length, 1)
assert.propertyVal(payload[0][0], 'name', 'http.request')
assert.propertyVal(payload[0][0].meta, 'foo', 'bar')
})
})

it('works for DD_TRACE_AGENT_URL', async () => {
it('works for DD_TRACE_AGENT_URL and DD_TRACE_SCOPE=async_resource', async () => {
proc = await spawnProc(startupTestFile, {
env: {
DD_TRACE_AGENT_URL: `http://localhost:${agent.port}`
DD_TRACE_AGENT_URL: `http://localhost:${agent.port}`,
DD_TRACE_SCOPE: 'async_resource'
}
})
return curlAndAssertMessage(agent, proc, ({ headers, payload }) => {
Expand All @@ -96,6 +129,7 @@ describe('startup', () => {
assert.isArray(payload[0])
assert.strictEqual(payload[0].length, 1)
assert.propertyVal(payload[0][0], 'name', 'http.request')
assert.propertyVal(payload[0][0].meta, 'foo', 'bar')
})
})
})
Expand All @@ -121,6 +155,7 @@ describe('startup', () => {
assert.isArray(payload[0])
assert.strictEqual(payload[0].length, 1)
assert.propertyVal(payload[0][0], 'name', 'http.request')
assert.propertyVal(payload[0][0].meta, 'foo', 'bar')
})
})
})
Expand Down
19 changes: 17 additions & 2 deletions integration-tests/startup/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,27 @@ if (process.env.AGENT_URL) {
options.url = process.env.AGENT_URL
}

require('../..').init(options)
if (process.env.SCOPE) {
options.scope = process.env.SCOPE
}

const tracer = require('../..').init(options)

const http = require('http')

const server = http.createServer((req, res) => {
res.end('hello, world\n')
process.nextTick(() => {
setImmediate(() => {
(async () => {
await new Promise(resolve => {
resolve()
}).then(() => {
tracer.scope().active().setTag('foo', 'bar')
res.end('hello, world\n')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin attaches the span to the request object and doesn't use the scope manager, so this doesn't actually end up testing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would adding a tag to the active span suffice here then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this actually testing? If it's testing the scope manager maybe it should just be used directly.

})
})()
})
})
}).listen(0, () => {
const port = server.address().port
process.send({ port })
Expand Down
5 changes: 3 additions & 2 deletions packages/dd-trace/src/platform/node/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const semver = require('semver')
const emitter = new EventEmitter()

const hasSupportedAsyncLocalStorage = semver.satisfies(process.versions.node, '>=14.5 || ^12.19.0')
const hasSupportedAsyncResource = semver.satisfies(process.versions.node, '>= 14 || ^13.9.0 || ^12.19.0')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these be the exact same versions as AsyncLocalStorage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because executionAsyncResource was added before AsyncLocalStorage was.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not really about when it was added, but more about when the bugs affecting us were fixed, which was in nodejs/node#33801 which affected both AsyncLocalStorage and executionAsyncResource and landed in 14.5.0.


const platform = {
_config: {},
Expand All @@ -42,9 +43,9 @@ const platform = {
off: emitter.removeListener.bind(emitter),
Loader,
getScope (scope) {
if (scope === scopes.ASYNC_RESOURCE) {
if (hasSupportedAsyncResource && scope === scopes.ASYNC_RESOURCE) {
return require('../../scope/async_resource')
} else if (scope === scopes.ASYNC_LOCAL_STORAGE || (!scope && hasSupportedAsyncLocalStorage)) {
} else if (hasSupportedAsyncLocalStorage && (scope === scopes.ASYNC_LOCAL_STORAGE || !scope)) {
return require('../../scope/async_local_storage')
} else {
return require('../../scope/async_hooks')
Expand Down