From 8233b9eba69c5bc3943747125f5b6c674cdf2cbe Mon Sep 17 00:00:00 2001 From: rochdev Date: Mon, 15 Oct 2018 11:39:26 -0400 Subject: [PATCH 01/30] v0.7.0 --- lib/version.js | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/version.js b/lib/version.js index 20c27058de0..a036199d98d 100644 --- a/lib/version.js +++ b/lib/version.js @@ -1 +1 @@ -module.exports = '0.6.0' +module.exports = '0.7.0' diff --git a/package.json b/package.json index c7c225fea10..f79c4d81eca 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "dd-trace", - "version": "0.6.0", + "version": "0.7.0", "description": "Datadog APM tracing client for JavaScript", "main": "index.js", "scripts": { From f890c34e648e581d7ee26dcffea2ca8b48b8b13e Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Mon, 15 Oct 2018 16:00:39 -0400 Subject: [PATCH 02/30] add support for redis >=0.12 <2.6 (#314) --- src/plugins/redis.js | 105 +++++++++++++++++++++++++------------ test/plugins/redis.spec.js | 3 +- 2 files changed, 73 insertions(+), 35 deletions(-) diff --git a/src/plugins/redis.js b/src/plugins/redis.js index 9c839ff84fc..df8a4039784 100644 --- a/src/plugins/redis.js +++ b/src/plugins/redis.js @@ -5,25 +5,7 @@ const Tags = require('opentracing').Tags function createWrapInternalSendCommand (tracer, config) { return function wrapInternalSendCommand (internalSendCommand) { return function internalSendCommandWithTrace (options) { - const scope = tracer.scopeManager().active() - const span = tracer.startSpan('redis.command', { - childOf: scope && scope.span(), - tags: { - [Tags.SPAN_KIND]: Tags.SPAN_KIND_RPC_CLIENT, - [Tags.DB_TYPE]: 'redis', - 'service.name': config.service || `${tracer._service}-redis`, - 'resource.name': options.command, - 'span.type': 'redis', - 'db.name': this.selected_db || '0' - } - }) - - if (this.connection_options) { - span.addTags({ - 'out.host': String(this.connection_options.host), - 'out.port': String(this.connection_options.port) - }) - } + const span = startSpan(tracer, config, this, options.command) options.callback = wrapCallback(tracer, span, options.callback) @@ -32,6 +14,53 @@ function createWrapInternalSendCommand (tracer, config) { } } +function createWrapSendCommand (tracer, config) { + return function wrapSendCommand (sendCommand) { + return function sendCommandWithTrace (command, args, callback) { + const span = startSpan(tracer, config, this, command) + + if (callback) { + callback = wrapCallback(tracer, span, callback) + } else if (args) { + args[(args.length || 1) - 1] = wrapCallback(tracer, span, args[args.length - 1]) + } else { + args = [wrapCallback(tracer, span)] + } + + return sendCommand.call(this, command, args, callback) + } + } +} + +function startSpan (tracer, config, client, command) { + const scope = tracer.scopeManager().active() + const span = tracer.startSpan('redis.command', { + childOf: scope && scope.span(), + tags: { + [Tags.SPAN_KIND]: Tags.SPAN_KIND_RPC_CLIENT, + [Tags.DB_TYPE]: 'redis', + 'service.name': config.service || `${tracer._service}-redis`, + 'resource.name': command, + 'span.type': 'redis', + 'db.name': client.selected_db || '0' + } + }) + + const connectionOptions = client.connection_options || client.connection_option || { + host: client.options.host || '127.0.0.1', + port: client.options.port || 6379 + } + + if (connectionOptions) { + span.addTags({ + 'out.host': String(connectionOptions.host), + 'out.port': String(connectionOptions.port) + }) + } + + return span +} + function wrapCallback (tracer, span, done) { return (err, res) => { if (err) { @@ -44,23 +73,31 @@ function wrapCallback (tracer, span, done) { span.finish() - if (done) { + if (typeof done === 'function') { done(err, res) } } } -function patch (redis, tracer, config) { - this.wrap(redis.RedisClient.prototype, 'internal_send_command', createWrapInternalSendCommand(tracer, config)) -} - -function unpatch (redis) { - this.unwrap(redis.RedisClient.prototype, 'internal_send_command') -} - -module.exports = { - name: 'redis', - versions: ['^2.6'], - patch, - unpatch -} +module.exports = [ + { + name: 'redis', + versions: ['^2.6'], + patch (redis, tracer, config) { + this.wrap(redis.RedisClient.prototype, 'internal_send_command', createWrapInternalSendCommand(tracer, config)) + }, + unpatch (redis) { + this.unwrap(redis.RedisClient.prototype, 'internal_send_command') + } + }, + { + name: 'redis', + versions: ['>=0.12 <2.6'], + patch (redis, tracer, config) { + this.wrap(redis.RedisClient.prototype, 'send_command', createWrapSendCommand(tracer, config)) + }, + unpatch (redis) { + this.unwrap(redis.RedisClient.prototype, 'send_command') + } + } +] diff --git a/test/plugins/redis.spec.js b/test/plugins/redis.spec.js index f6401a692d2..da20c3b6fc1 100644 --- a/test/plugins/redis.spec.js +++ b/test/plugins/redis.spec.js @@ -37,7 +37,6 @@ describe('Plugin', () => { it('should do automatic instrumentation when using callbacks', done => { client.on('error', done) - agent.use(() => client.get('foo')) // wait for initial info command agent .use(traces => { expect(traces[0][0]).to.have.property('name', 'redis.command') @@ -52,6 +51,8 @@ describe('Plugin', () => { }) .then(done) .catch(done) + + client.get('foo', () => {}) }) it('should run the callback in the parent context', done => { From 47c945c95c899020d15bb2a1ca0624134380e977 Mon Sep 17 00:00:00 2001 From: Tim Rogers Date: Tue, 23 Oct 2018 15:43:44 +0100 Subject: [PATCH 03/30] Add `validateStatus` option to HTTP plugin (#301) The [OpenTracing spec][1] states that a span should be tagged as an error "if and only if the application considers the operation represented by the Span to have failed". The `dd-trace-js` HTTP plugin allows you to automatically instrument outgoing HTTP requests. Currently, it only tags a span as an error when the request emits an `error` event, which only happens in the case of "hard failures" like a timeout or connection problem. https://github.com/DataDog/dd-trace-js/blob/34c168555f44a961a61ede95a39341d7c527701a/src/plugins/http.js#L59-L67 This means that requests that return responses never are considered errors, even if a 4XX or 5XX response is received. Having spans tagged as errors is useful, because it means they are flagged as such in the Datadog APM interface, useful for reporting. This commit adds support for a new `validateStatus` option for the HTTP plugin which allows the user to supply a `validateStatus` function which, when a response is received, is passed the response status and expected to return `false` if the response should be considered an error, or `true` if not. If it returns `false`, the span will be marked as an error. By default, we will consider requests that return `4XX` responses to be errors, since a `4XX` indicates a failure which is within the user's control. Fixes #297. [1]: https://github.com/opentracing/specification/blob/master/semantic_conventions.md --- docs/API.md | 5 ++- src/plugins/http.js | 23 ++++++++++ test/plugins/http.spec.js | 93 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 118 insertions(+), 3 deletions(-) diff --git a/docs/API.md b/docs/API.md index 1568eda0f7b..6c853df9227 100644 --- a/docs/API.md +++ b/docs/API.md @@ -214,8 +214,9 @@ query HelloWorld { | Option | Default | Description | |------------------|------------------|----------------------------------------| -| service | http-client | The service name for this integration. | -| splitByDomain | false | Use the remote endpoint host as the service name instead of the default. | +| service | http-client | The service name for this integration. | +| splitByDomain | false | Use the remote endpoint host as the service name instead of the default. | +| validateStatus | `code => code < 400 || code >= 500` | Callback function to determine if an HTTP response should be recorded as an error. It should take a status code as its only parameter and return `true` for success or `false` for errors.

ioredis

diff --git a/src/plugins/http.js b/src/plugins/http.js index 0558d7d53e9..5f092010e09 100644 --- a/src/plugins/http.js +++ b/src/plugins/http.js @@ -3,11 +3,13 @@ const url = require('url') const opentracing = require('opentracing') const semver = require('semver') +const log = require('../log') const Tags = opentracing.Tags const FORMAT_HTTP_HEADERS = opentracing.FORMAT_HTTP_HEADERS function patch (http, methodName, tracer, config) { + config = normalizeConfig(config) this.wrap(http, methodName, fn => makeRequestTrace(fn)) function makeRequestTrace (request) { @@ -53,6 +55,10 @@ function patch (http, methodName, tracer, config) { req.on('response', res => { span.setTag(Tags.HTTP_STATUS_CODE, res.statusCode) + if (!config.validateStatus(res.statusCode)) { + span.setTag('error', 1) + } + res.on('end', () => span.finish()) }) @@ -148,6 +154,23 @@ function unpatch (http) { this.unwrap(http, 'get') } +function getStatusValidator (config) { + if (typeof config.validateStatus === 'function') { + return config.validateStatus + } else if (config.hasOwnProperty('validateStatus')) { + log.error('Expected `validateStatus` to be a function.') + } + return code => code < 400 || code >= 500 +} + +function normalizeConfig (config) { + const validateStatus = getStatusValidator(config) + + return Object.assign({}, config, { + validateStatus + }) +} + module.exports = [ { name: 'http', diff --git a/test/plugins/http.spec.js b/test/plugins/http.spec.js index 33d83a6b0fb..052c79663f5 100644 --- a/test/plugins/http.spec.js +++ b/test/plugins/http.spec.js @@ -454,7 +454,7 @@ describe('Plugin', () => { }) }) - it('should handle errors', done => { + it('should handle connection errors', done => { getPort().then(port => { agent .use(traces => { @@ -471,6 +471,56 @@ describe('Plugin', () => { }) }) + it('should not record HTTP 5XX responses as errors by default', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(500).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('error', 0) + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + const req = http.request(`${protocol}://localhost:${port}/user`, res => { + res.on('data', () => { }) + }) + + req.end() + }) + }) + }) + + it('should record HTTP 4XX responses as errors by default', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(400).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('error', 1) + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + const req = http.request(`${protocol}://localhost:${port}/user`, res => { + res.on('data', () => { }) + }) + + req.end() + }) + }) + }) + it('should only record a request once', done => { // Make sure both plugins are loaded, which could cause double-counting. require('http') @@ -555,6 +605,47 @@ describe('Plugin', () => { }) }) + describe('with validateStatus configuration', () => { + let config + + beforeEach(() => { + config = { + validateStatus: status => status < 500 + } + + return agent.load(plugin, 'http', config) + .then(() => { + http = require(protocol) + express = require('express') + }) + }) + + it('should use the supplied function to decide if a response is an error', done => { + const app = express() + + app.get('/user', (req, res) => { + res.status(500).send() + }) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('error', 1) + }) + .then(done) + .catch(done) + + appListener = server(app, port, () => { + const req = http.request(`${protocol}://localhost:${port}/user`, res => { + res.on('data', () => { }) + }) + + req.end() + }) + }) + }) + }) + describe('with splitByDomain configuration', () => { let config From f9bea14a62fae0e2eb3ba0b8477681fdc053d382 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Tue, 23 Oct 2018 12:02:17 -0400 Subject: [PATCH 04/30] add support for graphql 0.10, 0.11, 0.12 and 14 (#326) --- src/plugins/graphql.js | 6 +++--- test/plugins/graphql.spec.js | 11 +---------- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/src/plugins/graphql.js b/src/plugins/graphql.js index 2226a0c7d37..0e72c2070a1 100644 --- a/src/plugins/graphql.js +++ b/src/plugins/graphql.js @@ -378,7 +378,7 @@ module.exports = [ { name: 'graphql', file: 'execution/execute.js', - versions: ['0.13.x'], + versions: ['>=0.10 <=14'], patch (execute, tracer, config) { this.wrap(execute, 'execute', createWrapExecute( tracer, @@ -394,7 +394,7 @@ module.exports = [ { name: 'graphql', file: 'language/parser.js', - versions: ['0.13.x'], + versions: ['>=0.10 <=14'], patch (parser, tracer, config) { this.wrap(parser, 'parse', createWrapParse(tracer, validateConfig(config))) }, @@ -405,7 +405,7 @@ module.exports = [ { name: 'graphql', file: 'validation/validate.js', - versions: ['0.13.x'], + versions: ['>=0.10 <=14'], patch (validate, tracer, config) { this.wrap(validate, 'validate', createWrapValidate(tracer, validateConfig(config))) }, diff --git a/test/plugins/graphql.spec.js b/test/plugins/graphql.spec.js index 90284a96dad..cd0c03d4ee7 100644 --- a/test/plugins/graphql.spec.js +++ b/test/plugins/graphql.spec.js @@ -649,15 +649,6 @@ describe('Plugin', () => { }) it('should handle executor exceptions', done => { - schema = new graphql.GraphQLSchema({ - query: new graphql.GraphQLObjectType({ - name: 'RootQueryType', - fields: { - hello: {} - } - }) - }) - const source = `{ hello }` const document = graphql.parse(source) @@ -679,7 +670,7 @@ describe('Plugin', () => { .catch(done) try { - graphql.execute(schema, document) + graphql.execute({}, document) } catch (e) { error = e } From 8a27aadd997959ff1a093dd9d0434bef3ba7c833 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Tue, 23 Oct 2018 12:03:47 -0400 Subject: [PATCH 05/30] add collapse option to graphql integration (#316) --- docs/API.md | 1 + src/plugins/graphql.js | 54 +++++++++++++++++++++++++++------- test/plugins/graphql.spec.js | 56 ++++++++++++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 10 deletions(-) diff --git a/docs/API.md b/docs/API.md index 6c853df9227..3b71d41b95a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -180,6 +180,7 @@ query HelloWorld { | service | *Service name of the app suffixed with -graphql* | The service name for this integration. | | variables | `undefined` | A callback to enable recording of variables. By default, no variables are recorded. For example, using `variables => variables` would record all variables. | | depth | -1 | The maximum depth of fields/resolvers to instrument. Set to `0` to only instrument the operation or to -1 to instrument all fields/resolvers. | +| collapse | false | Whether to collapse list items into a single element. (i.e. single `users.*.name` span instead of `users.0.name`, `users.1.name`, etc) |

hapi

diff --git a/src/plugins/graphql.js b/src/plugins/graphql.js index 0e72c2070a1..8ac39a990cd 100644 --- a/src/plugins/graphql.js +++ b/src/plugins/graphql.js @@ -97,14 +97,17 @@ function wrapFields (type, tracer, config, responsePathAsArray) { function wrapResolve (resolve, tracer, config, responsePathAsArray) { if (resolve._datadog_patched) return resolve + if (config.collapse) { + responsePathAsArray = withCollapse(responsePathAsArray) + } function resolveWithTrace (source, args, contextValue, info) { const operation = info.operation const path = responsePathAsArray(info.path) const depth = path.filter(item => typeof item === 'string').length - const fieldParent = getFieldParent(operation, path) if (config.depth >= 0 && config.depth < depth) { + const fieldParent = getFieldParent(operation, path) const scope = tracer.scopeManager().activate(fieldParent) return call(resolve, this, arguments, () => { @@ -113,15 +116,8 @@ function wrapResolve (resolve, tracer, config, responsePathAsArray) { }) } - const childOf = createPathSpan(tracer, config, 'field', fieldParent, path) - - operation._datadog_fields[path.join('.')] = { - span: childOf, - parent: fieldParent - } - - const span = createPathSpan(tracer, config, 'resolve', childOf, path) - const scope = tracer.scopeManager().activate(span) + const field = assertField(tracer, config, operation, path) + const scope = tracer.scopeManager().activate(field.resolveSpan) return call(resolve, this, arguments, err => finish(scope, operation, path, err)) } @@ -166,6 +162,29 @@ function call (fn, thisContext, args, callback) { } } +function assertField (tracer, config, operation, path) { + let field = getField(operation, path) + + if (!field) { + field = operation._datadog_fields[path.join('.')] = { + pending: 0, + error: null + } + + const fieldParent = getFieldParent(operation, path) + const childOf = createPathSpan(tracer, config, 'field', fieldParent, path) + const span = createPathSpan(tracer, config, 'resolve', childOf, path) + + field.parent = fieldParent + field.span = childOf + field.resolveSpan = span + } + + field.pending++ + + return field +} + function getFieldParent (operation, path) { for (let i = path.length - 1; i > 0; i--) { const field = getField(operation, path.slice(0, i)) @@ -287,6 +306,14 @@ function createPathSpan (tracer, config, name, childOf, path) { } function finish (scope, operation, path, error) { + const field = getField(operation, path) + + field.pending-- + + if (field.error || field.pending > 0) return + + field.error = error + const span = scope.span() addError(span, error) @@ -318,6 +345,13 @@ function finishOperation (operation, error) { operation._datadog_span.finish() } +function withCollapse (responsePathAsArray) { + return function () { + return responsePathAsArray.apply(this, arguments) + .map(segment => typeof segment === 'number' ? '*' : segment) + } +} + function getField (operation, path) { return operation._datadog_fields[path.join('.')] } diff --git a/test/plugins/graphql.spec.js b/test/plugins/graphql.spec.js index cd0c03d4ee7..730e0e7ebe5 100644 --- a/test/plugins/graphql.spec.js +++ b/test/plugins/graphql.spec.js @@ -947,6 +947,62 @@ describe('Plugin', () => { }) }) + describe('with collapsing enabled', () => { + before(() => { + tracer = require('../..') + + return agent.load(plugin, 'graphql', { collapse: true }) + }) + + after(() => { + return agent.close() + }) + + beforeEach(() => { + graphql = require(`../../versions/graphql@${version}`).get() + buildSchema() + }) + + it('should collapse list field resolvers', done => { + const source = `{ friends { name } }` + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans).to.have.length(8) + + const execute = spans[3] + const friendsField = spans[4] + const friendsResolve = spans[5] + const friendNameField = spans[6] + const friendNameResolve = spans[7] + + expect(execute).to.have.property('name', 'graphql.execute') + + expect(friendsField).to.have.property('name', 'graphql.field') + expect(friendsField).to.have.property('resource', 'friends') + expect(friendsField.parent_id.toString()).to.equal(execute.span_id.toString()) + + expect(friendsResolve).to.have.property('name', 'graphql.resolve') + expect(friendsResolve).to.have.property('resource', 'friends') + expect(friendsResolve.parent_id.toString()).to.equal(friendsField.span_id.toString()) + + expect(friendNameField).to.have.property('name', 'graphql.field') + expect(friendNameField).to.have.property('resource', 'friends.*.name') + expect(friendNameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) + + expect(friendNameResolve).to.have.property('name', 'graphql.resolve') + expect(friendNameResolve).to.have.property('resource', 'friends.*.name') + expect(friendNameResolve.parent_id.toString()).to.equal(friendNameField.span_id.toString()) + }) + .then(done) + .catch(done) + + graphql.graphql(schema, source).catch(done) + }) + }) + withVersions(plugin, 'apollo-server-core', apolloVersion => { let runQuery let mergeSchemas From deec44443860c10ea5bbeff14de2bc35299a87fe Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 24 Oct 2018 11:31:21 -0400 Subject: [PATCH 06/30] add support for restify >=3 <=6 (#321) --- src/instrumenter.js | 36 ++++++++++++------- src/plugins/restify.js | 69 +++++++++++++++++++++--------------- test/instrumenter.spec.js | 8 ++--- test/plugins/agent.js | 2 +- test/plugins/restify.spec.js | 4 +++ 5 files changed, 73 insertions(+), 46 deletions(-) diff --git a/src/instrumenter.js b/src/instrumenter.js index 2d04986e611..55b4c814d26 100644 --- a/src/instrumenter.js +++ b/src/instrumenter.js @@ -11,7 +11,7 @@ shimmer({ logger: () => {} }) class Instrumenter { constructor (tracer) { this._tracer = tracer - this._names = [] + this._names = new Set() this._plugins = new Map() this._instrumented = new Map() } @@ -58,33 +58,45 @@ class Instrumenter { const instrumentedModules = uniq(instrumentations .map(instrumentation => instrumentation.name)) - this._names = instrumentations - .map(instrumentation => filename(instrumentation)) + this._names = new Set(instrumentations + .map(instrumentation => filename(instrumentation))) hook(instrumentedModules, { internals: true }, this.hookModule.bind(this)) } - wrap (nodule, name) { - if (typeof nodule[name] !== 'function') { - throw new Error(`Expected object ${nodule} to contain method ${name}.`) - } + wrap (nodules, names, wrapper) { + nodules = [].concat(nodules) + names = [].concat(names) + + nodules.forEach(nodule => { + names.forEach(name => { + if (typeof nodule[name] !== 'function') { + throw new Error(`Expected object ${nodule} to contain method ${name}.`) + } + }) + }) - return shimmer.wrap.apply(this, arguments) + return shimmer.massWrap.call(this, nodules, names, wrapper) } - unwrap () { - return shimmer.unwrap.apply(this, arguments) + unwrap (nodules, names, wrapper) { + nodules = [].concat(nodules) + names = [].concat(names) + + return shimmer.massUnwrap.call(this, nodules, names, wrapper) } hookModule (moduleExports, moduleName, moduleBaseDir) { - if (this._names.indexOf(moduleName) === -1) { + if (!this._names.has(moduleName)) { return moduleExports } const moduleVersion = getVersion(moduleBaseDir) Array.from(this._plugins.keys()) - .filter(plugin => [].concat(plugin).some(instrumentation => filename(instrumentation) === moduleName)) + .filter(plugin => [].concat(plugin).some(instrumentation => + filename(instrumentation) === moduleName && matchVersion(moduleVersion, instrumentation.versions) + )) .forEach(plugin => this._validate(plugin, moduleBaseDir)) this._plugins diff --git a/src/plugins/restify.js b/src/plugins/restify.js index b56d591216f..f45e4d500a3 100644 --- a/src/plugins/restify.js +++ b/src/plugins/restify.js @@ -1,58 +1,69 @@ 'use strict' const web = require('./util/web') +const handlers = ['use', 'pre'] +const methods = ['del', 'get', 'head', 'opts', 'post', 'put', 'patch'] -function createWrapOnRequest (tracer, config) { +function createWrapSetupRequest (tracer, config) { config = web.normalizeConfig(config) - return function wrapOnRequest (onRequest) { - return function onRequestWithTrace (req, res) { + return function wrapSetupRequest (setupRequest) { + return function setupRequestWithTrace (req, res) { web.instrument(tracer, config, req, res, 'restify.request') web.beforeEnd(req, () => { - const route = req.getRoute() - - if (route) { - web.enterRoute(req, route.path) + if (req.route) { + web.enterRoute(req, req.route.path) } }) - return onRequest.apply(this, arguments) + return setupRequest.apply(this, arguments) } } } -function createWrapAdd (tracer, config) { - return function wrapAdd (add) { - return function addWithTrace (handler) { - return add.call(this, function (req, res, next) { - web.reactivate(req) - handler.apply(this, arguments) - }) +function createWrapMethod (tracer, config) { + return function wrapMethod (method) { + return function methodWithTrace (path) { + const middleware = wrapMiddleware(Array.prototype.slice.call(arguments, 1)) + + return method.apply(this, [path].concat(middleware)) + } + } +} + +function createWrapHandler (tracer, config) { + return function wrapMethod (method) { + return function methodWithTrace () { + return method.apply(this, wrapMiddleware(arguments)) } } } +function wrapMiddleware (middleware) { + return Array.prototype.map.call(middleware, wrapFn) +} + +function wrapFn (fn) { + return function (req, res, next) { + web.reactivate(req) + return fn.apply(this, arguments) + } +} + module.exports = [ { name: 'restify', - versions: ['7.x'], + versions: ['>=3 <=7'], file: 'lib/server.js', patch (Server, tracer, config) { - this.wrap(Server.prototype, '_onRequest', createWrapOnRequest(tracer, config)) + this.wrap(Server.prototype, '_setupRequest', createWrapSetupRequest(tracer, config)) + this.wrap(Server.prototype, handlers, createWrapHandler(tracer, config)) + this.wrap(Server.prototype, methods, createWrapMethod(tracer, config)) }, unpatch (Server) { - this.unwrap(Server.prototype, '_onRequest') - } - }, - { - name: 'restify', - versions: ['7.x'], - file: 'lib/chain.js', - patch (Chain, tracer, config) { - this.wrap(Chain.prototype, 'add', createWrapAdd(tracer, config)) - }, - unpatch (Chain) { - this.unwrap(Chain.prototype, 'add') + this.unwrap(Server.prototype, '_setupRequest') + this.unwrap(Server.prototype, handlers) + this.unwrap(Server.prototype, methods) } } ] diff --git a/test/instrumenter.spec.js b/test/instrumenter.spec.js index 3e647b13990..4cd0f6ab6a8 100644 --- a/test/instrumenter.spec.js +++ b/test/instrumenter.spec.js @@ -45,8 +45,8 @@ describe('Instrumenter', () => { } shimmer = sinon.spy() - shimmer.wrap = sinon.spy() - shimmer.unwrap = sinon.spy() + shimmer.massWrap = sinon.spy() + shimmer.massUnwrap = sinon.spy() Instrumenter = proxyquire('../src/instrumenter', { 'shimmer': shimmer, @@ -217,7 +217,7 @@ describe('Instrumenter', () => { instrumenter.wrap(obj, 'method', wrapper) - expect(shimmer.wrap).to.have.been.calledWith(obj, 'method', wrapper) + expect(shimmer.massWrap).to.have.been.calledWith([obj], ['method'], wrapper) }) it('should throw if the method does not exist', () => { @@ -234,7 +234,7 @@ describe('Instrumenter', () => { instrumenter.unwrap(obj, 'method') - expect(shimmer.unwrap).to.have.been.calledWith(obj, 'method') + expect(shimmer.massUnwrap).to.have.been.calledWith([obj], ['method']) }) }) }) diff --git a/test/plugins/agent.js b/test/plugins/agent.js index abfe87488c9..82283d78521 100644 --- a/test/plugins/agent.js +++ b/test/plugins/agent.js @@ -27,7 +27,7 @@ module.exports = { }) agent.put('/v0.4/traces', (req, res) => { - res.status(200).send('OK') + res.status(200).send({ rate_by_service: { 'service:,env:': 1 } }) handlers.forEach(handler => handler(req.body)) }) diff --git a/test/plugins/restify.spec.js b/test/plugins/restify.spec.js index 6cf9e850ee5..d1e1f885b09 100644 --- a/test/plugins/restify.spec.js +++ b/test/plugins/restify.spec.js @@ -2,6 +2,7 @@ const axios = require('axios') const getPort = require('get-port') +const semver = require('semver') const agent = require('./agent') const plugin = require('../../src/plugins/restify') @@ -14,6 +15,9 @@ describe('Plugin', () => { describe('restify', () => { withVersions(plugin, 'restify', version => { + // Some internal code of older versions is not compatible with Node >6 + if (semver.intersects(version, '<5') && semver.intersects(process.version, '>6')) return + beforeEach(() => { tracer = require('../..') restify = require(`../../versions/restify@${version}`).get() From 2c2b7fb7a39e7caaf85fd7e7d6561147372c4d23 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 24 Oct 2018 11:31:52 -0400 Subject: [PATCH 07/30] add support for mongodb-core 2.x (#327) --- src/plugins/mongodb-core.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/mongodb-core.js b/src/plugins/mongodb-core.js index 3c5fec92478..75acb96d0c8 100644 --- a/src/plugins/mongodb-core.js +++ b/src/plugins/mongodb-core.js @@ -298,7 +298,7 @@ function isObject (val) { module.exports = [ { name: 'mongodb-core', - versions: ['3.x'], + versions: ['>=2 <=3'], patch (mongo, tracer, config) { this.wrap(mongo.Server.prototype, 'command', createWrapOperation(tracer, config)) this.wrap(mongo.Server.prototype, 'insert', createWrapOperation(tracer, config, 'insert')) From 74643019b89b24a4ab3c569747353a9427bc4677 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 24 Oct 2018 11:32:43 -0400 Subject: [PATCH 08/30] add support for pg 4.x and 5.x (#328) --- src/plugins/pg.js | 2 +- test/plugins/pg.spec.js | 33 ++++++++++++++++++--------------- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/plugins/pg.js b/src/plugins/pg.js index 4e209253e91..09478622e13 100644 --- a/src/plugins/pg.js +++ b/src/plugins/pg.js @@ -73,7 +73,7 @@ function unpatch (pg) { module.exports = { name: 'pg', - versions: ['6.x', '7.x'], + versions: ['>=4 <=7'], patch, unpatch } diff --git a/test/plugins/pg.spec.js b/test/plugins/pg.spec.js index 26f4dd64422..a6a63eb09f4 100644 --- a/test/plugins/pg.spec.js +++ b/test/plugins/pg.spec.js @@ -1,5 +1,6 @@ 'use strict' +const semver = require('semver') const agent = require('./agent') const plugin = require('../../src/plugins/pg') @@ -60,23 +61,25 @@ describe('Plugin', () => { }) }) - it('should do automatic instrumentation when using promises', done => { - agent.use(traces => { - expect(traces[0][0]).to.have.property('service', 'test-postgres') - expect(traces[0][0]).to.have.property('resource', 'SELECT $1::text as message') - expect(traces[0][0]).to.have.property('type', 'sql') - expect(traces[0][0].meta).to.have.property('db.name', 'postgres') - expect(traces[0][0].meta).to.have.property('db.user', 'postgres') - expect(traces[0][0].meta).to.have.property('db.type', 'postgres') - expect(traces[0][0].meta).to.have.property('span.kind', 'client') + if (semver.intersects(version, '>=5.1')) { // initial promise support + it('should do automatic instrumentation when using promises', done => { + agent.use(traces => { + expect(traces[0][0]).to.have.property('service', 'test-postgres') + expect(traces[0][0]).to.have.property('resource', 'SELECT $1::text as message') + expect(traces[0][0]).to.have.property('type', 'sql') + expect(traces[0][0].meta).to.have.property('db.name', 'postgres') + expect(traces[0][0].meta).to.have.property('db.user', 'postgres') + expect(traces[0][0].meta).to.have.property('db.type', 'postgres') + expect(traces[0][0].meta).to.have.property('span.kind', 'client') - done() - }) + done() + }) - client.query('SELECT $1::text as message', ['Hello world!']) - .then(() => client.end()) - .catch(done) - }) + client.query('SELECT $1::text as message', ['Hello world!']) + .then(() => client.end()) + .catch(done) + }) + } it('should handle errors', done => { agent.use(traces => { From 5d983af581dc60a0db008a0cb626080ae033d9b9 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 24 Oct 2018 11:33:16 -0400 Subject: [PATCH 09/30] add support for elasticsearch >=10 <=14 (#331) --- src/plugins/elasticsearch.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/elasticsearch.js b/src/plugins/elasticsearch.js index 04e877030d6..3a5607bf4e0 100644 --- a/src/plugins/elasticsearch.js +++ b/src/plugins/elasticsearch.js @@ -75,7 +75,7 @@ module.exports = [ { name: 'elasticsearch', file: 'src/lib/transport.js', - versions: ['15.x'], + versions: ['>=10 <=15'], patch (Transport, tracer, config) { this.wrap(Transport.prototype, 'request', createWrapRequest(tracer, config)) }, From 4f180d5cf4016a62ca0f8716c2178b1b06e8f6a0 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 24 Oct 2018 11:33:34 -0400 Subject: [PATCH 10/30] add support for mysql2 1.x (#332) --- src/plugins/mysql2.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/mysql2.js b/src/plugins/mysql2.js index faf917569cc..278ad85aeee 100644 --- a/src/plugins/mysql2.js +++ b/src/plugins/mysql2.js @@ -73,7 +73,7 @@ module.exports = [ { name: 'mysql2', file: 'lib/connection.js', - versions: ['^1.5'], + versions: ['1.x'], patch: patchConnection, unpatch: unpatchConnection } From 3f68b4f3ede33ea593ca471ba40475bfbed56dfa Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 24 Oct 2018 15:49:26 -0400 Subject: [PATCH 11/30] enable collapse option by default in graphql plugin (#329) --- docs/API.md | 2 +- src/plugins/graphql.js | 3 +- test/plugins/graphql.spec.js | 64 ++++++++++++++++++------------------ 3 files changed, 35 insertions(+), 34 deletions(-) diff --git a/docs/API.md b/docs/API.md index 3b71d41b95a..b6305adbef7 100644 --- a/docs/API.md +++ b/docs/API.md @@ -180,7 +180,7 @@ query HelloWorld { | service | *Service name of the app suffixed with -graphql* | The service name for this integration. | | variables | `undefined` | A callback to enable recording of variables. By default, no variables are recorded. For example, using `variables => variables` would record all variables. | | depth | -1 | The maximum depth of fields/resolvers to instrument. Set to `0` to only instrument the operation or to -1 to instrument all fields/resolvers. | -| collapse | false | Whether to collapse list items into a single element. (i.e. single `users.*.name` span instead of `users.0.name`, `users.1.name`, etc) | +| collapse | true | Whether to collapse list items into a single element. (i.e. single `users.*.name` span instead of `users.0.name`, `users.1.name`, etc) |

hapi

diff --git a/src/plugins/graphql.js b/src/plugins/graphql.js index 8ac39a990cd..7c0da1bfa9a 100644 --- a/src/plugins/graphql.js +++ b/src/plugins/graphql.js @@ -386,7 +386,8 @@ function addError (span, error) { function validateConfig (config) { return Object.assign({}, config, { depth: getDepth(config), - variables: getVariablesFilter(config) + variables: getVariablesFilter(config), + collapse: config.collapse === undefined || !!config.collapse }) } diff --git a/test/plugins/graphql.spec.js b/test/plugins/graphql.spec.js index 730e0e7ebe5..2150e7a2c81 100644 --- a/test/plugins/graphql.spec.js +++ b/test/plugins/graphql.spec.js @@ -391,15 +391,13 @@ describe('Plugin', () => { .use(traces => { const spans = sort(traces[0]) - expect(spans).to.have.length(10) + expect(spans).to.have.length(8) const execute = spans[3] const friendsField = spans[4] const friendsResolve = spans[5] - const friend0NameField = spans[6] - const friend0NameResolve = spans[7] - const friend1NameField = spans[8] - const friend1NameResolve = spans[9] + const friendNameField = spans[6] + const friendNameResolve = spans[7] expect(execute).to.have.property('name', 'graphql.execute') @@ -411,21 +409,13 @@ describe('Plugin', () => { expect(friendsResolve).to.have.property('resource', 'friends') expect(friendsResolve.parent_id.toString()).to.equal(friendsField.span_id.toString()) - expect(friend0NameField).to.have.property('name', 'graphql.field') - expect(friend0NameField).to.have.property('resource', 'friends.0.name') - expect(friend0NameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) - - expect(friend0NameResolve).to.have.property('name', 'graphql.resolve') - expect(friend0NameResolve).to.have.property('resource', 'friends.0.name') - expect(friend0NameResolve.parent_id.toString()).to.equal(friend0NameField.span_id.toString()) - - expect(friend1NameField).to.have.property('name', 'graphql.field') - expect(friend1NameField).to.have.property('resource', 'friends.1.name') - expect(friend1NameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) + expect(friendNameField).to.have.property('name', 'graphql.field') + expect(friendNameField).to.have.property('resource', 'friends.*.name') + expect(friendNameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) - expect(friend1NameResolve).to.have.property('name', 'graphql.resolve') - expect(friend1NameResolve).to.have.property('resource', 'friends.1.name') - expect(friend1NameResolve.parent_id.toString()).to.equal(friend1NameField.span_id.toString()) + expect(friendNameResolve).to.have.property('name', 'graphql.resolve') + expect(friendNameResolve).to.have.property('resource', 'friends.*.name') + expect(friendNameResolve.parent_id.toString()).to.equal(friendNameField.span_id.toString()) }) .then(done) .catch(done) @@ -937,7 +927,7 @@ describe('Plugin', () => { ].indexOf(span.resource) !== -1 }) - expect(spans).to.have.length(16) + expect(spans).to.have.length(12) expect(ignored).to.have.length(0) }) .then(done) @@ -947,11 +937,11 @@ describe('Plugin', () => { }) }) - describe('with collapsing enabled', () => { + describe('with collapsing disabled', () => { before(() => { tracer = require('../..') - return agent.load(plugin, 'graphql', { collapse: true }) + return agent.load(plugin, 'graphql', { collapse: false }) }) after(() => { @@ -963,20 +953,22 @@ describe('Plugin', () => { buildSchema() }) - it('should collapse list field resolvers', done => { + it('should not collapse list field resolvers', done => { const source = `{ friends { name } }` agent .use(traces => { const spans = sort(traces[0]) - expect(spans).to.have.length(8) + expect(spans).to.have.length(10) const execute = spans[3] const friendsField = spans[4] const friendsResolve = spans[5] - const friendNameField = spans[6] - const friendNameResolve = spans[7] + const friend0NameField = spans[6] + const friend0NameResolve = spans[7] + const friend1NameField = spans[8] + const friend1NameResolve = spans[9] expect(execute).to.have.property('name', 'graphql.execute') @@ -988,13 +980,21 @@ describe('Plugin', () => { expect(friendsResolve).to.have.property('resource', 'friends') expect(friendsResolve.parent_id.toString()).to.equal(friendsField.span_id.toString()) - expect(friendNameField).to.have.property('name', 'graphql.field') - expect(friendNameField).to.have.property('resource', 'friends.*.name') - expect(friendNameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) + expect(friend0NameField).to.have.property('name', 'graphql.field') + expect(friend0NameField).to.have.property('resource', 'friends.0.name') + expect(friend0NameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) - expect(friendNameResolve).to.have.property('name', 'graphql.resolve') - expect(friendNameResolve).to.have.property('resource', 'friends.*.name') - expect(friendNameResolve.parent_id.toString()).to.equal(friendNameField.span_id.toString()) + expect(friend0NameResolve).to.have.property('name', 'graphql.resolve') + expect(friend0NameResolve).to.have.property('resource', 'friends.0.name') + expect(friend0NameResolve.parent_id.toString()).to.equal(friend0NameField.span_id.toString()) + + expect(friend1NameField).to.have.property('name', 'graphql.field') + expect(friend1NameField).to.have.property('resource', 'friends.1.name') + expect(friend1NameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) + + expect(friend1NameResolve).to.have.property('name', 'graphql.resolve') + expect(friend1NameResolve).to.have.property('resource', 'friends.1.name') + expect(friend1NameResolve.parent_id.toString()).to.equal(friend1NameField.span_id.toString()) }) .then(done) .catch(done) From 2da5861bb7288c3e2646d8b1490aa76e1a963067 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 24 Oct 2018 15:50:37 -0400 Subject: [PATCH 12/30] add blacklist and whiltelist options to the http plugin (#330) --- docs/API.md | 6 ++++-- src/plugins/http.js | 29 +++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/docs/API.md b/docs/API.md index b6305adbef7..30a6e0003e5 100644 --- a/docs/API.md +++ b/docs/API.md @@ -213,11 +213,13 @@ query HelloWorld {
Configuration Options
-| Option | Default | Description | -|------------------|------------------|----------------------------------------| +| Option | Default | Description | +|------------------|---------------------------------------|-------------------| | service | http-client | The service name for this integration. | | splitByDomain | false | Use the remote endpoint host as the service name instead of the default. | | validateStatus | `code => code < 400 || code >= 500` | Callback function to determine if an HTTP response should be recorded as an error. It should take a status code as its only parameter and return `true` for success or `false` for errors. +| blacklist | [] | List of URLs that should not be instrumented. Can be a string, RegExp, callback that takes the URL as a parameter, or an array of any of these. +| whitelist | /.*/ | List of URLs that should be instrumented. If this is set, other URLs will not be instrumented. Can be a string, RegExp, callback that takes the URL as a parameter, or an array of any of these.

ioredis

diff --git a/src/plugins/http.js b/src/plugins/http.js index 5f092010e09..184234c58aa 100644 --- a/src/plugins/http.js +++ b/src/plugins/http.js @@ -9,7 +9,7 @@ const Tags = opentracing.Tags const FORMAT_HTTP_HEADERS = opentracing.FORMAT_HTTP_HEADERS function patch (http, methodName, tracer, config) { - config = normalizeConfig(config) + config = normalizeConfig(tracer, config) this.wrap(http, methodName, fn => makeRequestTrace(fn)) function makeRequestTrace (request) { @@ -19,7 +19,7 @@ function patch (http, methodName, tracer, config) { const options = args.options const callback = args.callback - if (uri === `${tracer._url.href}/v0.4/traces`) { + if (!config.filter(uri)) { return request.call(this, options, callback) } @@ -163,11 +163,32 @@ function getStatusValidator (config) { return code => code < 400 || code >= 500 } -function normalizeConfig (config) { +function getFilter (tracer, config) { + const whitelist = config.whitelist || /.*/ + const blacklist = [`${tracer._url.href}/v0.4/traces`].concat(config.blacklist || []) + + return uri => applyFilter(whitelist, uri) && !applyFilter(blacklist, uri) + + function applyFilter (filter, uri) { + if (typeof filter === 'function') { + return filter(uri) + } else if (filter instanceof RegExp) { + return filter.test(uri) + } else if (filter instanceof Array) { + return filter.some(filter => applyFilter(filter, uri)) + } + + return filter === uri + } +} + +function normalizeConfig (tracer, config) { const validateStatus = getStatusValidator(config) + const filter = getFilter(tracer, config) return Object.assign({}, config, { - validateStatus + validateStatus, + filter }) } From 77cea5714d33beedfee49fc461bac235c22e4dd4 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Fri, 26 Oct 2018 10:31:47 -0400 Subject: [PATCH 13/30] add graphql metadata (#335) --- src/plugins/graphql.js | 53 +++++++++++++++++++++++++++++------- test/plugins/graphql.spec.js | 28 +++++++++++++++++++ 2 files changed, 71 insertions(+), 10 deletions(-) diff --git a/src/plugins/graphql.js b/src/plugins/graphql.js index 7c0da1bfa9a..993d1d5e0c1 100644 --- a/src/plugins/graphql.js +++ b/src/plugins/graphql.js @@ -116,7 +116,7 @@ function wrapResolve (resolve, tracer, config, responsePathAsArray) { }) } - const field = assertField(tracer, config, operation, path) + const field = assertField(tracer, config, operation, path, info) const scope = tracer.scopeManager().activate(field.resolveSpan) return call(resolve, this, arguments, err => finish(scope, operation, path, err)) @@ -162,7 +162,7 @@ function call (fn, thisContext, args, callback) { } } -function assertField (tracer, config, operation, path) { +function assertField (tracer, config, operation, path, info) { let field = getField(operation, path) if (!field) { @@ -172,8 +172,8 @@ function assertField (tracer, config, operation, path) { } const fieldParent = getFieldParent(operation, path) - const childOf = createPathSpan(tracer, config, 'field', fieldParent, path) - const span = createPathSpan(tracer, config, 'resolve', childOf, path) + const childOf = createPathSpan(tracer, config, 'field', fieldParent, path, info) + const span = createPathSpan(tracer, config, 'resolve', childOf, path, info) field.parent = fieldParent field.span = childOf @@ -222,11 +222,12 @@ function addOperationSpan (tracer, config, operation, document, variableValues) tracer, config, operation, - document._datadog_source, + document, variableValues, startTime ) + Object.defineProperty(operation, '_datadog_source', { value: document._datadog_source }) Object.defineProperty(operation, '_datadog_span', { value: operationSpan }) Object.defineProperty(operation, '_datadog_fields', { value: {} }) } @@ -255,17 +256,26 @@ function addValidateSpan (tracer, config, document, operation) { } } -function createOperationSpan (tracer, config, operation, source, variableValues, startTime) { +function createOperationSpan (tracer, config, operation, document, variableValues, startTime) { const type = operation.operation const name = operation.name && operation.name.value + const def = document.definitions.find(def => def.kind === 'OperationDefinition') const parentScope = tracer.scopeManager().active() const tags = { 'service.name': getService(tracer, config), 'resource.name': [type, name].filter(val => val).join(' ') } - if (source) { - tags['graphql.document'] = source + if (def) { + tags['graphql.operation.type'] = def.operation + + if (def.name) { + tags['graphql.operation.name'] = def.name.value + } + } + + if (document._datadog_source) { + tags['graphql.document'] = document._datadog_source } if (variableValues && config.variables) { @@ -295,13 +305,36 @@ function createSpan (tracer, config, name, childOf, startTime) { return span } -function createPathSpan (tracer, config, name, childOf, path) { +function createPathSpan (tracer, config, name, childOf, path, info) { const span = createSpan(tracer, config, name, childOf) + const document = info.operation._datadog_source + const fieldNode = info.fieldNodes.find(fieldNode => fieldNode.kind === 'Field') span.addTags({ - 'resource.name': path.join('.') + 'resource.name': path.join('.'), + 'graphql.field.name': info.fieldName, + 'graphql.field.path': path.join('.'), + 'graphql.field.type': info.returnType }) + if (fieldNode) { + if (document) { + span.setTag('graphql.field.source', document.substring(fieldNode.loc.start, fieldNode.loc.end)) + } + + if (config.variables) { + const variables = config.variables(info.variableValues) + + fieldNode.arguments + .filter(arg => arg.value && arg.value.kind === 'Variable') + .filter(arg => arg.value.name && variables[arg.value.name.value]) + .map(arg => arg.value.name.value) + .forEach(name => { + span.setTag(`graphql.variables.${name}`, variables[name]) + }) + } + } + return span } diff --git a/test/plugins/graphql.spec.js b/test/plugins/graphql.spec.js index 2150e7a2c81..26501e88811 100644 --- a/test/plugins/graphql.spec.js +++ b/test/plugins/graphql.spec.js @@ -171,6 +171,8 @@ describe('Plugin', () => { expect(spans[0]).to.have.property('name', 'graphql.query') expect(spans[0]).to.have.property('resource', 'query MyQuery') expect(spans[0].meta).to.have.property('graphql.document', source) + expect(spans[0].meta).to.have.property('graphql.operation.type', 'query') + expect(spans[0].meta).to.have.property('graphql.operation.name', 'MyQuery') }) .then(done) .catch(done) @@ -203,6 +205,10 @@ describe('Plugin', () => { expect(spans[4]).to.have.property('service', 'test-graphql') expect(spans[4]).to.have.property('name', 'graphql.field') expect(spans[4]).to.have.property('resource', 'hello') + expect(spans[4].meta).to.have.property('graphql.field.name', 'hello') + expect(spans[4].meta).to.have.property('graphql.field.path', 'hello') + expect(spans[4].meta).to.have.property('graphql.field.type', 'String') + expect(spans[4].meta).to.have.property('graphql.field.source', 'hello(name: "world")') }) .then(done) .catch(done) @@ -221,6 +227,10 @@ describe('Plugin', () => { expect(spans[5]).to.have.property('service', 'test-graphql') expect(spans[5]).to.have.property('name', 'graphql.resolve') expect(spans[5]).to.have.property('resource', 'hello') + expect(spans[5].meta).to.have.property('graphql.field.name', 'hello') + expect(spans[5].meta).to.have.property('graphql.field.path', 'hello') + expect(spans[5].meta).to.have.property('graphql.field.type', 'String') + expect(spans[5].meta).to.have.property('graphql.field.source', 'hello(name: "world")') }) .then(done) .catch(done) @@ -330,39 +340,47 @@ describe('Plugin', () => { expect(humanField).to.have.property('name', 'graphql.field') expect(humanField).to.have.property('resource', 'human') + expect(humanField.meta).to.have.property('graphql.field.path', 'human') expect(humanField.parent_id.toString()).to.equal(execute.span_id.toString()) expect(humanField.duration.toNumber()).to.be.lte(execute.duration.toNumber()) expect(humanResolve).to.have.property('name', 'graphql.resolve') expect(humanResolve).to.have.property('resource', 'human') + expect(humanResolve.meta).to.have.property('graphql.field.path', 'human') expect(humanResolve.parent_id.toString()).to.equal(humanField.span_id.toString()) expect(humanResolve.duration.toNumber()).to.be.lte(humanField.duration.toNumber()) expect(humanNameField).to.have.property('name', 'graphql.field') expect(humanNameField).to.have.property('resource', 'human.name') + expect(humanNameField.meta).to.have.property('graphql.field.path', 'human.name') expect(humanNameField.parent_id.toString()).to.equal(humanField.span_id.toString()) expect(humanNameResolve).to.have.property('name', 'graphql.resolve') expect(humanNameResolve).to.have.property('resource', 'human.name') + expect(humanNameResolve.meta).to.have.property('graphql.field.path', 'human.name') expect(humanNameResolve.parent_id.toString()).to.equal(humanNameField.span_id.toString()) expect(addressField).to.have.property('name', 'graphql.field') expect(addressField).to.have.property('resource', 'human.address') + expect(addressField.meta).to.have.property('graphql.field.path', 'human.address') expect(addressField.parent_id.toString()).to.equal(humanField.span_id.toString()) expect(addressField.duration.toNumber()).to.be.lte(humanField.duration.toNumber()) expect(addressResolve).to.have.property('name', 'graphql.resolve') expect(addressResolve).to.have.property('resource', 'human.address') + expect(addressResolve.meta).to.have.property('graphql.field.path', 'human.address') expect(addressResolve.parent_id.toString()).to.equal(addressField.span_id.toString()) expect(addressResolve.duration.toNumber()).to.be.lte(addressField.duration.toNumber()) expect(addressCivicNumberField).to.have.property('name', 'graphql.field') expect(addressCivicNumberField).to.have.property('resource', 'human.address.civicNumber') + expect(addressCivicNumberField.meta).to.have.property('graphql.field.path', 'human.address.civicNumber') expect(addressCivicNumberField.parent_id.toString()).to.equal(addressField.span_id.toString()) expect(addressCivicNumberField.duration.toNumber()).to.be.lte(addressField.duration.toNumber()) expect(addressCivicNumberResolve).to.have.property('name', 'graphql.resolve') expect(addressCivicNumberResolve).to.have.property('resource', 'human.address.civicNumber') + expect(addressCivicNumberResolve.meta).to.have.property('graphql.field.path', 'human.address.civicNumber') expect(addressCivicNumberResolve.parent_id.toString()) .to.equal(addressCivicNumberField.span_id.toString()) expect(addressCivicNumberResolve.duration.toNumber()) @@ -370,11 +388,13 @@ describe('Plugin', () => { expect(addressStreetField).to.have.property('name', 'graphql.field') expect(addressStreetField).to.have.property('resource', 'human.address.street') + expect(addressStreetField.meta).to.have.property('graphql.field.path', 'human.address.street') expect(addressStreetField.parent_id.toString()).to.equal(addressField.span_id.toString()) expect(addressStreetField.duration.toNumber()).to.be.lte(addressField.duration.toNumber()) expect(addressStreetResolve).to.have.property('name', 'graphql.resolve') expect(addressStreetResolve).to.have.property('resource', 'human.address.street') + expect(addressStreetResolve.meta).to.have.property('graphql.field.path', 'human.address.street') expect(addressStreetResolve.parent_id.toString()).to.equal(addressStreetField.span_id.toString()) expect(addressStreetResolve.duration.toNumber()).to.be.lte(addressStreetField.duration.toNumber()) }) @@ -403,18 +423,22 @@ describe('Plugin', () => { expect(friendsField).to.have.property('name', 'graphql.field') expect(friendsField).to.have.property('resource', 'friends') + expect(friendsField.meta).to.have.property('graphql.field.path', 'friends') expect(friendsField.parent_id.toString()).to.equal(execute.span_id.toString()) expect(friendsResolve).to.have.property('name', 'graphql.resolve') expect(friendsResolve).to.have.property('resource', 'friends') + expect(friendsResolve.meta).to.have.property('graphql.field.path', 'friends') expect(friendsResolve.parent_id.toString()).to.equal(friendsField.span_id.toString()) expect(friendNameField).to.have.property('name', 'graphql.field') expect(friendNameField).to.have.property('resource', 'friends.*.name') + expect(friendNameField.meta).to.have.property('graphql.field.path', 'friends.*.name') expect(friendNameField.parent_id.toString()).to.equal(friendsField.span_id.toString()) expect(friendNameResolve).to.have.property('name', 'graphql.resolve') expect(friendNameResolve).to.have.property('resource', 'friends.*.name') + expect(friendNameResolve.meta).to.have.property('graphql.field.path', 'friends.*.name') expect(friendNameResolve.parent_id.toString()).to.equal(friendNameField.span_id.toString()) }) .then(done) @@ -802,6 +826,10 @@ describe('Plugin', () => { expect(spans[0].meta).to.have.property('graphql.variables.title', 'planet') expect(spans[0].meta).to.have.property('graphql.variables.who', 'REDACTED') + expect(spans[4].meta).to.have.property('graphql.variables.title', 'planet') + expect(spans[4].meta).to.have.property('graphql.variables.who', 'REDACTED') + expect(spans[5].meta).to.have.property('graphql.variables.title', 'planet') + expect(spans[5].meta).to.have.property('graphql.variables.who', 'REDACTED') }) .then(done) .catch(done) From b86243fbae8e20795fcaf9d32739e2435a4e56ec Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Fri, 26 Oct 2018 10:32:41 -0400 Subject: [PATCH 14/30] add graphql option to configure variables to record as an array (#336) --- docs/API.md | 4 ++-- src/plugins/graphql.js | 5 ++++- test/plugins/graphql.spec.js | 39 ++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/docs/API.md b/docs/API.md index 30a6e0003e5..0503f9b1b83 100644 --- a/docs/API.md +++ b/docs/API.md @@ -148,7 +148,7 @@ Each integration also has its own list of default tags. These tags get automatic

graphql

-The `graphql` integration uses the operation name as the span resource name. If no operation name is set, the resource name will always be just `query` or `mutation`. +The `graphql` integration uses the operation name as the span resource name. If no operation name is set, the resource name will always be just `query`, `mutation` or `subscription`. For example: @@ -178,7 +178,7 @@ query HelloWorld { | Option | Default | Description | |-----------------|--------------------------------------------------|------------------------------------------------------------------------| | service | *Service name of the app suffixed with -graphql* | The service name for this integration. | -| variables | `undefined` | A callback to enable recording of variables. By default, no variables are recorded. For example, using `variables => variables` would record all variables. | +| variables | [] | An array of variable names to record. Can also be a callback that returns the key/value pairs to record. For example, using `variables => variables` would record all variables. | | depth | -1 | The maximum depth of fields/resolvers to instrument. Set to `0` to only instrument the operation or to -1 to instrument all fields/resolvers. | | collapse | true | Whether to collapse list items into a single element. (i.e. single `users.*.name` span instead of `users.0.name`, `users.1.name`, etc) | diff --git a/src/plugins/graphql.js b/src/plugins/graphql.js index 993d1d5e0c1..dbdb64e1d1a 100644 --- a/src/plugins/graphql.js +++ b/src/plugins/graphql.js @@ -1,5 +1,6 @@ 'use strict' +const pick = require('lodash.pick') const platform = require('../platform') const log = require('../log') @@ -436,8 +437,10 @@ function getDepth (config) { function getVariablesFilter (config) { if (typeof config.variables === 'function') { return config.variables + } else if (config.variables instanceof Array) { + return variables => pick(variables, config.variables) } else if (config.hasOwnProperty('variables')) { - log.error('Expected `variables` to be a function.') + log.error('Expected `variables` to be an array or function.') } return null } diff --git a/test/plugins/graphql.spec.js b/test/plugins/graphql.spec.js index 26501e88811..e84f1edd169 100644 --- a/test/plugins/graphql.spec.js +++ b/test/plugins/graphql.spec.js @@ -838,6 +838,45 @@ describe('Plugin', () => { }) }) + describe('with an array of variable names', () => { + before(() => { + tracer = require('../..') + + return agent.load(plugin, 'graphql', { + variables: ['title'] + }) + }) + + after(() => { + return agent.close() + }) + + beforeEach(() => { + graphql = require(`../../versions/graphql@${version}`).get() + buildSchema() + }) + + it('should only include the configured variables', done => { + const source = ` + query MyQuery($title: String!, $who: String!) { + hello(title: $title, name: $who) + } + ` + + agent + .use(traces => { + const spans = sort(traces[0]) + + expect(spans[0].meta).to.have.property('graphql.variables.title', 'planet') + expect(spans[0].meta).to.not.have.property('graphql.variables.who') + }) + .then(done) + .catch(done) + + graphql.graphql(schema, source, null, null, { title: 'planet', who: 'world' }).catch(done) + }) + }) + describe('with a depth of 0', () => { before(() => { tracer = require('../..') From 9c054e1eccf5870cc4bade100ae1cf39790242fc Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Mon, 29 Oct 2018 13:09:15 -0400 Subject: [PATCH 15/30] add support for http servers without any framework (#338) --- src/plugins/{http.js => http/client.js} | 2 +- src/plugins/http/index.js | 6 ++ src/plugins/http/server.js | 34 ++++++++ src/plugins/util/web.js | 37 +++++--- .../{http.spec.js => http/client.spec.js} | 6 +- test/plugins/http/server.spec.js | 84 +++++++++++++++++++ test/plugins/util/web.spec.js | 25 ++++++ 7 files changed, 177 insertions(+), 17 deletions(-) rename src/plugins/{http.js => http/client.js} (99%) create mode 100644 src/plugins/http/index.js create mode 100644 src/plugins/http/server.js rename test/plugins/{http.spec.js => http/client.spec.js} (99%) create mode 100644 test/plugins/http/server.spec.js diff --git a/src/plugins/http.js b/src/plugins/http/client.js similarity index 99% rename from src/plugins/http.js rename to src/plugins/http/client.js index 184234c58aa..dde6469fd97 100644 --- a/src/plugins/http.js +++ b/src/plugins/http/client.js @@ -3,7 +3,7 @@ const url = require('url') const opentracing = require('opentracing') const semver = require('semver') -const log = require('../log') +const log = require('../../log') const Tags = opentracing.Tags const FORMAT_HTTP_HEADERS = opentracing.FORMAT_HTTP_HEADERS diff --git a/src/plugins/http/index.js b/src/plugins/http/index.js new file mode 100644 index 00000000000..1637d97c12d --- /dev/null +++ b/src/plugins/http/index.js @@ -0,0 +1,6 @@ +'use strict' + +const client = require('./client') +const server = require('./server') + +module.exports = [].concat(client, server) diff --git a/src/plugins/http/server.js b/src/plugins/http/server.js new file mode 100644 index 00000000000..69b3f72d210 --- /dev/null +++ b/src/plugins/http/server.js @@ -0,0 +1,34 @@ +'use strict' + +const web = require('../util/web') + +function createWrapEmit (tracer, config) { + config = web.normalizeConfig(config) + + return function wrapEmit (emit) { + return function emitWithTrace (eventName, req, res) { + if (eventName === 'request') { + web.instrument(tracer, config, req, res, 'http.request') + } + + return emit.apply(this, arguments) + } + } +} + +function plugin (name) { + return { + name, + patch (http, tracer, config) { + this.wrap(http.Server.prototype, 'emit', createWrapEmit(tracer, config)) + }, + unpatch (http) { + this.unwrap(http.Server.prototype, 'emit') + } + } +} + +module.exports = [ + plugin('http'), + plugin('https') +] diff --git a/src/plugins/util/web.js b/src/plugins/util/web.js index bb995ead3fd..4b03b732556 100644 --- a/src/plugins/util/web.js +++ b/src/plugins/util/web.js @@ -32,24 +32,14 @@ const web = { // Start a span and activate a scope for a request. instrument (tracer, config, req, res, name, callback) { - const childOf = tracer.extract(FORMAT_HTTP_HEADERS, req.headers) - const span = tracer.startSpan(name, { childOf }) - const scope = tracer.scopeManager().activate(span) + this.patch(req) + + const span = startSpan(tracer, config, req, res, name) if (config.service) { span.setTag(SERVICE_NAME, config.service) } - this.patch(req) - - req._datadog.tracer = tracer - req._datadog.config = config - req._datadog.span = span - req._datadog.scope = scope - req._datadog.res = res - - addRequestTags(req) - callback && callback(span) wrapEnd(req) @@ -98,6 +88,27 @@ const web = { } } +function startSpan (tracer, config, req, res, name) { + if (req._datadog.span) { + req._datadog.span.context().name = name + return req._datadog.span + } + + const childOf = tracer.extract(FORMAT_HTTP_HEADERS, req.headers) + const span = tracer.startSpan(name, { childOf }) + const scope = tracer.scopeManager().activate(span) + + req._datadog.tracer = tracer + req._datadog.config = config + req._datadog.span = span + req._datadog.scope = scope + req._datadog.res = res + + addRequestTags(req) + + return span +} + function finish (req) { if (req._datadog.finished) return diff --git a/test/plugins/http.spec.js b/test/plugins/http/client.spec.js similarity index 99% rename from test/plugins/http.spec.js rename to test/plugins/http/client.spec.js index 052c79663f5..492a02c93d4 100644 --- a/test/plugins/http.spec.js +++ b/test/plugins/http/client.spec.js @@ -1,7 +1,7 @@ 'use strict' const getPort = require('get-port') -const agent = require('./agent') +const agent = require('../agent') const semver = require('semver') wrapIt() @@ -34,8 +34,8 @@ describe('Plugin', () => { } beforeEach(() => { - plugin = require('../../src/plugins/http') - tracer = require('../..') + plugin = require('../../../src/plugins/http/client') + tracer = require('../../..') appListener = null }) diff --git a/test/plugins/http/server.spec.js b/test/plugins/http/server.spec.js new file mode 100644 index 00000000000..efb4cc4ad20 --- /dev/null +++ b/test/plugins/http/server.spec.js @@ -0,0 +1,84 @@ +'use strict' + +const getPort = require('get-port') +const agent = require('../agent') +const axios = require('axios') + +wrapIt() + +describe('Plugin', () => { + let plugin + let http + let listener + let appListener + let tracer + let port + let app + + describe('http/server', () => { + beforeEach(() => { + plugin = require('../../../src/plugins/http/server') + tracer = require('../../..') + listener = (req, res) => { + app && app(req, res) + res.writeHead(200) + res.end() + } + }) + + beforeEach(() => { + return getPort().then(newPort => { + port = newPort + }) + }) + + afterEach(() => { + appListener && appListener.close() + return agent.close() + }) + + describe('without configuration', () => { + beforeEach(() => { + return agent.load(plugin, 'http') + .then(() => { + http = require('http') + }) + }) + + beforeEach(done => { + const server = new http.Server(listener) + appListener = server + .listen(port, 'localhost', () => done()) + }) + + it('should do automatic instrumentation', done => { + agent + .use(traces => { + expect(traces[0][0]).to.have.property('name', 'http.request') + expect(traces[0][0]).to.have.property('service', 'test') + expect(traces[0][0]).to.have.property('type', 'http') + expect(traces[0][0]).to.have.property('resource', 'GET') + expect(traces[0][0].meta).to.have.property('span.kind', 'server') + expect(traces[0][0].meta).to.have.property('http.url', `http://localhost:${port}/user`) + expect(traces[0][0].meta).to.have.property('http.method', 'GET') + expect(traces[0][0].meta).to.have.property('http.status_code', '200') + }) + .then(done) + .catch(done) + + axios.get(`http://localhost:${port}/user`).catch(done) + }) + + it('should run the request listener in the request scope', done => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return done() + + app = (req, res) => { + expect(tracer.scopeManager().active()).to.not.be.null + done() + } + + axios.get(`http://localhost:${port}/user`).catch(done) + }) + }) + }) +}) diff --git a/test/plugins/util/web.spec.js b/test/plugins/util/web.spec.js index ea4ebc1689b..648086603a5 100644 --- a/test/plugins/util/web.spec.js +++ b/test/plugins/util/web.spec.js @@ -105,6 +105,31 @@ describe('plugins/util/web', () => { [`${HTTP_HEADERS}.host`]: 'localhost' }) }) + + it('should only start one span for the entire request', () => { + const span1 = web.instrument(tracer, config, req, res, 'test.request') + const scope1 = tracer.scopeManager().active() + const span2 = web.instrument(tracer, config, req, res, 'test.request') + const scope2 = tracer.scopeManager().active() + + expect(span1).to.equal(span2) + expect(scope1).to.equal(scope2) + }) + + it('should allow overriding the span name', () => { + span = web.instrument(tracer, config, req, res, 'test.request') + span = web.instrument(tracer, config, req, res, 'test2.request') + + expect(span.context().name).to.equal('test2.request') + }) + + it('should allow overriding the span service name', () => { + span = web.instrument(tracer, config, req, res, 'test.request') + config.service = 'test2' + span = web.instrument(tracer, config, req, res, 'test.request') + + expect(span.context().tags).to.have.property('service.name', 'test2') + }) }) describe('on request end', () => { From 5ef4f6b670f49e567565c88cc0b34c0b33a9825e Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Tue, 30 Oct 2018 16:34:51 -0400 Subject: [PATCH 16/30] add support for the router module (#342) --- src/plugins/express.js | 124 ++--------------------------------- src/plugins/router.js | 127 ++++++++++++++++++++++++++++++++++++ test/plugins/router.spec.js | 85 ++++++++++++++++++++++++ 3 files changed, 217 insertions(+), 119 deletions(-) create mode 100644 src/plugins/router.js create mode 100644 test/plugins/router.spec.js diff --git a/src/plugins/express.js b/src/plugins/express.js index d6dd143e19a..79010f0bfc0 100644 --- a/src/plugins/express.js +++ b/src/plugins/express.js @@ -1,8 +1,8 @@ 'use strict' const METHODS = require('methods').concat('use', 'route', 'param', 'all') -const pathToRegExp = require('path-to-regexp') const web = require('./util/web') +const routerPlugin = require('./router') function createWrapMethod (tracer, config) { config = web.normalizeConfig(config) @@ -26,128 +26,14 @@ function createWrapMethod (tracer, config) { } } -function createWrapHandle (tracer, config) { - return function wrapHandle (handle) { - return function handleWithTracer (req) { - web.patch(req) - - return handle.apply(this, arguments) - } - } -} - -function createWrapProcessParams (tracer, config) { - return function wrapProcessParams (processParams) { - return function processParamsWithTrace (layer, called, req, res, done) { - const matchers = layer._datadog_matchers - - req = done ? req : called - - if (matchers) { - // Try to guess which path actually matched - for (let i = 0; i < matchers.length; i++) { - if (matchers[i].test(layer.path)) { - web.enterRoute(req, matchers[i].path) - - break - } - } - } - - return processParams.apply(this, arguments) - } - } -} - -function createWrapRouterMethod (tracer) { - return function wrapRouterMethod (original) { - return function methodWithTrace (fn) { - const offset = this.stack.length - const router = original.apply(this, arguments) - const matchers = extractMatchers(fn) - - this.stack.slice(offset).forEach(layer => { - const handle = layer.handle - - if (handle.length === 4) { - layer.handle = (error, req, res, next) => { - return handle.call(layer, error, req, res, wrapNext(tracer, layer, req, next)) - } - } else { - layer.handle = (req, res, next) => { - return handle.call(layer, req, res, wrapNext(tracer, layer, req, next)) - } - } - - layer._datadog_matchers = matchers - }) - - return router - } - } -} - -function wrapNext (tracer, layer, req, next) { - if (!web.active(req)) { - return next - } - - const originalNext = next - - web.reactivate(req) - - return function (error) { - if (!error && layer.path && !isFastStar(layer)) { - web.exitRoute(req) - } - - process.nextTick(() => { - originalNext.apply(null, arguments) - }) - } -} - -function extractMatchers (fn) { - const arg = flatten([].concat(fn)) - - if (typeof arg[0] === 'function') { - return [] - } - - return arg.map(pattern => ({ - path: pattern instanceof RegExp ? `(${pattern})` : pattern, - test: path => pathToRegExp(pattern).test(path) - })) -} - -function isFastStar (layer) { - if (layer.regexp.fast_star !== undefined) { - return layer.regexp.fast_star - } - - return layer._datadog_matchers.some(matcher => matcher.path === '*') -} - -function flatten (arr) { - return arr.reduce((acc, val) => Array.isArray(val) ? acc.concat(flatten(val)) : acc.concat(val), []) -} - function patch (express, tracer, config) { - METHODS.forEach(method => { - this.wrap(express.application, method, createWrapMethod(tracer, config)) - }) - this.wrap(express.Router, 'handle', createWrapHandle(tracer, config)) - this.wrap(express.Router, 'process_params', createWrapProcessParams(tracer, config)) - this.wrap(express.Router, 'use', createWrapRouterMethod(tracer, config)) - this.wrap(express.Router, 'route', createWrapRouterMethod(tracer, config)) + this.wrap(express.application, METHODS, createWrapMethod(tracer, config)) + routerPlugin.patch.call(this, { prototype: express.Router }, tracer, config) } function unpatch (express) { - METHODS.forEach(method => this.unwrap(express.application, method)) - this.unwrap(express.Router, 'handle') - this.unwrap(express.Router, 'process_params') - this.unwrap(express.Router, 'use') - this.unwrap(express.Router, 'route') + this.unwrap(express.application, METHODS) + routerPlugin.unpatch.call(this, { prototype: express.Router }) } module.exports = { diff --git a/src/plugins/router.js b/src/plugins/router.js new file mode 100644 index 00000000000..8acc50a8e48 --- /dev/null +++ b/src/plugins/router.js @@ -0,0 +1,127 @@ +'use strict' + +const pathToRegExp = require('path-to-regexp') +const web = require('./util/web') + +function createWrapHandle (tracer, config) { + return function wrapHandle (handle) { + return function handleWithTracer (req) { + web.patch(req) + + return handle.apply(this, arguments) + } + } +} + +function createWrapProcessParams (tracer, config) { + return function wrapProcessParams (processParams) { + return function processParamsWithTrace (layer, called, req, res, done) { + const matchers = layer._datadog_matchers + + req = done ? req : called + + if (web.active(req) && matchers) { + // Try to guess which path actually matched + for (let i = 0; i < matchers.length; i++) { + if (matchers[i].test(layer.path)) { + web.enterRoute(req, matchers[i].path) + + break + } + } + } + + return processParams.apply(this, arguments) + } + } +} + +function createWrapRouterMethod (tracer) { + return function wrapRouterMethod (original) { + return function methodWithTrace (fn) { + const offset = this.stack.length + const router = original.apply(this, arguments) + const matchers = extractMatchers(fn) + + this.stack.slice(offset).forEach(layer => { + const handle = layer.handle + + if (handle.length === 4) { + layer.handle = (error, req, res, next) => { + return handle.call(layer, error, req, res, wrapNext(tracer, layer, req, next)) + } + } else { + layer.handle = (req, res, next) => { + return handle.call(layer, req, res, wrapNext(tracer, layer, req, next)) + } + } + + layer._datadog_matchers = matchers + }) + + return router + } + } +} + +function wrapNext (tracer, layer, req, next) { + if (!web.active(req)) { + return next + } + + const originalNext = next + + web.reactivate(req) + + return function (error) { + if (!error && layer.path && !isFastStar(layer)) { + web.exitRoute(req) + } + + process.nextTick(() => { + originalNext.apply(null, arguments) + }) + } +} + +function extractMatchers (fn) { + const arg = flatten([].concat(fn)) + + if (typeof arg[0] === 'function') { + return [] + } + + return arg.map(pattern => ({ + path: pattern instanceof RegExp ? `(${pattern})` : pattern, + test: path => pathToRegExp(pattern).test(path) + })) +} + +function isFastStar (layer) { + if (layer.regexp.fast_star !== undefined) { + return layer.regexp.fast_star + } + + return layer._datadog_matchers.some(matcher => matcher.path === '*') +} + +function flatten (arr) { + return arr.reduce((acc, val) => Array.isArray(val) ? acc.concat(flatten(val)) : acc.concat(val), []) +} + +module.exports = { + name: 'router', + versions: ['1.x'], + patch (Router, tracer, config) { + this.wrap(Router.prototype, 'handle', createWrapHandle(tracer, config)) + this.wrap(Router.prototype, 'process_params', createWrapProcessParams(tracer, config)) + this.wrap(Router.prototype, 'use', createWrapRouterMethod(tracer, config)) + this.wrap(Router.prototype, 'route', createWrapRouterMethod(tracer, config)) + }, + unpatch (Router) { + this.unwrap(Router.prototype, 'handle') + this.unwrap(Router.prototype, 'process_params') + this.unwrap(Router.prototype, 'use') + this.unwrap(Router.prototype, 'route') + } +} diff --git a/test/plugins/router.spec.js b/test/plugins/router.spec.js new file mode 100644 index 00000000000..5f70ac6df3f --- /dev/null +++ b/test/plugins/router.spec.js @@ -0,0 +1,85 @@ +'use strict' + +// TODO: move tests from express since it uses the router plugin now + +const axios = require('axios') +const http = require('http') +const getPort = require('get-port') +const agent = require('./agent') +const web = require('../../src/plugins/util/web') +const plugin = require('../../src/plugins/router') + +wrapIt() + +describe('Plugin', () => { + let tracer + let Router + let appListener + + function server (router) { + return http.createServer((req, res) => { + const config = web.normalizeConfig({}) + + web.instrument(tracer, config, req, res, 'http.request') + + return router(req, res, err => { + res.writeHead(err ? 500 : 404) + res.end() + }) + }) + } + + describe('router', () => { + withVersions(plugin, 'router', version => { + beforeEach(() => { + tracer = require('../..') + }) + + afterEach(() => { + appListener.close() + }) + + describe('without configuration', () => { + before(() => { + return agent.load(plugin, 'router') + }) + + after(() => { + return agent.close() + }) + + beforeEach(() => { + Router = require(`../../versions/router@${version}`).get() + }) + + it('should add the route to the request span', done => { + const router = Router() + const childRouter = Router() + + childRouter.use('/child/:id', (req, res) => { + res.writeHead(200) + res.end() + }) + + router.use('/parent', childRouter) + + getPort().then(port => { + agent + .use(traces => { + expect(traces[0]).to.have.length(1) + expect(traces[0][0]).to.have.property('resource', 'GET /parent/child/:id') + }) + .then(done) + .catch(done) + + appListener = server(router).listen(port, 'localhost', () => { + axios + .get(`http://localhost:${port}/parent/child/123`) + .catch(done) + }) + }) + }) + }) + }) + }) +}) From 996aa4ef5f06f92a73a7de00ddc828015e9c03da Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Mon, 5 Nov 2018 09:24:34 -0500 Subject: [PATCH 17/30] add the http.route tag which allows overriding the route (#348) --- ext/tags.js | 1 + src/plugins/util/web.js | 13 +++++++++++-- test/plugins/util/web.spec.js | 25 ++++++++++++++++++++++++- 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/ext/tags.js b/ext/tags.js index 477b3d1c599..33b9be219da 100644 --- a/ext/tags.js +++ b/ext/tags.js @@ -13,5 +13,6 @@ module.exports = { HTTP_URL: 'http.url', HTTP_METHOD: 'http.method', HTTP_STATUS_CODE: 'http.status_code', + HTTP_ROUTE: 'http.route', HTTP_HEADERS: 'http.headers' } diff --git a/src/plugins/util/web.js b/src/plugins/util/web.js index 4b03b732556..fde4cd5280e 100644 --- a/src/plugins/util/web.js +++ b/src/plugins/util/web.js @@ -16,6 +16,7 @@ const ERROR = tags.ERROR const HTTP_METHOD = tags.HTTP_METHOD const HTTP_URL = tags.HTTP_URL const HTTP_STATUS_CODE = tags.HTTP_STATUS_CODE +const HTTP_ROUTE = tags.HTTP_ROUTE const HTTP_HEADERS = tags.HTTP_HEADERS const web = { @@ -150,11 +151,19 @@ function addRequestTags (req) { } function addResponseTags (req) { - const path = req._datadog.paths.join('') - const resource = [req.method].concat(path).filter(val => val).join(' ') const span = req._datadog.span + const tags = span.context().tags const res = req._datadog.res + if (!tags.hasOwnProperty(HTTP_ROUTE) && req._datadog.paths.length > 0) { + span.setTag(HTTP_ROUTE, req._datadog.paths.join('')) + } + + const resource = [req.method] + .concat(tags[HTTP_ROUTE]) + .filter(val => val) + .join(' ') + span.addTags({ [RESOURCE_NAME]: resource, [HTTP_STATUS_CODE]: res.statusCode diff --git a/test/plugins/util/web.spec.js b/test/plugins/util/web.spec.js index 648086603a5..1e7350d543b 100644 --- a/test/plugins/util/web.spec.js +++ b/test/plugins/util/web.spec.js @@ -14,6 +14,7 @@ const ERROR = tags.ERROR const HTTP_METHOD = tags.HTTP_METHOD const HTTP_URL = tags.HTTP_URL const HTTP_STATUS_CODE = tags.HTTP_STATUS_CODE +const HTTP_ROUTE = tags.HTTP_ROUTE const HTTP_HEADERS = tags.HTTP_HEADERS describe('plugins/util/web', () => { @@ -205,6 +206,16 @@ describe('plugins/util/web', () => { [ERROR]: 'true' }) }) + + it('should use the user provided route', () => { + span.setTag('http.route', '/custom/route') + + res.end() + + expect(span.context().tags).to.include({ + [HTTP_ROUTE]: '/custom/route' + }) + }) }) }) @@ -222,10 +233,22 @@ describe('plugins/util/web', () => { res.end() expect(span.context().tags).to.have.property(RESOURCE_NAME, 'GET /foo/bar') + expect(span.context().tags).to.have.property(HTTP_ROUTE, '/foo/bar') + }) + + it('should not override user provided routes', () => { + req.method = 'GET' + span.setTag('http.route', '/custom') + + web.enterRoute(req, '/foo') + res.end() + + expect(span.context().tags).to.have.property(RESOURCE_NAME, 'GET /custom') + expect(span.context().tags).to.have.property(HTTP_ROUTE, '/custom') }) }) - describe('enterRoute', () => { + describe('exitRoute', () => { beforeEach(() => { config = web.normalizeConfig(config) span = web.instrument(tracer, config, req, res, 'test.request') From 20348784da2b5b6e6c5f606730802786616d086b Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Mon, 5 Nov 2018 12:54:37 -0500 Subject: [PATCH 18/30] add span hooks for web framework integrations (#351) --- src/plugins/util/web.js | 19 ++++++++++++++++--- test/plugins/util/web.spec.js | 22 +++++++++++++++++++++- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/plugins/util/web.js b/src/plugins/util/web.js index fde4cd5280e..0816f3069cc 100644 --- a/src/plugins/util/web.js +++ b/src/plugins/util/web.js @@ -24,10 +24,12 @@ const web = { normalizeConfig (config) { const headers = getHeadersToRecord(config) const validateStatus = getStatusValidator(config) + const hooks = getHooks(config) return Object.assign({}, config, { headers, - validateStatus + validateStatus, + hooks }) }, @@ -78,6 +80,7 @@ const web = { span: null, scope: null, paths: [], + hooks: [], beforeEnd: [] } }) @@ -90,6 +93,8 @@ const web = { } function startSpan (tracer, config, req, res, name) { + req._datadog.hooks.push(config.hooks) + if (req._datadog.span) { req._datadog.span.context().name = name return req._datadog.span @@ -110,11 +115,12 @@ function startSpan (tracer, config, req, res, name) { return span } -function finish (req) { +function finish (req, res) { if (req._datadog.finished) return addResponseTags(req) + req._datadog.hooks.forEach(hooks => hooks.request(req._datadog.span, req, res)) req._datadog.span.finish() req._datadog.scope && req._datadog.scope.close() req._datadog.finished = true @@ -129,7 +135,7 @@ function wrapEnd (req) { const returnValue = end.apply(this, arguments) - finish(req) + finish(req, res) return returnValue } @@ -212,4 +218,11 @@ function getStatusValidator (config) { return code => code < 500 } +function getHooks (config) { + const noop = () => {} + const request = (config.hooks && config.hooks.request) || noop + + return { request } +} + module.exports = web diff --git a/test/plugins/util/web.spec.js b/test/plugins/util/web.spec.js index 1e7350d543b..cb0b92ae2fe 100644 --- a/test/plugins/util/web.spec.js +++ b/test/plugins/util/web.spec.js @@ -37,7 +37,7 @@ describe('plugins/util/web', () => { res = { end } - config = {} + config = { hooks: {} } tracer = require('../../..').init({ plugins: false }) web = require('../../../src/plugins/util/web') @@ -216,6 +216,26 @@ describe('plugins/util/web', () => { [HTTP_ROUTE]: '/custom/route' }) }) + + it('should execute the request end hook', () => { + config.hooks.request = sinon.spy() + + res.end() + + expect(config.hooks.request).to.have.been.calledWith(span, req, res) + }) + + it('should execute multiple end hooks', () => { + config.hooks = { + request: sinon.spy() + } + + span = web.instrument(tracer, config, req, res, 'test.request') + + res.end() + + expect(config.hooks.request).to.have.been.calledWith(span, req, res) + }) }) }) From 57dfacdf53d5c3155418661968f7f059a4cdb81a Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Mon, 5 Nov 2018 14:59:02 -0500 Subject: [PATCH 19/30] fix mongo operation name and add missing query tag (#353) --- src/plugins/mongodb-core.js | 212 +++--------------------------- test/plugins/mongodb-core.spec.js | 22 +--- 2 files changed, 22 insertions(+), 212 deletions(-) diff --git a/src/plugins/mongodb-core.js b/src/plugins/mongodb-core.js index 75acb96d0c8..b75c7ec6078 100644 --- a/src/plugins/mongodb-core.js +++ b/src/plugins/mongodb-core.js @@ -2,200 +2,15 @@ const Buffer = require('safe-buffer').Buffer -// TODO: remove sanitization when implemented by the agent - -// Reference https://docs.mongodb.com/v3.6/reference/command/ -const DATABASE_COMMANDS = [ - // Aggregation Commands - 'aggregate', - 'count', - 'distinct', - 'group', - 'mapReduce', - - // Geospatial Commands - 'geoNear', - 'geoSearch', - - // Query and Write Operation Commands - 'delete', - 'eval', - 'find', - 'findAndModify', - 'getLastError', - 'getMore', - 'getPrevError', - 'insert', - 'parallelCollectionScan', - 'resetError', - 'update', - - // Query Plan Cache Commands - 'planCacheClear', - 'planCacheClearFilters', - 'planCacheListFilters', - 'planCacheListPlans', - 'planCacheListQueryShapes', - 'planCacheSetFilter', - - // Authentication Commands - 'authenticate', - 'authSchemaUpgrade', - 'copydbgetnonce', - 'getnonce', - 'logout', - - // User Management Commands - 'createUser', - 'dropAllUsersFromDatabase', - 'dropUser', - 'grantRolesToUser', - 'revokeRolesFromUser', - 'updateUser', - 'usersInfo', - - // Role Management Commands - 'createRole', - 'dropRole', - 'dropAllRolesFromDatabase', - 'grantPrivilegesToRole', - 'grantRolesToRole', - 'invalidateUserCache', - 'revokePrivilegesFromRole', - 'revokeRolesFromRole', - 'rolesInfo', - 'updateRole', - - // Replication Commands - 'applyOps', - 'isMaster', - 'replSetAbortPrimaryCatchUp', - 'replSetFreeze', - 'replSetGetConfig', - 'replSetGetStatus', - 'replSetInitiate', - 'replSetMaintenance', - 'replSetReconfig', - 'replSetResizeOplog', - 'replSetStepDown', - 'replSetSyncFrom', - 'resync', - - // Sharding Commands - 'addShard', - 'addShardToZone', - 'balancerStart', - 'balancerStatus', - 'balancerStop', - 'checkShardingIndex', - 'cleanupOrphaned', - 'enableSharding', - 'flushRouterConfig', - 'getShardMap', - 'getShardVersion', - 'isdbgrid', - 'listShards', - 'medianKey', - 'moveChunk', - 'movePrimary', - 'mergeChunks', - 'removeShard', - 'removeShardFromZone', - 'setShardVersion', - 'shardCollection', - 'shardingState', - 'split', - 'splitChunk', - 'splitVector', - 'unsetSharding', - 'updateZoneKeyRange', - - // Session Commands - 'endSessions', - 'killAllSessions', - 'killAllSessionsByPattern', - 'killSessions', - 'refreshSessions', - 'startSession', - - // Administration Commands - 'clean', - 'clone', - 'cloneCollection', - 'cloneCollectionAsCapped', - 'collMod', - 'compact', - 'connPoolSync', - 'convertToCapped', - 'copydb', - 'create', - 'createIndexes', - 'currentOp', - 'drop', - 'dropDatabase', - 'dropIndexes', - 'filemd5', - 'fsync', - 'fsyncUnlock', - 'getParameter', - 'killCursors', - 'killOp', - 'listCollections', - 'listDatabases', - 'listIndexes', - 'logRotate', - 'reIndex', - 'renameCollection', - 'repairCursor', - 'repairDatabase', - 'setFeatureCompatibilityVersion', - 'setParameter', - 'shutdown', - 'touch', - - // Diagnostic Commands - 'availableQueryOptions', - 'buildInfo', - 'collStats', - 'connPoolStats', - 'connectionStatus', - 'cursorInfo', - 'dataSize', - 'dbHash', - 'dbStats', - 'diagLogging', - 'driverOIDTest', - 'explain', - 'features', - 'getCmdLineOpts', - 'getLog', - 'hostInfo', - 'isSelf', - 'listCommands', - 'netstat', - 'ping', - 'profile', - 'serverStatus', - 'shardConnPoolStats', - 'top', - 'validate', - 'whatsmyuri', - - // System Events Auditing Commands - 'logApplicationMessage' -] - function createWrapOperation (tracer, config, operationName) { return function wrapOperation (operation) { return function operationWithTrace (ns, ops, options, callback) { - const resource = getResource(ns, ops, operationName) - const parentScope = tracer.scopeManager().active() const span = tracer.startSpan('mongodb.query', { childOf: parentScope && parentScope.span() }) - addTags(span, tracer, config, resource, ns, this) + addTags(span, tracer, config, ns, ops, this, operationName) if (typeof options === 'function') { return operation.call(this, ns, ops, wrapCallback(tracer, span, options)) @@ -209,14 +24,12 @@ function createWrapOperation (tracer, config, operationName) { function createWrapNext (tracer, config) { return function wrapNext (next) { return function nextWithTrace (cb) { - const resource = getResource(this.ns, this.cmd) - const parentScope = tracer.scopeManager().active() const span = tracer.startSpan('mongodb.query', { childOf: parentScope && parentScope.span() }) - addTags(span, tracer, config, resource, this.ns, this.topology) + addTags(span, tracer, config, this.ns, this.cmd, this.topology) if (this.cursorState) { span.addTags({ @@ -229,7 +42,10 @@ function createWrapNext (tracer, config) { } } -function addTags (span, tracer, config, resource, ns, topology) { +function addTags (span, tracer, config, ns, cmd, topology, operationName) { + const query = getQuery(cmd) + const resource = getResource(ns, cmd, query, operationName) + span.addTags({ 'service.name': config.service || `${tracer._service}-mongodb`, 'resource.name': resource, @@ -237,6 +53,10 @@ function addTags (span, tracer, config, resource, ns, topology) { 'db.name': ns }) + if (query) { + span.setTag('mongodb.query', query) + } + if (topology.s && topology.s.options) { span.addTags({ 'out.host': topology.s.options.host, @@ -263,15 +83,19 @@ function wrapCallback (tracer, span, done) { } } -function getResource (ns, cmd, operationName) { +function getQuery (cmd) { + return cmd.query && JSON.stringify(sanitize(cmd.query)) +} + +function getResource (ns, cmd, query, operationName) { if (!operationName) { - operationName = DATABASE_COMMANDS.find(name => cmd[name] !== undefined) || 'unknownCommand' + operationName = Object.keys(cmd)[0] } const parts = [operationName, ns] - if (cmd.query) { - parts.push(JSON.stringify(sanitize(cmd.query))) + if (query) { + parts.push(query) } return parts.join(' ') diff --git a/test/plugins/mongodb-core.spec.js b/test/plugins/mongodb-core.spec.js index dca663dec89..c40b946c665 100644 --- a/test/plugins/mongodb-core.spec.js +++ b/test/plugins/mongodb-core.spec.js @@ -87,29 +87,15 @@ describe('Plugin', () => { }, () => {}) }) - it('should use a fallback for unknown commands', done => { + it('should sanitize the query', done => { agent .use(traces => { const span = traces[0][0] - const resource = `unknownCommand test.${collection}` - - expect(span).to.have.property('resource', resource) - }) - .then(done) - .catch(done) - - server.command(`test.${collection}`, { - invalidCommand: `test.${collection}` - }, () => {}) - }) - - it('should sanitize the query as the resource', done => { - agent - .use(traces => { - const span = traces[0][0] - const resource = `find test.${collection} {"foo":"?","bar":{"baz":"?"}}` + const query = '{"foo":"?","bar":{"baz":"?"}}' + const resource = `find test.${collection} ${query}` expect(span).to.have.property('resource', resource) + expect(span.meta).to.have.property('mongodb.query', query) }) .then(done) .catch(done) From 9476ac8bea6ad8edbeacac790709a934d22e6982 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 7 Nov 2018 07:04:49 -0500 Subject: [PATCH 20/30] add redis.raw_command tag and extract common redis functionality (#356) --- src/plugins/ioredis.js | 35 +++----------- src/plugins/redis.js | 61 +++++------------------- src/plugins/util/redis.js | 63 +++++++++++++++++++++++++ src/plugins/util/tx.js | 57 +++++++++++++++++++++++ test/plugins/ioredis.spec.js | 1 + test/plugins/redis.spec.js | 1 + test/plugins/util/redis.spec.js | 82 +++++++++++++++++++++++++++++++++ test/plugins/util/tx.spec.js | 81 ++++++++++++++++++++++++++++++++ 8 files changed, 303 insertions(+), 78 deletions(-) create mode 100644 src/plugins/util/redis.js create mode 100644 src/plugins/util/tx.js create mode 100644 test/plugins/util/redis.spec.js create mode 100644 test/plugins/util/tx.spec.js diff --git a/src/plugins/ioredis.js b/src/plugins/ioredis.js index 8028545fdf9..1738cab90e6 100644 --- a/src/plugins/ioredis.js +++ b/src/plugins/ioredis.js @@ -1,44 +1,21 @@ 'use strict' +const tx = require('./util/redis') + function createWrapSendCommand (tracer, config) { return function wrapSendCommand (sendCommand) { return function sendCommandWithTrace (command, stream) { - const scope = tracer.scopeManager().active() - const span = tracer.startSpan('redis.command', { - childOf: scope && scope.span(), - tags: { - 'span.kind': 'client', - 'span.type': 'redis', - 'service.name': config.service || `${tracer._service}-redis`, - 'resource.name': command.name, - 'db.type': 'redis', - 'db.name': this.options.db || '0', - 'out.host': this.options.host, - 'out.port': String(this.options.port) - } - }) + const db = this.options.db + const span = tx.instrument(tracer, config, db, command.name, command.args) - command.promise - .then(() => finish(span)) - .catch(err => finish(span, err)) + tx.setHost(span, this.options.host, this.options.port) + tx.wrap(span, command.promise) return sendCommand.apply(this, arguments) } } } -function finish (span, error) { - if (error) { - span.addTags({ - 'error.type': error.name, - 'error.msg': error.message, - 'error.stack': error.stack - }) - } - - span.finish() -} - module.exports = { name: 'ioredis', versions: ['4.x'], diff --git a/src/plugins/redis.js b/src/plugins/redis.js index df8a4039784..160f3d48219 100644 --- a/src/plugins/redis.js +++ b/src/plugins/redis.js @@ -1,13 +1,13 @@ 'use strict' -const Tags = require('opentracing').Tags +const tx = require('./util/redis') function createWrapInternalSendCommand (tracer, config) { return function wrapInternalSendCommand (internalSendCommand) { return function internalSendCommandWithTrace (options) { - const span = startSpan(tracer, config, this, options.command) + const span = startSpan(tracer, config, this, options.command, options.args) - options.callback = wrapCallback(tracer, span, options.callback) + options.callback = tx.wrap(span, options.callback) return internalSendCommand.call(this, options) } @@ -17,14 +17,14 @@ function createWrapInternalSendCommand (tracer, config) { function createWrapSendCommand (tracer, config) { return function wrapSendCommand (sendCommand) { return function sendCommandWithTrace (command, args, callback) { - const span = startSpan(tracer, config, this, command) + const span = startSpan(tracer, config, this, command, args) if (callback) { - callback = wrapCallback(tracer, span, callback) + callback = tx.wrap(span, callback) } else if (args) { - args[(args.length || 1) - 1] = wrapCallback(tracer, span, args[args.length - 1]) + args[(args.length || 1) - 1] = tx.wrap(span, args[args.length - 1]) } else { - args = [wrapCallback(tracer, span)] + args = [tx.wrap(span)] } return sendCommand.call(this, command, args, callback) @@ -32,53 +32,16 @@ function createWrapSendCommand (tracer, config) { } } -function startSpan (tracer, config, client, command) { - const scope = tracer.scopeManager().active() - const span = tracer.startSpan('redis.command', { - childOf: scope && scope.span(), - tags: { - [Tags.SPAN_KIND]: Tags.SPAN_KIND_RPC_CLIENT, - [Tags.DB_TYPE]: 'redis', - 'service.name': config.service || `${tracer._service}-redis`, - 'resource.name': command, - 'span.type': 'redis', - 'db.name': client.selected_db || '0' - } - }) - - const connectionOptions = client.connection_options || client.connection_option || { - host: client.options.host || '127.0.0.1', - port: client.options.port || 6379 - } +function startSpan (tracer, config, client, command, args) { + const db = client.selected_db + const connectionOptions = client.connection_options || client.connection_option || {} + const span = tx.instrument(tracer, config, db, command, args) - if (connectionOptions) { - span.addTags({ - 'out.host': String(connectionOptions.host), - 'out.port': String(connectionOptions.port) - }) - } + tx.setHost(span, connectionOptions.host, connectionOptions.port) return span } -function wrapCallback (tracer, span, done) { - return (err, res) => { - if (err) { - span.addTags({ - 'error.type': err.name, - 'error.msg': err.message, - 'error.stack': err.stack - }) - } - - span.finish() - - if (typeof done === 'function') { - done(err, res) - } - } -} - module.exports = [ { name: 'redis', diff --git a/src/plugins/util/redis.js b/src/plugins/util/redis.js new file mode 100644 index 00000000000..210e114927c --- /dev/null +++ b/src/plugins/util/redis.js @@ -0,0 +1,63 @@ +'use strict' + +const tx = require('./tx') + +const redis = { + // Start a span for a Redis command. + instrument (tracer, config, db, command, args) { + const scope = tracer.scopeManager().active() + const span = tracer.startSpan('redis.command', { + childOf: scope && scope.span(), + tags: { + 'span.kind': 'client', + 'resource.name': command, + 'span.type': 'redis', + 'db.type': 'redis', + 'db.name': db || '0', + 'out.host': '127.0.0.1', + 'out.port': String(6379), + 'redis.raw_command': formatCommand(command, args) + } + }) + + span.setTag('service.name', config.service || `${span.context().tags['service.name']}-redis`) + + return span + } +} + +function formatCommand (command, args) { + command = command.toUpperCase() + + if (!args) return command + + for (let i = 0, l = args.length; i < l; i++) { + if (typeof args[i] === 'function') continue + + command = `${command} ${formatArg(args[i])}` + + if (command.length > 1000) return trim(command, 1000) + } + + return command +} + +function formatArg (arg) { + switch (typeof arg) { + case 'string': + case 'number': + return trim(String(arg), 100) + default: + return '?' + } +} + +function trim (str, maxlen) { + if (str.length > maxlen) { + str = str.substr(0, maxlen - 3) + '...' + } + + return str +} + +module.exports = Object.assign({}, tx, redis) diff --git a/src/plugins/util/tx.js b/src/plugins/util/tx.js new file mode 100644 index 00000000000..603087ef05f --- /dev/null +++ b/src/plugins/util/tx.js @@ -0,0 +1,57 @@ +'use strict' + +const tx = { + // Set the outgoing host. + setHost (span, hostname, port) { + hostname && span.setTag('out.host', hostname) + port && span.setTag('out.port', port) + }, + + // Wrap a promise or a callback to also finish the span. + wrap (span, done) { + if (typeof done === 'function' || !done) { + return wrapCallback(span, done) + } else if (isPromise(done)) { + return wrapPromise(span, done) + } + } +} + +function wrapCallback (span, callback) { + return function (err) { + finish(span, err) + + if (callback) { + callback.apply(this, arguments) + } + } +} + +function wrapPromise (span, promise) { + promise.then( + () => finish(span), + err => finish(span, err) + ) +} + +function finish (span, error) { + if (error) { + span.addTags({ + 'error.type': error.name, + 'error.msg': error.message, + 'error.stack': error.stack + }) + } + + span.finish() +} + +function isPromise (obj) { + return isObject(obj) && typeof obj.then === 'function' +} + +function isObject (obj) { + return typeof obj === 'object' && obj !== null +} + +module.exports = tx diff --git a/test/plugins/ioredis.spec.js b/test/plugins/ioredis.spec.js index 23eca7316a8..9a17d15c6f6 100644 --- a/test/plugins/ioredis.spec.js +++ b/test/plugins/ioredis.spec.js @@ -39,6 +39,7 @@ describe('Plugin', () => { expect(traces[0][0].meta).to.have.property('span.kind', 'client') expect(traces[0][0].meta).to.have.property('out.host', 'localhost') expect(traces[0][0].meta).to.have.property('out.port', '6379') + expect(traces[0][0].meta).to.have.property('redis.raw_command', 'GET foo') }) .then(done) .catch(done) diff --git a/test/plugins/redis.spec.js b/test/plugins/redis.spec.js index da20c3b6fc1..bf8fde3dec4 100644 --- a/test/plugins/redis.spec.js +++ b/test/plugins/redis.spec.js @@ -48,6 +48,7 @@ describe('Plugin', () => { expect(traces[0][0].meta).to.have.property('span.kind', 'client') expect(traces[0][0].meta).to.have.property('out.host', '127.0.0.1') expect(traces[0][0].meta).to.have.property('out.port', '6379') + expect(traces[0][0].meta).to.have.property('redis.raw_command', 'GET foo') }) .then(done) .catch(done) diff --git a/test/plugins/util/redis.spec.js b/test/plugins/util/redis.spec.js new file mode 100644 index 00000000000..e578b2471dd --- /dev/null +++ b/test/plugins/util/redis.spec.js @@ -0,0 +1,82 @@ +'use strict' + +describe('plugins/util/redis', () => { + let redis + let tracer + let config + let span + + beforeEach(() => { + config = {} + tracer = require('../../..').init({ service: 'test', plugins: false }) + redis = require('../../../src/plugins/util/redis') + }) + + describe('instrument', () => { + it('should start a span with the correct tags', () => { + span = redis.instrument(tracer, config, '1', 'set', ['foo', 'bar']) + + expect(span.context().tags).to.deep.include({ + 'span.kind': 'client', + 'service.name': 'test-redis', + 'resource.name': 'set', + 'span.type': 'redis', + 'db.type': 'redis', + 'db.name': '1', + 'out.host': '127.0.0.1', + 'out.port': '6379', + 'redis.raw_command': 'SET foo bar' + }) + }) + + it('should use the parent from the scope', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const parent = tracer.startSpan('parent') + + tracer.scopeManager().activate(parent) + + span = redis.instrument(tracer, config, '1', 'ping', []) + + expect(span.context().parentId.toString()).to.equal(parent.context().spanId.toString()) + }) + + it('should trim command arguments if yoo long', () => { + let key = '' + + for (let i = 0; i <= 100; i++) { + key += 'a' + } + + span = redis.instrument(tracer, config, '1', 'get', [key]) + + const rawCommand = span.context().tags['redis.raw_command'] + + expect(rawCommand).to.have.length(104) + expect(rawCommand.substr(0, 10)).to.equal('GET aaaaaa') + expect(rawCommand.substr(94)).to.equal('aaaaaaa...') + }) + + it('should trim the command if too long', () => { + const values = [] + + for (let i = 0; i < 10; i++) { + let value = '' + + for (let i = 0; i < 100; i++) { + value += 'a' + } + + values.push(value) + } + + span = redis.instrument(tracer, config, '1', 'get', values) + + const rawCommand = span.context().tags['redis.raw_command'] + + expect(rawCommand).to.have.length(1000) + expect(rawCommand.substr(0, 10)).to.equal('GET aaaaaa') + expect(rawCommand.substr(990)).to.equal('aaaaaaa...') + }) + }) +}) diff --git a/test/plugins/util/tx.spec.js b/test/plugins/util/tx.spec.js new file mode 100644 index 00000000000..2d79c38e619 --- /dev/null +++ b/test/plugins/util/tx.spec.js @@ -0,0 +1,81 @@ +'use strict' + +describe('plugins/util/tx', () => { + let tx + let tracer + let span + + beforeEach(() => { + tracer = require('../../..').init({ plugins: false }) + span = tracer.startSpan('test') + tx = require('../../../src/plugins/util/tx') + + sinon.spy(span, 'finish') + }) + + afterEach(() => { + span.finish.restore() + }) + + describe('setHost', () => { + it('should set the out.host and out.port tags', () => { + tx.setHost(span, 'example.com', '1234') + + expect(span.context().tags).to.have.property('out.host', 'example.com') + expect(span.context().tags).to.have.property('out.port', '1234') + }) + }) + + describe('wrap', () => { + describe('with a callback', () => { + it('should return a wrapper that finishes the span', () => { + const callback = sinon.spy() + const wrapper = tx.wrap(span, callback) + + wrapper(null, 'foo', 'bar') + + expect(callback).to.have.been.calledWith(null, 'foo', 'bar') + expect(span.finish).to.have.been.called + }) + + it('should return a wrapper that sets error tags', () => { + const callback = sinon.spy() + const error = new Error('boom') + const wrapper = tx.wrap(span, callback) + + wrapper(error) + + expect(span.context().tags).to.have.property('error.msg', error.message) + expect(span.context().tags).to.have.property('error.type', error.name) + expect(span.context().tags).to.have.property('error.stack', error.stack) + }) + }) + + describe('with a promise', () => { + it('should finish the span when the promise is resolved', () => { + const promise = Promise.resolve('value') + + tx.wrap(span, promise) + + return promise.then(value => { + expect(value).to.equal('value') + expect(span.finish).to.have.been.called + }) + }) + + it('should set the error tags when the promise is rejected', () => { + const error = new Error('boom') + const promise = Promise.reject(error) + + tx.wrap(span, promise) + + return promise.catch(err => { + expect(err).to.equal(error) + expect(span.context().tags).to.have.property('error.msg', error.message) + expect(span.context().tags).to.have.property('error.type', error.name) + expect(span.context().tags).to.have.property('error.stack', error.stack) + }) + }) + }) + }) +}) From 718d399ef28fa2d3fe0f13e900e97b0b0f2c4703 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 7 Nov 2018 07:05:19 -0500 Subject: [PATCH 21/30] fix wrong name for memcached.command tag (#357) --- src/plugins/memcached.js | 2 +- test/plugins/memcached.spec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/plugins/memcached.js b/src/plugins/memcached.js index 1aa05fb540f..9f830f426d0 100644 --- a/src/plugins/memcached.js +++ b/src/plugins/memcached.js @@ -27,7 +27,7 @@ function wrapQueryCompiler (original, client, server, span) { span.addTags({ 'resource.name': query.type, - 'memcached.query': query.command + 'memcached.command': query.command }) addHost(span, client, server, query) diff --git a/test/plugins/memcached.spec.js b/test/plugins/memcached.spec.js index a740f2dbb1f..64ab38a06f9 100644 --- a/test/plugins/memcached.spec.js +++ b/test/plugins/memcached.spec.js @@ -37,7 +37,7 @@ describe('Plugin', () => { expect(traces[0][0].meta).to.have.property('span.kind', 'client') expect(traces[0][0].meta).to.have.property('out.host', 'localhost') expect(traces[0][0].meta).to.have.property('out.port', '11211') - expect(traces[0][0].meta).to.have.property('memcached.query', 'get test') + expect(traces[0][0].meta).to.have.property('memcached.command', 'get test') }) .then(done) .catch(done) From 67dcd584e05892f3dfea8c2b95d5ae6b920f87f7 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Wed, 7 Nov 2018 12:58:39 -0500 Subject: [PATCH 22/30] use http.route in http hooks for the resource name if not provided (#360) --- src/opentracing/tracer.js | 4 +--- src/plugins/util/web.js | 26 ++++++++++++++++++-------- test/opentracing/tracer.spec.js | 3 +-- test/plugins/util/web.spec.js | 24 +++++++++++++----------- 4 files changed, 33 insertions(+), 24 deletions(-) diff --git a/src/opentracing/tracer.js b/src/opentracing/tracer.js index 35849192100..49bea92a0db 100644 --- a/src/opentracing/tracer.js +++ b/src/opentracing/tracer.js @@ -42,11 +42,9 @@ class DatadogTracer extends Tracer { const references = getReferences(fields.references) const parent = getParent(references) const tags = { - 'resource.name': name + 'service.name': this._service } - tags['service.name'] = this._service - if (this._env) { tags.env = this._env } diff --git a/src/plugins/util/web.js b/src/plugins/util/web.js index 0816f3069cc..e9508514470 100644 --- a/src/plugins/util/web.js +++ b/src/plugins/util/web.js @@ -121,6 +121,9 @@ function finish (req, res) { addResponseTags(req) req._datadog.hooks.forEach(hooks => hooks.request(req._datadog.span, req, res)) + + addResourceTag(req) + req._datadog.span.finish() req._datadog.scope && req._datadog.scope.close() req._datadog.finished = true @@ -158,26 +161,33 @@ function addRequestTags (req) { function addResponseTags (req) { const span = req._datadog.span - const tags = span.context().tags const res = req._datadog.res - if (!tags.hasOwnProperty(HTTP_ROUTE) && req._datadog.paths.length > 0) { + if (req._datadog.paths.length > 0) { span.setTag(HTTP_ROUTE, req._datadog.paths.join('')) } - const resource = [req.method] - .concat(tags[HTTP_ROUTE]) - .filter(val => val) - .join(' ') - span.addTags({ - [RESOURCE_NAME]: resource, [HTTP_STATUS_CODE]: res.statusCode }) addStatusError(req) } +function addResourceTag (req) { + const span = req._datadog.span + const tags = span.context().tags + + if (tags['resource.name']) return + + const resource = [req.method] + .concat(tags[HTTP_ROUTE]) + .filter(val => val) + .join(' ') + + span.setTag(RESOURCE_NAME, resource) +} + function addHeaders (req) { const span = req._datadog.span diff --git a/test/opentracing/tracer.spec.js b/test/opentracing/tracer.spec.js index b3276900f2b..7814f5fcce6 100644 --- a/test/opentracing/tracer.spec.js +++ b/test/opentracing/tracer.spec.js @@ -130,8 +130,7 @@ describe('Tracer', () => { parent: null, tags: { 'foo': 'bar', - 'service.name': 'service', - 'resource.name': 'name' + 'service.name': 'service' }, startTime: fields.startTime }) diff --git a/test/plugins/util/web.spec.js b/test/plugins/util/web.spec.js index cb0b92ae2fe..e77238d18ab 100644 --- a/test/plugins/util/web.spec.js +++ b/test/plugins/util/web.spec.js @@ -28,6 +28,7 @@ describe('plugins/util/web', () => { beforeEach(() => { req = { + method: 'GET', headers: { 'host': 'localhost' }, @@ -236,6 +237,18 @@ describe('plugins/util/web', () => { expect(config.hooks.request).to.have.been.calledWith(span, req, res) }) + + it('should set the resource name from the http.route tag set in the hooks', () => { + config.hooks = { + request: span => span.setTag('http.route', '/custom/route') + } + + span = web.instrument(tracer, config, req, res, 'test.request') + + res.end() + + expect(span.context().tags).to.have.property('resource.name', 'GET /custom/route') + }) }) }) @@ -255,17 +268,6 @@ describe('plugins/util/web', () => { expect(span.context().tags).to.have.property(RESOURCE_NAME, 'GET /foo/bar') expect(span.context().tags).to.have.property(HTTP_ROUTE, '/foo/bar') }) - - it('should not override user provided routes', () => { - req.method = 'GET' - span.setTag('http.route', '/custom') - - web.enterRoute(req, '/foo') - res.end() - - expect(span.context().tags).to.have.property(RESOURCE_NAME, 'GET /custom') - expect(span.context().tags).to.have.property(HTTP_ROUTE, '/custom') - }) }) describe('exitRoute', () => { From 1e9b79fe93add95f9a1a7eda26182f6bd5b78775 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Thu, 8 Nov 2018 13:05:05 -0500 Subject: [PATCH 23/30] fix calling web.instrument multiple times not updating metadata (#361) --- src/plugins/express.js | 2 -- src/plugins/koa.js | 2 -- src/plugins/util/web.js | 10 +++++----- test/plugins/util/web.spec.js | 8 ++++++++ 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/src/plugins/express.js b/src/plugins/express.js index 79010f0bfc0..fab639e2c36 100644 --- a/src/plugins/express.js +++ b/src/plugins/express.js @@ -8,8 +8,6 @@ function createWrapMethod (tracer, config) { config = web.normalizeConfig(config) function ddTrace (req, res, next) { - if (web.active(req)) return next() - web.instrument(tracer, config, req, res, 'express.request') next() diff --git a/src/plugins/koa.js b/src/plugins/koa.js index d05650a6951..b8c3b887f4a 100644 --- a/src/plugins/koa.js +++ b/src/plugins/koa.js @@ -6,8 +6,6 @@ function createWrapUse (tracer, config) { config = web.normalizeConfig(config) function ddTrace (ctx, next) { - if (web.active(ctx.req)) return next() - web.instrument(tracer, config, ctx.req, ctx.res, 'koa.request') return next() diff --git a/src/plugins/util/web.js b/src/plugins/util/web.js index e9508514470..4d050a77fd7 100644 --- a/src/plugins/util/web.js +++ b/src/plugins/util/web.js @@ -80,7 +80,6 @@ const web = { span: null, scope: null, paths: [], - hooks: [], beforeEnd: [] } }) @@ -93,7 +92,7 @@ const web = { } function startSpan (tracer, config, req, res, name) { - req._datadog.hooks.push(config.hooks) + req._datadog.config = config if (req._datadog.span) { req._datadog.span.context().name = name @@ -105,7 +104,6 @@ function startSpan (tracer, config, req, res, name) { const scope = tracer.scopeManager().activate(span) req._datadog.tracer = tracer - req._datadog.config = config req._datadog.span = span req._datadog.scope = scope req._datadog.res = res @@ -120,7 +118,7 @@ function finish (req, res) { addResponseTags(req) - req._datadog.hooks.forEach(hooks => hooks.request(req._datadog.span, req, res)) + req._datadog.config.hooks.request(req._datadog.span, req, res) addResourceTag(req) @@ -133,7 +131,9 @@ function wrapEnd (req) { const res = req._datadog.res const end = res.end - res.end = function () { + if (end === req._datadog.end) return + + req._datadog.end = res.end = function () { req._datadog.beforeEnd.forEach(beforeEnd => beforeEnd()) const returnValue = end.apply(this, arguments) diff --git a/test/plugins/util/web.spec.js b/test/plugins/util/web.spec.js index e77238d18ab..462c13518d7 100644 --- a/test/plugins/util/web.spec.js +++ b/test/plugins/util/web.spec.js @@ -132,6 +132,14 @@ describe('plugins/util/web', () => { expect(span.context().tags).to.have.property('service.name', 'test2') }) + + it('should only wrap res.end once', () => { + span = web.instrument(tracer, config, req, res, 'test.request') + const end = res.end + span = web.instrument(tracer, config, req, res, 'test.request') + + expect(end).to.equal(res.end) + }) }) describe('on request end', () => { From bdb993bf09e7212528e1e6b56d990991a5c107cd Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Mon, 12 Nov 2018 13:22:15 -0500 Subject: [PATCH 24/30] add environment variable to set the agent globally (#373) --- src/config.js | 7 ++++++- test/config.spec.js | 9 +++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/config.js b/src/config.js index 79c38be156d..0cc4f714def 100644 --- a/src/config.js +++ b/src/config.js @@ -12,7 +12,12 @@ class Config { const debug = coalesce(options.debug, platform.env('DD_TRACE_DEBUG'), false) const env = coalesce(options.env, platform.env('DD_ENV')) const protocol = 'http' - const hostname = coalesce(options.hostname, platform.env('DD_TRACE_AGENT_HOSTNAME'), 'localhost') + const hostname = coalesce( + options.hostname, + platform.env('DD_AGENT_HOST'), + platform.env('DD_TRACE_AGENT_HOSTNAME'), + 'localhost' + ) const port = coalesce(options.port, platform.env('DD_TRACE_AGENT_PORT'), 8126) const sampleRate = coalesce(Math.min(Math.max(options.sampleRate, 0), 1), 1) const flushInterval = coalesce(parseInt(options.flushInterval, 10), 2000) diff --git a/test/config.spec.js b/test/config.spec.js index fdbdcc8488e..db0a567fde4 100644 --- a/test/config.spec.js +++ b/test/config.spec.js @@ -85,6 +85,15 @@ describe('Config', () => { expect(config).to.have.property('plugins', false) }) + it('should give priority to the common agent environment variable', () => { + platform.env.withArgs('DD_TRACE_AGENT_HOSTNAME').returns('trace-agent') + platform.env.withArgs('DD_AGENT_HOST').returns('agent') + + const config = new Config() + + expect(config).to.have.nested.property('url.hostname', 'agent') + }) + it('should give priority to the options', () => { platform.env.withArgs('DD_TRACE_AGENT_HOSTNAME').returns('agent') platform.env.withArgs('DD_TRACE_AGENT_PORT').returns('6218') From 52b89371871b8ffe52fe1014e1ce19d091933625 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Mon, 12 Nov 2018 15:13:16 -0500 Subject: [PATCH 25/30] add support for bluebird (#372) --- src/plugins/bluebird.js | 16 +++++ src/plugins/index.js | 1 + src/plugins/util/promise.js | 30 +++++++++ src/scope/scope_manager.js | 6 +- test/plugins/bluebird.spec.js | 104 +++++++++++++++++++++++++++++++ test/scope/scope_manager.spec.js | 9 +++ 6 files changed, 164 insertions(+), 2 deletions(-) create mode 100644 src/plugins/bluebird.js create mode 100644 src/plugins/util/promise.js create mode 100644 test/plugins/bluebird.spec.js diff --git a/src/plugins/bluebird.js b/src/plugins/bluebird.js new file mode 100644 index 00000000000..defae254547 --- /dev/null +++ b/src/plugins/bluebird.js @@ -0,0 +1,16 @@ +'use strict' + +const tx = require('./util/promise') + +module.exports = [ + { + name: 'bluebird', + versions: ['2.0.2 - 3'], // 2.0.0 and 2.0.1 were removed from npm + patch (Promise, tracer, config) { + this.wrap(Promise.prototype, '_then', tx.createWrapThen(tracer, config)) + }, + unpatch (Promise) { + this.unwrap(Promise.prototype, '_then') + } + } +] diff --git a/src/plugins/index.js b/src/plugins/index.js index 965b798810a..218d292b22f 100644 --- a/src/plugins/index.js +++ b/src/plugins/index.js @@ -3,6 +3,7 @@ module.exports = { 'amqp10': require('./amqp10'), 'amqplib': require('./amqplib'), + 'bluebird': require('./bluebird'), 'elasticsearch': require('./elasticsearch'), 'express': require('./express'), 'graphql': require('./graphql'), diff --git a/src/plugins/util/promise.js b/src/plugins/util/promise.js new file mode 100644 index 00000000000..da221dc03b5 --- /dev/null +++ b/src/plugins/util/promise.js @@ -0,0 +1,30 @@ +'use strict' + +module.exports = { + createWrapThen (tracer, config) { + return function wrapThen (then) { + return function thenWithTrace (onFulfilled, onRejected, onProgress) { + arguments[0] = wrapCallback(tracer, onFulfilled) + arguments[1] = wrapCallback(tracer, onRejected) + + // not standard but sometimes supported + if (onProgress) { + arguments[2] = wrapCallback(tracer, onProgress) + } + + return then.apply(this, arguments) + } + } + } +} + +function wrapCallback (tracer, callback) { + if (typeof callback !== 'function') return callback + + const scope = tracer.scopeManager().active() + + return function () { + tracer.scopeManager().activate(scope ? scope.span() : null) + return callback.apply(this, arguments) + } +} diff --git a/src/scope/scope_manager.js b/src/scope/scope_manager.js index 0082bca1e14..706e555704c 100644 --- a/src/scope/scope_manager.js +++ b/src/scope/scope_manager.js @@ -49,8 +49,10 @@ class ScopeManager { let execution = this._active while (execution !== null) { - if (execution.scope()) { - return execution.scope() + const scope = execution.scope() + + if (scope) { + return scope.span() ? scope : null } execution = execution.parent() diff --git a/test/plugins/bluebird.spec.js b/test/plugins/bluebird.spec.js new file mode 100644 index 00000000000..25624ad56c1 --- /dev/null +++ b/test/plugins/bluebird.spec.js @@ -0,0 +1,104 @@ +'use strict' + +const agent = require('./agent') +const plugin = require('../../src/plugins/bluebird') + +wrapIt() + +describe('Plugin', () => { + let Promise + let tracer + + describe('bluebird', () => { + withVersions(plugin, 'bluebird', version => { + beforeEach(() => { + tracer = require('../..') + }) + + afterEach(() => { + return agent.close() + }) + + describe('without configuration', () => { + beforeEach(() => { + return agent.load(plugin, 'bluebird') + .then(() => { + Promise = require(`../../versions/bluebird@${version}`).get() + }) + }) + + it('should run the then() callback in context where then() was called', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const span = {} + const promise = new Promise((resolve, reject) => { + setImmediate(() => { + tracer.scopeManager().activate({}) + resolve() + }) + }) + + tracer.scopeManager().activate(span) + + return promise + .then(() => { + tracer.scopeManager().activate({}) + }) + .then(() => { + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) + }) + }) + + it('should run the catch() callback in context where catch() was called', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const span = {} + const promise = new Promise((resolve, reject) => { + setImmediate(() => { + tracer.scopeManager().activate({}) + reject(new Error()) + }) + }) + + tracer.scopeManager().activate(span) + + return promise + .catch(err => { + tracer.scopeManager().activate({}) + throw err + }) + .catch(() => { + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) + }) + }) + + it('should allow to run without a scope if not available when calling then()', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + tracer.scopeManager().activate(null) + + const promise = new Promise((resolve, reject) => { + setImmediate(() => { + tracer.scopeManager().activate({}) + resolve() + }) + }) + + return promise + .then(() => { + tracer.scopeManager().activate({}) + }) + .then(() => { + expect(tracer.scopeManager().active()).to.be.null + }) + }) + }) + }) + }) +}) diff --git a/test/scope/scope_manager.spec.js b/test/scope/scope_manager.spec.js index f3803ae8ef6..3913ccd76ab 100644 --- a/test/scope/scope_manager.spec.js +++ b/test/scope/scope_manager.spec.js @@ -241,4 +241,13 @@ describe('ScopeManager', () => { asyncHooks.before(1) }).not.to.throw() }) + + it('should allow deactivating a scope', () => { + const span = {} + + scopeManager.activate(span) + scopeManager.activate(null) + + expect(scopeManager.active()).to.be.null + }) }) From 80aed2bea3cb85fc8bf177ba586204865337e678 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Tue, 13 Nov 2018 17:10:09 -0500 Subject: [PATCH 26/30] add support for Q (#375) --- src/plugins/index.js | 1 + src/plugins/q.js | 16 ++++++++ test/plugins/q.spec.js | 85 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+) create mode 100644 src/plugins/q.js create mode 100644 test/plugins/q.spec.js diff --git a/src/plugins/index.js b/src/plugins/index.js index 218d292b22f..2957b013046 100644 --- a/src/plugins/index.js +++ b/src/plugins/index.js @@ -16,6 +16,7 @@ module.exports = { 'mysql': require('./mysql'), 'mysql2': require('./mysql2'), 'pg': require('./pg'), + 'q': require('./q'), 'redis': require('./redis'), 'restify': require('./restify') } diff --git a/src/plugins/q.js b/src/plugins/q.js new file mode 100644 index 00000000000..1a05ae5f82d --- /dev/null +++ b/src/plugins/q.js @@ -0,0 +1,16 @@ +'use strict' + +const tx = require('./util/promise') + +module.exports = [ + { + name: 'q', + versions: ['1'], + patch (Q, tracer, config) { + this.wrap(Q.makePromise.prototype, 'then', tx.createWrapThen(tracer, config)) + }, + unpatch (Q) { + this.unwrap(Q.makePromise.prototype, 'then') + } + } +] diff --git a/test/plugins/q.spec.js b/test/plugins/q.spec.js new file mode 100644 index 00000000000..b29dc55d8e9 --- /dev/null +++ b/test/plugins/q.spec.js @@ -0,0 +1,85 @@ +'use strict' + +const agent = require('./agent') +const plugin = require('../../src/plugins/q') + +wrapIt() + +describe('Plugin', () => { + let Q + let tracer + + describe('q', () => { + withVersions(plugin, 'q', version => { + beforeEach(() => { + tracer = require('../..') + }) + + afterEach(() => { + return agent.close() + }) + + describe('without configuration', () => { + beforeEach(() => { + return agent.load(plugin, 'q') + .then(() => { + Q = require(`../../versions/q@${version}`).get() + }) + }) + + it('should run the then() callback in context where then() was called', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const span = {} + const deferred = Q.defer() + const promise = deferred.promise + + setImmediate(() => { + tracer.scopeManager().activate({}) + deferred.resolve() + }) + + tracer.scopeManager().activate(span) + + return promise + .then(() => { + tracer.scopeManager().activate({}) + }) + .then(() => { + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) + }) + }) + + it('should run the catch() callback in context where catch() was called', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const span = {} + const deferred = Q.defer() + const promise = deferred.promise + + setImmediate(() => { + tracer.scopeManager().activate({}) + deferred.reject(new Error()) + }) + + tracer.scopeManager().activate(span) + + return promise + .catch(err => { + tracer.scopeManager().activate({}) + throw err + }) + .catch(() => { + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) + }) + }) + }) + }) + }) +}) From 6c35df02d9099ca1ded7ccddcabc6931d435d5e0 Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Tue, 13 Nov 2018 17:10:35 -0500 Subject: [PATCH 27/30] add support for when (#376) --- src/plugins/index.js | 3 +- src/plugins/when.js | 17 ++++++ test/plugins/when.spec.js | 111 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 src/plugins/when.js create mode 100644 test/plugins/when.spec.js diff --git a/src/plugins/index.js b/src/plugins/index.js index 2957b013046..da63a6ee439 100644 --- a/src/plugins/index.js +++ b/src/plugins/index.js @@ -18,5 +18,6 @@ module.exports = { 'pg': require('./pg'), 'q': require('./q'), 'redis': require('./redis'), - 'restify': require('./restify') + 'restify': require('./restify'), + 'when': require('./when') } diff --git a/src/plugins/when.js b/src/plugins/when.js new file mode 100644 index 00000000000..73d3a080bfb --- /dev/null +++ b/src/plugins/when.js @@ -0,0 +1,17 @@ +'use strict' + +const tx = require('./util/promise') + +module.exports = [ + { + name: 'when', + file: 'lib/Promise.js', + versions: ['3'], + patch (Promise, tracer, config) { + this.wrap(Promise.prototype, 'then', tx.createWrapThen(tracer, config)) + }, + unpatch (Promise) { + this.unwrap(Promise.prototype, 'then') + } + } +] diff --git a/test/plugins/when.spec.js b/test/plugins/when.spec.js new file mode 100644 index 00000000000..a34ce7e00c3 --- /dev/null +++ b/test/plugins/when.spec.js @@ -0,0 +1,111 @@ +'use strict' + +const agent = require('./agent') +const plugin = require('../../src/plugins/when') + +wrapIt() + +describe('Plugin', () => { + let when + let tracer + + describe('when', () => { + withVersions(plugin, 'when', version => { + beforeEach(() => { + tracer = require('../..') + }) + + afterEach(() => { + return agent.close() + }) + + describe('without configuration', () => { + beforeEach(() => { + return agent.load(plugin, 'when') + .then(() => { + when = require(`../../versions/when@${version}`).get() + }) + }) + + it('should run the then() callback in context where then() was called', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const span = {} + const deferred = when.defer() + const promise = deferred.promise + + setImmediate(() => { + tracer.scopeManager().activate({}) + deferred.resolve() + }) + + tracer.scopeManager().activate(span) + + return promise + .then(() => { + tracer.scopeManager().activate({}) + }) + .then(() => { + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) + }) + }) + + it('should run the catch() callback in context where catch() was called', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const span = {} + const deferred = when.defer() + const promise = deferred.promise + + setImmediate(() => { + tracer.scopeManager().activate({}) + deferred.reject(new Error()) + }) + + tracer.scopeManager().activate(span) + + return promise + .catch(err => { + tracer.scopeManager().activate({}) + throw err + }) + .catch(() => { + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) + }) + }) + + it('should run the onProgress callback in context where then() was called', () => { + if (process.env.DD_CONTEXT_PROPAGATION === 'false') return + + const span = {} + const deferred = when.defer() + const promise = deferred.promise + + setImmediate(() => { + tracer.scopeManager().activate({}) + deferred.resolve() + }) + + tracer.scopeManager().activate(span) + + return promise + .then(() => { + tracer.scopeManager().activate({}) + }) + .then(() => {}, () => {}, () => { + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) + }) + }) + }) + }) + }) +}) From 4659b9446aa3f62bbfcaa2a27f24fbadecbc2c6e Mon Sep 17 00:00:00 2001 From: Roch Devost Date: Tue, 13 Nov 2018 17:11:29 -0500 Subject: [PATCH 28/30] add support for ioredis 2 - 3 (#377) --- src/plugins/ioredis.js | 2 +- test/plugins/agent.js | 1 + test/plugins/ioredis.spec.js | 9 +++++++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/plugins/ioredis.js b/src/plugins/ioredis.js index 1738cab90e6..c093bcf0704 100644 --- a/src/plugins/ioredis.js +++ b/src/plugins/ioredis.js @@ -18,7 +18,7 @@ function createWrapSendCommand (tracer, config) { module.exports = { name: 'ioredis', - versions: ['4.x'], + versions: ['>=2 <=4'], patch (Redis, tracer, config) { this.wrap(Redis.prototype, 'sendCommand', createWrapSendCommand(tracer, config)) }, diff --git a/test/plugins/agent.js b/test/plugins/agent.js index 82283d78521..1bc479c3ed8 100644 --- a/test/plugins/agent.js +++ b/test/plugins/agent.js @@ -49,6 +49,7 @@ module.exports = { plugins: false }) + tracer.use('bluebird') tracer.use(pluginName, config) }) }) diff --git a/test/plugins/ioredis.spec.js b/test/plugins/ioredis.spec.js index 9a17d15c6f6..c13204fa6ea 100644 --- a/test/plugins/ioredis.spec.js +++ b/test/plugins/ioredis.spec.js @@ -50,11 +50,16 @@ describe('Plugin', () => { it('should run the callback in the parent context', () => { if (process.env.DD_CONTEXT_PROPAGATION === 'false') return - const scope = tracer.scopeManager().activate({}) + const span = {} + + tracer.scopeManager().activate(span) return redis.get('foo') .then(() => { - expect(tracer.scopeManager().active()).to.equal(scope) + const scope = tracer.scopeManager().active() + + expect(scope).to.not.be.null + expect(scope.span()).to.equal(span) }) }) From a53e2589ab47a7870776f084b9b7c49a48ebc31b Mon Sep 17 00:00:00 2001 From: rochdev Date: Thu, 15 Nov 2018 15:27:18 -0500 Subject: [PATCH 29/30] split build further --- .circleci/config.yml | 124 +++++++++++++++++++++++++++++++---- test/leak/plugins/amqplib.js | 4 +- 2 files changed, 114 insertions(+), 14 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index a41ef3a6da0..7e799b87da1 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -97,6 +97,11 @@ jobs: docker: - image: node:8 + node-core-10: + <<: *node-core-base + docker: + - image: node:10 + node-core-latest: <<: *node-core-base docker: @@ -145,6 +150,14 @@ jobs: - QPIDD_ADMIN_USERNAME=admin - QPIDD_ADMIN_PASSWORD=admin + node-bluebird: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES= + - PLUGINS=bluebird + node-elasticsearch: <<: *node-plugin-base docker: @@ -157,6 +170,14 @@ jobs: - discovery.type=single-node - "ES_JAVA_OPTS=-Xms64m -Xmx64m" + node-express: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES= + - PLUGINS=express + node-graphql: <<: *node-plugin-base docker: @@ -165,6 +186,14 @@ jobs: - SERVICES= - PLUGINS=graphql + node-hapi: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES= + - PLUGINS=hapi + node-http: <<: *node-plugin-base docker: @@ -173,6 +202,23 @@ jobs: - SERVICES= - PLUGINS=http + node-ioredis: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES=redis + - PLUGINS=ioredis + - image: redis:4.0-alpine + + node-koa: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES= + - PLUGINS=koa + node-memcached: <<: *node-plugin-base docker: @@ -188,13 +234,25 @@ jobs: - image: node:8 environment: - SERVICES=mysql - - PLUGINS=mysql|mysql2 + - PLUGINS=mysql + - image: mysql:5.7 + environment: + - MYSQL_ALLOW_EMPTY_PASSWORD=yes + - MYSQL_DATABASE=db + + node-mysql2: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES=mysql + - PLUGINS=mysql2 - image: mysql:5.7 environment: - MYSQL_ALLOW_EMPTY_PASSWORD=yes - MYSQL_DATABASE=db - node-mongo: + node-mongodb-core: <<: *node-plugin-base docker: - image: node:8 @@ -203,7 +261,7 @@ jobs: - PLUGINS=mongodb-core - image: mongo:3.6 - node-postgres: + node-pg: <<: *node-plugin-base docker: - image: node:8 @@ -212,22 +270,46 @@ jobs: - PLUGINS=pg - image: postgres:9.5 + node-q: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES= + - PLUGINS=q + node-redis: <<: *node-plugin-base docker: - image: node:8 environment: - SERVICES=redis - - PLUGINS=redis|ioredis + - PLUGINS=redis - image: redis:4.0-alpine - node-web: + node-restify: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES= + - PLUGINS=restify + + node-router: + <<: *node-plugin-base + docker: + - image: node:8 + environment: + - SERVICES= + - PLUGINS=router + + node-when: <<: *node-plugin-base docker: - image: node:8 environment: - SERVICES= - - PLUGINS=express|hapi|koa|restify + - PLUGINS=when workflows: version: 2 @@ -238,18 +320,27 @@ workflows: - node-core-4 - node-core-6 - node-core-8 + - node-core-10 - node-core-latest - node-amqplib - node-amqp10 + - node-bluebird - node-elasticsearch + - node-express - node-graphql + - node-hapi - node-http + - node-ioredis + - node-koa - node-memcached - - node-mongo + - node-mongodb-core - node-mysql - - node-postgres + - node-mysql2 + - node-pg + - node-q - node-redis - - node-web + - node-restify + - node-when nightly: triggers: - schedule: @@ -262,15 +353,24 @@ workflows: - node-core-4 - node-core-6 - node-core-8 + - node-core-10 - node-core-latest - node-amqplib - node-amqp10 + - node-bluebird - node-elasticsearch + - node-express - node-graphql + - node-hapi - node-http + - node-ioredis + - node-koa - node-memcached - - node-mongo + - node-mongodb-core - node-mysql - - node-postgres + - node-mysql2 + - node-pg + - node-q - node-redis - - node-web + - node-restify + - node-when diff --git a/test/leak/plugins/amqplib.js b/test/leak/plugins/amqplib.js index 4fdb5fbfbc5..e71bec41cf9 100644 --- a/test/leak/plugins/amqplib.js +++ b/test/leak/plugins/amqplib.js @@ -15,7 +15,7 @@ test('amqplib plugin should not leak when using callbacks', t => { conn.createChannel((err, ch) => { if (err) return t.fail(err) - profile(t, operation).then(() => conn.close()) + profile(t, operation, 400).then(() => conn.close()) function operation (done) { ch.assertQueue('test', {}, done) @@ -29,7 +29,7 @@ test('amqplib plugin should not leak when using promises', t => { .then(conn => { return conn.createChannel() .then(ch => { - profile(t, operation).then(() => conn.close()) + profile(t, operation, 400).then(() => conn.close()) function operation (done) { ch.assertQueue('test', {}).then(done) From b8be798d7457ef49f6c29efee299262ab27de0e8 Mon Sep 17 00:00:00 2001 From: rochdev Date: Thu, 15 Nov 2018 15:49:45 -0500 Subject: [PATCH 30/30] fix build --- .circleci/config.yml | 35 +++++------------------------------ package.json | 2 +- 2 files changed, 6 insertions(+), 31 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 7e799b87da1..9a635a43eac 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -202,15 +202,6 @@ jobs: - SERVICES= - PLUGINS=http - node-ioredis: - <<: *node-plugin-base - docker: - - image: node:8 - environment: - - SERVICES=redis - - PLUGINS=ioredis - - image: redis:4.0-alpine - node-koa: <<: *node-plugin-base docker: @@ -234,19 +225,7 @@ jobs: - image: node:8 environment: - SERVICES=mysql - - PLUGINS=mysql - - image: mysql:5.7 - environment: - - MYSQL_ALLOW_EMPTY_PASSWORD=yes - - MYSQL_DATABASE=db - - node-mysql2: - <<: *node-plugin-base - docker: - - image: node:8 - environment: - - SERVICES=mysql - - PLUGINS=mysql2 + - PLUGINS=mysql|mysql2 - image: mysql:5.7 environment: - MYSQL_ALLOW_EMPTY_PASSWORD=yes @@ -261,7 +240,7 @@ jobs: - PLUGINS=mongodb-core - image: mongo:3.6 - node-pg: + node-postgres: <<: *node-plugin-base docker: - image: node:8 @@ -284,7 +263,7 @@ jobs: - image: node:8 environment: - SERVICES=redis - - PLUGINS=redis + - PLUGINS=redis|ioredis - image: redis:4.0-alpine node-restify: @@ -330,13 +309,11 @@ workflows: - node-graphql - node-hapi - node-http - - node-ioredis - node-koa - node-memcached - node-mongodb-core - node-mysql - - node-mysql2 - - node-pg + - node-postgres - node-q - node-redis - node-restify @@ -363,13 +340,11 @@ workflows: - node-graphql - node-hapi - node-http - - node-ioredis - node-koa - node-memcached - node-mongodb-core - node-mysql - - node-mysql2 - - node-pg + - node-postgres - node-q - node-redis - node-restify diff --git a/package.json b/package.json index de394d82024..30b28ae1403 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,7 @@ "tdd": "yarn services && mocha --watch 'test/setup/**/*.js'", "test": "yarn services && cov8 --include \"src/**/*.js\" -- mocha --exit 'test/setup/all.js' 'test/**/*.spec.js'", "test:core": "mocha --exit --exclude \"test/plugins/**/*.spec.js\" --file test/setup/core.js \"test/**/*.spec.js\"", - "test:plugins": "yarn services && cov8 --include \"src/**/*.js\" -- mocha --exit --file \"test/setup/all.js\" \"test/plugins/@($(echo $PLUGINS)).spec.js\"", + "test:plugins": "yarn services && cov8 --include \"src/**/*.js\" -- mocha --exit --file \"test/setup/all.js\" \"test/plugins/@($(echo $PLUGINS)).spec.js\" \"test/plugins/@($(echo $PLUGINS))/**/*.spec.js\"", "leak:core": "node ./scripts/install_plugin_modules && (cd test/leak && yarn) && NODE_PATH=./test/leak/node_modules node --no-warnings ./node_modules/.bin/tape 'test/leak/{,!(node_modules|plugins)/**/}/*.js'", "leak:plugins": "yarn services && (cd test/leak && yarn) && NODE_PATH=./test/leak/node_modules node --no-warnings ./node_modules/.bin/tape \"test/leak/plugins/@($(echo $PLUGINS)).js\"" },