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 (#2179)

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,18 +254,19 @@ 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) {
       if (evnt === 'error') {
         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.

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)

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,30 @@ 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/<host>` 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)
+
+  // 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)
-    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
   }
 }

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) {

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)