From 47b798ce05ca2c547750e2944fe11d0a4e23b1df Mon Sep 17 00:00:00 2001 From: Bob Evans Date: Wed, 1 May 2024 15:23:26 -0400 Subject: [PATCH 1/2] refactor: Updated instrumentation for http, undici, grpc to use a new `segment.captureExternalAttributes` to centralize the necessary data needed to create segment and span attributes --- lib/instrumentation/core/http-outbound.js | 73 +++++++------- lib/instrumentation/grpc-js/grpc.js | 17 ++-- lib/instrumentation/undici.js | 29 ++++-- lib/spans/span-event.js | 6 +- lib/spans/streaming-span-event.js | 96 +++++++++---------- lib/transaction/trace/segment.js | 79 +++++++-------- .../instrumentation/http/outbound.test.js | 2 +- test/unit/metric/datastore-instance.test.js | 58 ----------- test/unit/spans/span-event.test.js | 2 +- test/unit/spans/streaming-span-event.test.js | 36 +++++-- test/versioned/grpc/util.cjs | 8 +- test/versioned/undici/requests.tap.js | 2 +- 12 files changed, 192 insertions(+), 216 deletions(-) diff --git a/lib/instrumentation/core/http-outbound.js b/lib/instrumentation/core/http-outbound.js index 217418c0c6..c1d6d2d52b 100644 --- a/lib/instrumentation/core/http-outbound.js +++ b/lib/instrumentation/core/http-outbound.js @@ -17,8 +17,6 @@ const http = require('http') const synthetics = require('../../synthetics') const NAMES = require('../../metrics/names') -const { DESTINATIONS } = require('../../config/attribute-filter') - const DEFAULT_HOST = 'localhost' const DEFAULT_HTTP_PORT = 80 const DEFAULT_SSL_PORT = 443 @@ -78,6 +76,23 @@ function parseOpts(opts) { return opts } +/** + * Extracts host, hostname, port from http request options + * + * @param {object} opts HTTP request options + * @returns {object} { host, hostname, port } + */ +function extractHostPort(opts) { + const defaultPort = getDefaultPort(opts) + const hostname = getDefaultHostName(opts) + const port = getPort(opts, defaultPort) + let host = hostname + if (port && port !== defaultPort) { + host += `:${port}` + } + return { host, hostname, port } +} + /** * Instruments an outbound HTTP request. * @@ -88,21 +103,14 @@ function parseOpts(opts) { */ module.exports = function instrumentOutbound(agent, opts, makeRequest) { opts = parseOpts(opts) - const defaultPort = getDefaultPort(opts) - const host = getDefaultHostName(opts) - const port = getPort(opts, defaultPort) + const { host, hostname, port } = extractHostPort(opts) - if (!host || port < 1) { - logger.warn('Invalid host name (%s) or port (%s) for outbound request.', host, port) + if (!hostname || port < 1) { + logger.warn('Invalid host name (%s) or port (%s) for outbound request.', hostname, port) return makeRequest(opts) } - let hostname = host - if (port && port !== defaultPort) { - hostname += ':' + port - } - - const name = NAMES.EXTERNAL.PREFIX + hostname + const name = NAMES.EXTERNAL.PREFIX + host const parent = agent.tracer.getSegment() if (parent && parent.opaque) { @@ -117,7 +125,7 @@ module.exports = function instrumentOutbound(agent, opts, makeRequest) { return agent.tracer.addSegment( name, - recordExternal(hostname, 'http'), + recordExternal(host, 'http'), parent, false, instrumentRequest.bind(null, { agent, opts, makeRequest, host, port, hostname }) @@ -133,8 +141,8 @@ module.exports = function instrumentOutbound(agent, opts, makeRequest) { * @param {Agent} params.agent New Relic agent * @param {string|object} params.opts a url string or HTTP request options * @param {Function} params.makeRequest function to make request - * @param {string} params.hostname host + port of outbound request - * @param {string} params.host host of outbound request + * @param {string} params.host domain + port of outbound request + * @param {string} params.hostname domain of outbound request * @param {string} params.port port of outbound request * @param {TraceSegment} segment outbound http segment * @returns {http.IncomingMessage} request actual http outbound request @@ -151,7 +159,7 @@ function instrumentRequest({ agent, opts, makeRequest, host, port, hostname }, s const request = applySegment({ opts, makeRequest, hostname, host, port, segment }) - instrumentRequestEmit(agent, hostname, segment, request) + instrumentRequestEmit(agent, host, segment, request) return request } @@ -226,19 +234,16 @@ function applySegment({ opts, makeRequest, host, port, hostname, segment }) { parsed.path = urltils.obfuscatePath(segment.transaction.agent.config, parsed.path) const proto = parsed.protocol || opts.protocol || 'http:' segment.name += parsed.path + segment.captureExternalAttributes({ + protocol: proto, + hostname, + host, + method: opts.method, + port, + path: parsed.path, + queryParams: parsed.parameters + }) request[symbols.segment] = segment - - if (parsed.parameters) { - // Scrub and parse returns on object with a null prototype. - // eslint-disable-next-line guard-for-in - for (const key in parsed.parameters) { - segment.addSpanAttribute(`request.parameters.${key}`, parsed.parameters[key]) - } - } - segment.attributes.addAttribute(DESTINATIONS.SPAN_EVENT, 'host', host) - segment.attributes.addAttribute(DESTINATIONS.SPAN_EVENT, 'port', port) - segment.addAttribute('url', `${proto}//${hostname}${parsed.path}`) - segment.addAttribute('procedure', opts.method || 'GET') return request } @@ -249,10 +254,11 @@ function applySegment({ opts, makeRequest, host, port, hostname, segment }) { * * @param {Agent} agent New Relic agent * @param {string} hostname host of outbound request + * @param host * @param {TraceSegment} segment outbound http segment * @param {http.IncomingMessage} request actual http outbound request */ -function instrumentRequestEmit(agent, hostname, segment, request) { +function instrumentRequestEmit(agent, host, segment, request) { shimmer.wrapMethod(request, 'request.emit', 'emit', function wrapEmit(emit) { const boundEmit = agent.tracer.bindFunction(emit, segment) return function wrappedRequestEmit(evnt, arg) { @@ -260,7 +266,7 @@ function instrumentRequestEmit(agent, hostname, segment, request) { segment.end() handleError(segment, request, arg) } else if (evnt === 'response') { - handleResponse(segment, hostname, arg) + handleResponse(segment, host, arg) } return boundEmit.apply(this, arguments) @@ -296,9 +302,10 @@ function handleError(segment, req, error) { * * @param {object} segment TraceSegment instance * @param {string} hostname host of the HTTP request + * @param host * @param {object} res http.ServerResponse */ -function handleResponse(segment, hostname, res) { +function handleResponse(segment, host, res) { // Add response attributes for spans segment.addSpanAttribute('http.statusCode', res.statusCode) segment.addSpanAttribute('http.statusText', res.statusMessage) @@ -308,7 +315,7 @@ function handleResponse(segment, hostname, res) { if (agent.config.cross_application_tracer.enabled && !agent.config.distributed_tracing.enabled) { const { appData } = cat.extractCatHeaders(res.headers) const decodedAppData = cat.parseAppData(agent.config, appData) - cat.assignCatToSegment(decodedAppData, segment, hostname) + cat.assignCatToSegment(decodedAppData, segment, host) } // Again a custom emit wrapper because we want to watch for the `end` event. diff --git a/lib/instrumentation/grpc-js/grpc.js b/lib/instrumentation/grpc-js/grpc.js index e27a07d6be..438facefec 100644 --- a/lib/instrumentation/grpc-js/grpc.js +++ b/lib/instrumentation/grpc-js/grpc.js @@ -98,12 +98,17 @@ function wrapStart(shim, original) { segment.addAttribute('component', 'gRPC') - const protocol = 'grpc' - - const url = `${protocol}://${authorityName}${method}` - - segment.addAttribute('http.url', url) - segment.addAttribute('http.method', method) + const protocol = 'grpc:' + const [hostname, port] = authorityName.split(':') + + segment.captureExternalAttributes({ + protocol, + host: authorityName, + port, + hostname, + method, + path: method + }) if (originalListener && originalListener.onReceiveStatus) { const onReceiveStatuts = shim.bindSegment(originalListener.onReceiveStatus, segment) diff --git a/lib/instrumentation/undici.js b/lib/instrumentation/undici.js index 496d652279..3ed47eafee 100644 --- a/lib/instrumentation/undici.js +++ b/lib/instrumentation/undici.js @@ -13,7 +13,7 @@ const symbols = require('../symbols') const { executionAsyncResource } = require('async_hooks') const diagnosticsChannel = require('diagnostics_channel') const synthetics = require('../synthetics') -const { DESTINATIONS } = require('../config/attribute-filter') +const urltils = require('../util/urltils') const channels = [ { channel: diagnosticsChannel.channel('undici:request:create'), hook: requestCreateHook }, @@ -116,22 +116,33 @@ function addDTHeaders({ transaction, config, request }) { */ function createExternalSegment({ shim, request, parentSegment }) { const url = new URL(request.origin + request.path) - const name = NAMES.EXTERNAL.PREFIX + url.host + url.pathname + const obfuscatedPath = urltils.obfuscatePath(shim.agent.config, url.pathname) + const name = NAMES.EXTERNAL.PREFIX + url.host + obfuscatedPath // Metrics for `External/` will have a suffix of undici // We will have to see if this matters for people only using fetch // It's undici under the hood so ¯\_(ツ)_/¯ const segment = shim.createSegment(name, recordExternal(url.host, 'undici'), parentSegment) + + // brown-fields alert!! + // instrumentation expects query params to be an object. url.searchParams is an Iterable + // convert to object + const queryParams = {} + url.searchParams.forEach((value, key) => { + queryParams[key] = value + }) if (segment) { segment.start() shim.setActiveSegment(segment) - segment.addAttribute('url', `${url.protocol}//${url.host}${url.pathname}`) - - url.searchParams.forEach((value, key) => { - segment.addSpanAttribute(`request.parameters.${key}`, value) + segment.captureExternalAttributes({ + protocol: url.protocol, + hostname: url.hostname, + host: url.host, + method: request.method, + port: url.port, + path: obfuscatedPath, + queryParams }) - segment.attributes.addAttribute(DESTINATIONS.SPAN_EVENT, 'host', url.hostname) - segment.attributes.addAttribute(DESTINATIONS.SPAN_EVENT, 'port', url.port) - segment.addAttribute('procedure', request.method || 'GET') + request[symbols.segment] = segment } } diff --git a/lib/spans/span-event.js b/lib/spans/span-event.js index 6970a09099..d366339fae 100644 --- a/lib/spans/span-event.js +++ b/lib/spans/span-event.js @@ -194,9 +194,9 @@ class HttpSpanEvent extends SpanEvent { attributes.url = null } - if (attributes.host) { - this.addAttribute('server.address', attributes.host) - attributes.host = null + if (attributes.hostname) { + this.addAttribute('server.address', attributes.hostname) + attributes.hostname = null } if (attributes.port) { diff --git a/lib/spans/streaming-span-event.js b/lib/spans/streaming-span-event.js index c89a271eef..adc717b3ec 100644 --- a/lib/spans/streaming-span-event.js +++ b/lib/spans/streaming-span-event.js @@ -182,35 +182,30 @@ class StreamingHttpSpanEvent extends StreamingSpanEvent { * Must be pre-filtered and truncated. */ constructor(traceId, agentAttributes, customAttributes) { - super(traceId, agentAttributes, customAttributes) + // remove mapped attributes before creating other agentAttributes + const { library, url, hostname, port, procedure, ...agentAttrs } = agentAttributes + super(traceId, agentAttrs, customAttributes) this.addIntrinsicAttribute('category', CATEGORIES.HTTP) - this.addIntrinsicAttribute('component', agentAttributes.library || HTTP_LIBRARY) + this.addIntrinsicAttribute('component', library || HTTP_LIBRARY) this.addIntrinsicAttribute('span.kind', CLIENT_KIND) - if (agentAttributes.library) { - agentAttributes.library = null + if (url) { + this.addAgentAttribute('http.url', url) } - if (agentAttributes.url) { - this.addAgentAttribute('http.url', agentAttributes.url) - agentAttributes.url = null + if (hostname) { + this.addAgentAttribute('server.address', hostname) + agentAttributes.hostname = null } - if (agentAttributes.host) { - this.addAgentAttribute('server.address', agentAttributes.host) - agentAttributes.host = null + if (port) { + this.addAgentAttribute('server.port', port, true) } - if (agentAttributes.port) { - this.addAgentAttribute('server.port', agentAttributes.port, true) - agentAttributes.port = null - } - - if (agentAttributes.procedure) { - this.addAgentAttribute('http.method', agentAttributes.procedure) - this.addAgentAttribute('http.request.method', agentAttributes.procedure) - agentAttributes.procedure = null + if (procedure) { + this.addAgentAttribute('http.method', procedure) + this.addAgentAttribute('http.request.method', procedure) } } @@ -235,56 +230,61 @@ class StreamingDatastoreSpanEvent extends StreamingSpanEvent { * @param {object} customAttributes Initial set of custom attributes. * Must be pre-filtered and truncated. */ + /* eslint-disable camelcase */ constructor(traceId, agentAttributes, customAttributes) { - super(traceId, agentAttributes, customAttributes) + // remove mapped attributes before creating other agentAttributes + const { + product, + collection, + sql, + sql_obfuscated, + database_name, + host, + port_path_or_id, + ...agentAttrs + } = agentAttributes + super(traceId, agentAttrs, customAttributes) this.addIntrinsicAttribute('category', CATEGORIES.DATASTORE) this.addIntrinsicAttribute('span.kind', CLIENT_KIND) - if (agentAttributes.product) { - this.addIntrinsicAttribute('component', agentAttributes.product) - this.addAgentAttribute('db.system', agentAttributes.product) - agentAttributes.product = null + if (product) { + this.addIntrinsicAttribute('component', product) + this.addAgentAttribute('db.system', product) } - if (agentAttributes.collection) { - this.addAgentAttribute('db.collection', agentAttributes.collection) - agentAttributes.collection = null + if (collection) { + this.addAgentAttribute('db.collection', collection) } - if (agentAttributes.sql || agentAttributes.sql_obfuscated) { - let sql = null - if (agentAttributes.sql_obfuscated) { - sql = _truncate(agentAttributes.sql_obfuscated) - agentAttributes.sql_obfuscated = null - } else if (agentAttributes.sql) { - sql = _truncate(agentAttributes.sql) - agentAttributes.sql = null + if (sql || sql_obfuscated) { + let finalSql = null + if (sql_obfuscated) { + finalSql = _truncate(agentAttributes.sql_obfuscated) + } else if (sql) { + finalSql = _truncate(agentAttributes.sql) } // Flag as exempt from normal attribute truncation - this.addAgentAttribute('db.statement', sql, true) + this.addAgentAttribute('db.statement', finalSql, true) } - if (agentAttributes.database_name) { - this.addAgentAttribute('db.instance', agentAttributes.database_name) - agentAttributes.database_name = null + if (database_name) { + this.addAgentAttribute('db.instance', database_name) } - if (agentAttributes.host) { - this.addAgentAttribute('peer.hostname', agentAttributes.host) - this.addAgentAttribute('server.address', agentAttributes.host) + if (host) { + this.addAgentAttribute('peer.hostname', host) + this.addAgentAttribute('server.address', host) - if (agentAttributes.port_path_or_id) { - const address = `${agentAttributes.host}:${agentAttributes.port_path_or_id}` + if (port_path_or_id) { + const address = `${host}:${port_path_or_id}` this.addAgentAttribute('peer.address', address) - this.addAgentAttribute('server.port', agentAttributes.port_path_or_id, true) - agentAttributes.port_path_or_id = null + this.addAgentAttribute('server.port', port_path_or_id, true) } - - agentAttributes.host = null } } + /* eslint-enable camelcase */ static isDatastoreSegment(segment) { return DATASTORE_REGEX.test(segment.name) diff --git a/lib/transaction/trace/segment.js b/lib/transaction/trace/segment.js index 3d95c7412d..2fee25fb98 100644 --- a/lib/transaction/trace/segment.js +++ b/lib/transaction/trace/segment.js @@ -8,7 +8,6 @@ const { DESTINATIONS } = require('../../config/attribute-filter') const logger = require('../../logger').child({ component: 'segment' }) const Timer = require('../../timer') -const urltils = require('../../util/urltils') const hashes = require('../../util/hashes') const { Attributes } = require('../../attributes') @@ -16,7 +15,6 @@ const ExclusiveCalculator = require('./exclusive-time-calculator') const SpanContext = require('../../spans/span-context') const NAMES = require('../../metrics/names') -const INSTANCE_UNKNOWN = 'unknown' const STATE = { EXTERNAL: 'EXTERNAL', CALLBACK: 'CALLBACK' @@ -119,47 +117,6 @@ TraceSegment.prototype.getSpanId = function getSpanId() { return null } -/** - * @param {string} host - * The name of the host of the database. This will be normalized if the string - * represents localhost. - * @param {string|number} port - * The database's port, path to unix socket, or id. - * @param {string|number|bool} database - * The name or ID of the database that was connected to. Or `false` if there is - * no database name (i.e. Redis has no databases, only hosts). - */ -TraceSegment.prototype.captureDBInstanceAttributes = captureDBInstanceAttributes - -function captureDBInstanceAttributes(host, port, database) { - const config = this.transaction.agent.config - const dsTracerConf = config.datastore_tracer - - // Add database name if provided and enabled. - if (database !== false && dsTracerConf.database_name_reporting.enabled) { - this.addAttribute( - 'database_name', - typeof database === 'number' ? database : database || INSTANCE_UNKNOWN - ) - } - - // Add instance information if enabled. - if (dsTracerConf.instance_reporting.enabled) { - // Determine appropriate defaults for host and port. - port = port || INSTANCE_UNKNOWN - if (host && urltils.isLocalhost(host)) { - host = config.getHostnameSafe(host) - } - if (!host || host === 'UNKNOWN_BOX') { - // Config's default name of a host. - host = INSTANCE_UNKNOWN - } - - this.addAttribute('host', host) - this.addAttribute('port_path_or_id', String(port)) - } -} - TraceSegment.prototype.moveToCallbackState = function moveToCallbackState() { this.state = STATE.CALLBACK } @@ -468,4 +425,40 @@ TraceSegment.prototype.toJSON = function toJSON() { return resultDest[0] } +/** + * Adds all the relevant segment attributes for an External http request + * + * Note: for hostname, port, and `request.parameters.*` they are added as span attributes + * only since these will get assigned when constructing SpanEvents from segment. + * + * @param {object} params function params + * @param {string} params.protocol protocol of request(i.e. `http:`, `grpc:`) + * @param {string} params.hostname hostname of request(no port) + * @param {string} params.host host of request(hostname + port) + * @param [string] params.port port of request if applicable + * @param {string} params.path uri of request + * @param {string} [params.method] method of request + * @param {object} [params.queryParams] query parameters of request + * @param params.port + */ +TraceSegment.prototype.captureExternalAttributes = function captureExternalAttributes({ + protocol, + hostname, + host, + port, + path, + method = 'GET', + queryParams = {} +}) { + // eslint-disable-next-line guard-for-in + for (const key in queryParams) { + this.addSpanAttribute(`request.parameters.${key}`, queryParams[key]) + } + + this.addSpanAttribute('hostname', hostname) + this.addSpanAttribute('port', port) + this.addAttribute('url', `${protocol}//${host}${path}`) + this.addAttribute('procedure', method) +} + module.exports = TraceSegment diff --git a/test/unit/instrumentation/http/outbound.test.js b/test/unit/instrumentation/http/outbound.test.js index bd9a2e468c..3c66e9db55 100644 --- a/test/unit/instrumentation/http/outbound.test.js +++ b/test/unit/instrumentation/http/outbound.test.js @@ -127,7 +127,7 @@ tap.test('instrumentOutbound', (t) => { t.same( transaction.trace.root.children[0].attributes.get(DESTINATIONS.SPAN_EVENT), { - 'host': HOSTNAME, + 'hostname': HOSTNAME, 'port': PORT, 'url': `http://${HOSTNAME}:${PORT}/asdf`, 'procedure': 'GET', diff --git a/test/unit/metric/datastore-instance.test.js b/test/unit/metric/datastore-instance.test.js index e7f891eb23..314965358c 100644 --- a/test/unit/metric/datastore-instance.test.js +++ b/test/unit/metric/datastore-instance.test.js @@ -9,7 +9,6 @@ const tap = require('tap') const helper = require('../../lib/agent_helper') const DatastoreShim = require('../../../lib/shim/datastore-shim') -const ParsedStatement = require('../../../lib/db/parsed-statement') const tests = require('../../lib/cross_agent_tests/datastores/datastore_instances') tap.test('Datastore instance metrics collected via the datastore shim', function (t) { @@ -72,63 +71,6 @@ tap.test('Datastore instance metrics collected via the datastore shim', function }) }) -tap.test('Datastore instance metrics captured through the segment', function (t) { - t.autoend() - t.beforeEach(function (t) { - t.context.agent = helper.loadMockedAgent() - }) - - t.afterEach(function (t) { - const { agent } = t.context - if (agent) { - helper.unloadAgent(agent) - } - }) - - tests.forEach(function (test) { - t.test(test.name, function (t) { - const { agent } = t.context - agent.config.getHostnameSafe = function () { - return test.system_hostname - } - - helper.runInTransaction(agent, function (tx) { - const ps = new ParsedStatement(test.product, 'SELECT', 'bar') - const child = tx.trace.root.add('test segment', ps.recordMetrics.bind(ps)) - - // Each instrumentation must make the following checks when pulling - // instance attributes from their respective drivers. - - // If we don't have a host name specified, but are connecting over the - // file system using either a domain socket or a path to the db file - // then the database host is localhost. - let dbHost = test.db_hostname - if (!dbHost && (test.unix_socket || test.database_path)) { - dbHost = 'localhost' - } - - // If any value is provided for a path or port, it must be used. - // Otherwise use 'default'. - let port = 'default' - if ( - test.hasOwnProperty('unix_socket') || - test.hasOwnProperty('database_path') || - test.hasOwnProperty('port') - ) { - port = test.unix_socket || test.database_path || test.port - } - - child.captureDBInstanceAttributes(dbHost, port, 'foo') - child.touch() - - tx.end() - t.ok(getMetrics(agent).unscoped[test.expected_instance_metric]) - t.end() - }) - }) - }) -}) - function getMetrics(agent) { return agent.metrics._metrics } diff --git a/test/unit/spans/span-event.test.js b/test/unit/spans/span-event.test.js index 4d3d166945..1e6dc9079a 100644 --- a/test/unit/spans/span-event.test.js +++ b/test/unit/spans/span-event.test.js @@ -175,7 +175,7 @@ tap.test('fromSegment()', (t) => { // should nullify mapped properties t.notOk(attributes.library) t.notOk(attributes.url) - t.notOk(attributes.host) + t.notOk(attributes.hostname) t.notOk(attributes.port) t.notOk(attributes.procedure) diff --git a/test/unit/spans/streaming-span-event.test.js b/test/unit/spans/streaming-span-event.test.js index 5f7cf9f604..b21629e602 100644 --- a/test/unit/spans/streaming-span-event.test.js +++ b/test/unit/spans/streaming-span-event.test.js @@ -156,6 +156,7 @@ tap.test('fromSegment()', (t) => { t.ok(agentAttributes) // Should have (most) http properties. + t.same(agentAttributes['request.parameters.foo'], { [STRING_TYPE]: 'bar' }) t.same(agentAttributes['http.url'], { [STRING_TYPE]: 'https://example.com/' }) t.same(agentAttributes['server.address'], { [STRING_TYPE]: 'example.com' }) t.same(agentAttributes['server.port'], { [INT_TYPE]: 443 }) @@ -164,14 +165,19 @@ tap.test('fromSegment()', (t) => { t.same(agentAttributes['http.statusCode'], { [INT_TYPE]: 200 }) t.same(agentAttributes['http.statusText'], { [STRING_TYPE]: 'OK' }) - // Should have no datastore properties. const hasOwnAttribute = Object.hasOwnProperty.bind(agentAttributes) - t.notOk(hasOwnAttribute('db.statement')) - t.notOk(hasOwnAttribute('db.instance')) - t.notOk(hasOwnAttribute('db.system')) - t.notOk(hasOwnAttribute('peer.hostname')) - t.notOk(hasOwnAttribute('peer.address')) + // should remove mapped attributes + ;['library', 'url', 'hostname', 'port', 'procedure'].forEach((attr) => { + t.notOk(hasOwnAttribute(attr)) + }) + + // Should have no datastore properties. + ;['db.statement', 'db.instance', 'db.system', 'peer.hostname', 'peer.address'].forEach( + (attr) => { + t.notOk(hasOwnAttribute(attr)) + } + ) t.end() }) }) @@ -250,10 +256,22 @@ tap.test('fromSegment()', (t) => { // Should have not http properties. const hasOwnAttribute = Object.hasOwnProperty.bind(agentAttributes) - t.notOk(hasOwnAttribute('http.url')) - t.notOk(hasOwnAttribute('http.method')) - t.notOk(hasOwnAttribute('http.request.method')) + ;['http.url', 'http.method', 'http.request.method'].forEach((attr) => { + t.notOk(hasOwnAttribute(attr)) + }) + // Should removed map attributes + ;[ + 'product', + 'collection', + 'sql', + 'sql_obfuscated', + 'database_name', + 'host', + 'port_path_or_id' + ].forEach((attr) => { + t.notOk(hasOwnAttribute(attr)) + }) // Should have (most) datastore properties. t.ok(agentAttributes['db.instance']) t.same(agentAttributes['db.collection'], { [STRING_TYPE]: 'my-collection' }) diff --git a/test/versioned/grpc/util.cjs b/test/versioned/grpc/util.cjs index 9da0391b63..1a743da252 100644 --- a/test/versioned/grpc/util.cjs +++ b/test/versioned/grpc/util.cjs @@ -122,8 +122,8 @@ util.getServerTransactionName = function getRPCName(fnName) { } /** - * Asserts the gRPC external segment and its relevant attributes: http.url, - * http.method, grpc.statusCode, grpc.statusText + * Asserts the gRPC external segment and its relevant attributes: url, + * procedure, grpc.statusCode, grpc.statusText * * @param {Object} params * @param {Object} params.t tap test @@ -146,11 +146,11 @@ util.assertExternalSegment = function assertExternalSegment({ const segment = metricsHelpers.findSegment(tx.trace.root, segmentName) const attributes = segment.getAttributes() t.equal( - attributes['http.url'], + attributes.url, `grpc://${CLIENT_ADDR}:${port}${methodName}`, 'http.url attribute should be correct' ) - t.equal(attributes['http.method'], methodName, 'method name should be correct') + t.equal(attributes.procedure, methodName, 'method name should be correct') t.equal( attributes['grpc.statusCode'], expectedStatusCode, diff --git a/test/versioned/undici/requests.tap.js b/test/versioned/undici/requests.tap.js index 6b23a0c2e0..08458e210b 100644 --- a/test/versioned/undici/requests.tap.js +++ b/test/versioned/undici/requests.tap.js @@ -148,7 +148,7 @@ tap.test('Undici request tests', (t) => { t.equal(spanAttrs['http.statusText'], 'OK') t.equal(spanAttrs['request.parameters.a'], 'b') t.equal(spanAttrs['request.parameters.c'], 'd') - t.equal(spanAttrs.host, 'localhost') + t.equal(spanAttrs.hostname, 'localhost') t.equal(spanAttrs.port, `${PORT}`) tx.end() t.end() From e5238ec5850b095b87a7bdfee444cebacfd737e6 Mon Sep 17 00:00:00 2001 From: Bob Evans Date: Thu, 2 May 2024 08:52:19 -0400 Subject: [PATCH 2/2] chore: fixed comment and method of converting query params to an object in undici --- lib/instrumentation/undici.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/instrumentation/undici.js b/lib/instrumentation/undici.js index 3ed47eafee..cda18b0942 100644 --- a/lib/instrumentation/undici.js +++ b/lib/instrumentation/undici.js @@ -123,13 +123,10 @@ function createExternalSegment({ shim, request, parentSegment }) { // It's undici under the hood so ¯\_(ツ)_/¯ const segment = shim.createSegment(name, recordExternal(url.host, 'undici'), parentSegment) - // brown-fields alert!! - // instrumentation expects query params to be an object. url.searchParams is an Iterable - // convert to object - const queryParams = {} - url.searchParams.forEach((value, key) => { - queryParams[key] = value - }) + // the captureExternalAttributes expects queryParams to be an object, do conversion + // to object see: https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams + const queryParams = Object.fromEntries(url.searchParams.entries()) + if (segment) { segment.start() shim.setActiveSegment(segment)