diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 307cf8053e..8d1a986c19 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -62,6 +62,9 @@ variable), then it should be removed. [float] ===== Bug fixes +* Fixed bug where the URL property for outgoing HTTP request spans was set + with the server's IP address rather than its hostname. The Agent now sets + this property with the actual URL requested by Node.js. * Fixed bug where external services were not listed under Dependencies on the APM Service Overview page due to the trace-context propagated `sample_rate` value not being set on either transactions or spans. diff --git a/lib/instrumentation/http-shared.js b/lib/instrumentation/http-shared.js index 8c75a32cca..8d1923e1ae 100644 --- a/lib/instrumentation/http-shared.js +++ b/lib/instrumentation/http-shared.js @@ -1,9 +1,7 @@ 'use strict' var url = require('url') - var endOfStream = require('end-of-stream') -var httpRequestToUrl = require('http-request-to-url') var { parseUrl } = require('../parsers') var { getHTTPDestination } = require('./context') @@ -191,13 +189,11 @@ exports.traceOutgoingRequest = function (agent, moduleName, method) { return emit.apply(req, arguments) } - let url, statusCode - httpRequestToUrl(req).then(_url => { - url = _url - }).catch(() => { + const url = getUrlFromRequestAndOptions(req, options, moduleName + ':') + if (!url) { agent.logger.warn('unable to identify http.ClientRequest url %o', { id: id }) - }) - + } + let statusCode return req // In case the request is ended prematurely @@ -260,3 +256,41 @@ function isAWSSigned (opts) { const auth = opts.headers && (opts.headers.Authorization || opts.headers.authorization) return typeof auth === 'string' ? auth.startsWith('AWS4-') : false } + +// Creates a sanitized URL suitable for the span's HTTP context +// +// This function reconstructs a URL using the request object's properties +// where it can (node versions v14.5.0, v12.19.0 and later) , and falling +// back to the options where it can not. This function also strips any +// authentication information provided with the hostname. In other words +// +// http://username:password@example.com/foo +// +// becomes http://example.com/foo +// +// NOTE: The options argument may not be the same options that are passed +// to http.request if the caller uses the the http.request(url,options,...) +// method signature. The agent normalizes the url and options into a single +// options array. This function expects those pre-normalized options. +// +// @param {ClientRequest} req +// @param {object} options +// @param {string} fallbackProtocol +// @return string|undefined +function getUrlFromRequestAndOptions (req, options, fallbackProtocol) { + if (!req) { + return undefined + } + options = options || {} + req = req || {} + req.agent = req.agent || {} + + const port = options.port ? `:${options.port}` : '' + // req.host and req.protocol are node versions v14.5.0/v12.19.0 and later + const host = req.host || options.hostname || options.host || 'localhost' + const protocol = req.protocol || req.agent.protocol || fallbackProtocol + + return `${protocol}//${host}${port}${req.path}` +} + +exports.getUrlFromRequestAndOptions = getUrlFromRequestAndOptions diff --git a/package.json b/package.json index bf803434ac..8e735cba98 100644 --- a/package.json +++ b/package.json @@ -93,7 +93,6 @@ "escape-string-regexp": "^4.0.0", "fast-safe-stringify": "^2.0.7", "http-headers": "^3.0.2", - "http-request-to-url": "^1.0.0", "is-native": "^1.0.1", "measured-reporting": "^1.51.1", "monitor-event-loop-delay": "^1.0.0", diff --git a/test/instrumentation/modules/http/outgoing.js b/test/instrumentation/modules/http/outgoing.js index 1ddc0da2d3..b2171bc5fd 100644 --- a/test/instrumentation/modules/http/outgoing.js +++ b/test/instrumentation/modules/http/outgoing.js @@ -172,15 +172,15 @@ function echoTest (type, opts, handler) { t.deepEqual(data.spans[0].context.http, { method: 'GET', status_code: 200, - url: `${type}://127.0.0.1:${port}/` + url: `${type}://localhost:${port}/` }) t.deepEqual(data.spans[0].context.destination, { service: { - name: `${type}://127.0.0.1:${port}`, - resource: `127.0.0.1:${port}`, + name: `${type}://localhost:${port}`, + resource: `localhost:${port}`, type: data.spans[0].type }, - address: '127.0.0.1', + address: 'localhost', port: Number(port) }) t.end() @@ -224,7 +224,7 @@ function abortTest (type, handler) { const httpContext = { method: 'GET', status_code: undefined, - url: undefined + url: `http://localhost:${port}/` } resetAgent({}, data => { @@ -241,11 +241,11 @@ function abortTest (type, handler) { if (httpContext.url) { t.deepEqual(data.spans[0].context.destination, { service: { - name: `${type}://127.0.0.1:${port}`, - resource: `127.0.0.1:${port}`, + name: `${type}://localhost:${port}`, + resource: `localhost:${port}`, type: data.spans[0].type }, - address: '127.0.0.1', + address: 'localhost', port: Number(port) }) } @@ -262,7 +262,7 @@ function abortTest (type, handler) { req.on('response', () => { httpContext.status_code = 200 - httpContext.url = `${type}://127.0.0.1:${port}/` + httpContext.url = `${type}://localhost:${port}/` }) // NOTE: Don't use an arrow function here diff --git a/test/instrumentation/modules/http/request-to-url.js b/test/instrumentation/modules/http/request-to-url.js new file mode 100644 index 0000000000..422e16d720 --- /dev/null +++ b/test/instrumentation/modules/http/request-to-url.js @@ -0,0 +1,136 @@ +const tape = require('tape') +const http = require('http') +const { getUrlFromRequestAndOptions } = require('../../../../lib/instrumentation/http-shared') + +// Creates a ClientRequest from options +// +// Creates and request an immediatly aborts/destroys it. +// This allows us to test with real ClientRequest objects +// and ensure their underlying properties are stable/consistant +// across versions. +// +// @param {options} options +// @return {ClientRequest} +function requestFromOptions (options) { + const req = http.request(options) + req.on('error', function () {}) + req.destroy() + return req +} + +tape('getUrlFromRequestAndOptions tests', function (suite) { + suite.test('options with host', function (t) { + const options = { + host: 'example.com' + } + const req = requestFromOptions(options) + + const url = getUrlFromRequestAndOptions(req, options) + t.equals(url, 'http://example.com/', 'url rendered as expected') + t.end() + }) + + suite.test('options with host and path', function (t) { + const options = { + host: 'example.com', + path: '/foo' + } + const req = requestFromOptions(options) + + const url = getUrlFromRequestAndOptions(req, options) + t.equals(url, 'http://example.com/foo', 'url rendered as expected') + t.end() + }) + + suite.test('options with host, path, port, and a query string', function (t) { + const options = { + host: 'example.com', + path: '/foo?fpp=bar', + port: 32 + } + const req = requestFromOptions(options) + + const url = getUrlFromRequestAndOptions(req, options) + t.equals(url, 'http://example.com:32/foo?fpp=bar', 'url rendered as expected') + t.end() + }) + + suite.test('options with host, path, port, query string, and a username/password', function (t) { + const options = { + host: 'example.com', + path: '/foo?fpp=bar', + auth: 'username:password', + port: 32 + } + const req = requestFromOptions(options) + + const url = getUrlFromRequestAndOptions(req, options) + t.equals(url, 'http://example.com:32/foo?fpp=bar', 'url rendered as expected') + t.equals(url.indexOf('username'), -1, 'no auth information in url') + t.equals(url.indexOf('password'), -1, 'no auth information in url') + t.end() + }) + + suite.test('options with host and hostname', function (t) { + const options = { + host: 'two.example.com', + hostname: 'one.example.com', + path: '/bar', + auth: 'username:password' + } + const req = requestFromOptions(options) + const url = getUrlFromRequestAndOptions(req, options) + t.equals(url, 'http://one.example.com/bar', 'url rendered as expected (hostname wins)') + t.equals(url.indexOf('username'), -1, 'no auth information in url') + t.equals(url.indexOf('password'), -1, 'no auth information in url') + t.end() + }) + + suite.test('does not crash with unexpected data', function (t) { + const options = { + host: 'two.example.com', + hostname: 'one.example.com', + path: '/bar' + } + const req = requestFromOptions(options) + + const url1 = getUrlFromRequestAndOptions(null, null) + const url2 = getUrlFromRequestAndOptions(req, null) + const url3 = getUrlFromRequestAndOptions(null, options) + + t.equal(url1, undefined, 'no url returned') + t.ok(url2, 'URL returned') + t.equal(url3, undefined, 'no url returned') + t.end() + }) + + suite.test('port 80 makes it through', function (t) { + const options = { + host: 'two.example.com', + port: 80 + } + const req = requestFromOptions(options) + + const url = getUrlFromRequestAndOptions(req, options) + t.equals(url, 'http://two.example.com:80/', 'port 80 made it thorugh') + t.end() + }) + + suite.test('missing protocol', function (t) { + const options = { + hostname: 'localhost', + path: '/get', + // A custom agent that implements the minimum to pass muster, but does + // *not* define `agent.protocol`. + agent: { + addRequest () {} + } + } + const req = requestFromOptions(options) + + const url = getUrlFromRequestAndOptions(req, options, 'http:') + t.equals(url, 'http://localhost/get', 'protocol falls back correctly') + t.end() + }) + suite.end() +})