feat: Moved entity.guid, entity.name, entity.type, and `hostnam…

…e` to `common.attributes` on logs payload instead of in every log message (newrelic#2736)

lib/agent.js

@@ -821,6 +821,8 @@ Agent.prototype._listenForConfigChanges = function _listenForConfigChanges() {
  * This method returns an object with the following keys/data:
  * - `trace.id`: The current trace ID
  * - `span.id`: The current span ID
+ *
+ * If `excludeServiceLinks` is false it will also include the following:
  * - `entity.name`: The application name specified in the connect request as
  *   app_name. If multiple application names are specified this will only be
  *   the first name
@@ -830,17 +832,14 @@ Agent.prototype._listenForConfigChanges = function _listenForConfigChanges() {
  *   utilization.full_hostname. If utilization.full_hostname is null or empty,
  *   this will be the hostname specified in the connect request as host.
  *
+ * @param {boolean} excludeServiceLinks flag to indicate if you should include `entity.guid`, `entity.type`, `entity.name` and `hostname` from linking metadata
  * @returns {object} The LinkingMetadata object with the data above
  */
-Agent.prototype.getLinkingMetadata = function getLinkingMetadata() {
+Agent.prototype.getLinkingMetadata = function getLinkingMetadata(excludeServiceLinks = false) {
   const segment = this.tracer.getSegment()
   const config = this.config
 
-  const linkingMetadata = {
-    'entity.name': config.applications()[0],
-    'entity.type': 'SERVICE',
-    'hostname': config.getHostnameSafe()
-  }
+  const linkingMetadata = {}
 
   if (config.distributed_tracing.enabled && segment) {
     linkingMetadata['trace.id'] = segment.transaction.traceId
@@ -852,10 +851,37 @@ Agent.prototype.getLinkingMetadata = function getLinkingMetadata() {
     logger.debug('getLinkingMetadata with no active transaction')
   }
 
+  if (!excludeServiceLinks) {
+    this.getServiceLinkingMetadata(linkingMetadata)
+  }
+
+  return linkingMetadata
+}
+
+/**
+ * This methods returns an object with the following keys/data:
+ * - `entity.name`: The application name specified in the connect request as
+ *   app_name. If multiple application names are specified this will only be
+ *   the first name
+ * - `entity.type`: The string "SERVICE"
+ * - `entity.guid`: The entity ID returned in the connect reply as entity_guid
+ * - `hostname`: The hostname as specified in the connect request as
+ *   utilization.full_hostname. If utilization.full_hostname is null or empty,
+ *   this will be the hostname specified in the connect request as host.
+ *
+ * @param {object} linkingMetadata object to add service linking keys
+ * @returns {object} with service linking metadata
+ */
+Agent.prototype.getServiceLinkingMetadata = function getServiceLinkingMetadata(
+  linkingMetadata = {}
+) {
+  const config = this.config
   if (config.entity_guid) {
     linkingMetadata['entity.guid'] = config.entity_guid
   }
-
+  linkingMetadata['entity.name'] = config.applications()[0]
+  linkingMetadata['entity.type'] = 'SERVICE'
+  linkingMetadata.hostname = config.getHostnameSafe()
   return linkingMetadata
 }
 

lib/aggregators/log-aggregator.js

@@ -56,7 +56,13 @@ class LogAggregator extends EventAggregator {
       return
     }
 
-    return [{ logs: formattedLogs }]
+    const commonAttrs = this.agent.getServiceLinkingMetadata()
+    return [
+      {
+        common: { attributes: commonAttrs },
+        logs: formattedLogs
+      }
+    ]
   }
 
   add(logLine) {

lib/instrumentation/nr-winston-transport.js

@@ -35,7 +35,7 @@ class NrTransport extends TransportStream {
    * @param {Function} callback callback to invoke once we are done
    */
   log(logLine, callback) {
-    const metadata = this.agent.getLinkingMetadata()
+    const metadata = this.agent.getLinkingMetadata(true)
     const formattedLine = reformatLogLine(logLine, metadata)
     this.agent.logs.add(formattedLine)
     callback()

lib/instrumentation/pino/pino.js

@@ -45,7 +45,6 @@ module.exports = function instrument(shim, tools) {
  * @param {object} params.tools exported `pino/lib/tools`
  */
 function wrapAsJson({ shim, tools }) {
-  const symbols = shim.require('./lib/symbols')
   const { agent } = shim
   const { config, metrics } = agent
 
@@ -88,12 +87,10 @@ function wrapAsJson({ shim, tools }) {
       const logLine = asJson.apply(this, args)
 
       if (isLogForwardingEnabled(config, agent)) {
-        const chindings = this[symbols.chindingsSym]
         const formatLogLine = reformatLogLine({
           msg: useMergeObj === true ? args[0].msg : args[1],
           logLine,
           agent,
-          chindings,
           level,
           logger: shim.logger
         })
@@ -114,22 +111,12 @@ function wrapAsJson({ shim, tools }) {
  * @param {object} params.logLine log line
  * @param {string} params.msg message of log line
  * @param {object} params.agent instance of agent
- * @param {string} params.chindings serialized string of all common log line data
  * @param {string} params.level log level
  * @param {object} params.logger instance of agent logger
  * @returns {function} wrapped log formatter function
  */
-function reformatLogLine({ logLine, msg, agent, chindings = '', level, logger }) {
-  const metadata = agent.getLinkingMetadata()
-
-  /**
-   * pino adds this already for us at times
-   * since asJson manually constructs the json string,
-   * it will have hostname twice if we do not delete ours.
-   */
-  if (chindings.includes('hostname')) {
-    delete metadata.hostname
-  }
+function reformatLogLine({ logLine, msg, agent, level, logger }) {
+  const metadata = agent.getLinkingMetadata(true)
 
   const agentMeta = Object.assign({}, { timestamp: Date.now() }, metadata)
   // eslint-disable-next-line eqeqeq

test/lib/logging-helper.js

@@ -7,28 +7,17 @@
 
 const assert = require('node:assert')
 
-// NOTE: pino adds hostname to log lines which is why we don't check it here
-const CONTEXT_KEYS = [
-  'entity.name',
-  'entity.type',
-  'entity.guid',
-  'trace.id',
-  'span.id',
-  'hostname'
-]
+const CONTEXT_KEYS = ['trace.id', 'span.id']
 
 /**
- * To be registered as a tap assertion
+ * Validates context about a given log line
+ *
+ * @param {object} params to fn
+ * @param {object} params.log log line
+ * @param {string} params.message message in log line
+ * @param {number} params.level log level
  */
-function validateLogLine({ line: logLine, message, level, config }) {
-  assert.equal(
-    logLine['entity.name'],
-    config.applications()[0],
-    'should have entity name that matches app'
-  )
-  assert.equal(logLine['entity.guid'], 'test-guid', 'should have set entity guid')
-  assert.equal(logLine['entity.type'], 'SERVICE', 'should have entity type of SERVICE')
-  assert.equal(logLine.hostname, config.getHostnameSafe(), 'should have proper hostname')
+function validateLogLine({ line: logLine, message, level }) {
   assert.equal(/[0-9]{10}/.test(logLine.timestamp), true, 'should have proper unix timestamp')
   assert.equal(
     logLine.message.includes('NR-LINKING'),
@@ -44,7 +33,26 @@ function validateLogLine({ line: logLine, message, level, config }) {
   }
 }
 
+/**
+ * Validates the common attributes of a given log payload
+ *
+ * @param {object} params to fn
+ * @param {object} params.commonAttrs common attributes on a log batch
+ * @param {object} params.config agent config
+ */
+function validateCommonAttrs({ commonAttrs, config }) {
+  assert.equal(
+    commonAttrs['entity.name'],
+    config.applications()[0],
+    'should have entity name that matches app'
+  )
+  assert.equal(commonAttrs['entity.guid'], 'test-guid', 'should have set entity guid')
+  assert.equal(commonAttrs['entity.type'], 'SERVICE', 'should have entity type of SERVICE')
+  assert.equal(commonAttrs.hostname, config.getHostnameSafe(), 'should have proper hostname')
+}
+
 module.exports = {
   CONTEXT_KEYS,
-  validateLogLine
+  validateLogLine,
+  validateCommonAttrs
 }

test/unit/agent/agent.test.js

@@ -1112,3 +1112,52 @@ test('_reset*', async (t) => {
     assert.equal(agent.customEventAggregator.clear.callCount, 1)
   })
 })
+
+test('getLinkingMetadata', async (t) => {
+  t.beforeEach((ctx) => {
+    const agent = helper.loadMockedAgent()
+    ctx.nr = { agent }
+  })
+
+  t.afterEach((ctx) => {
+    helper.unloadAgent(ctx.nr.agent)
+  })
+
+  await t.test('should include service links by default', (t, end) => {
+    const { agent } = t.nr
+    helper.runInTransaction(agent, (tx) => {
+      const metadata = agent.getLinkingMetadata()
+      assert.ok(metadata['trace.id'], tx.traceId)
+      assert.ok(metadata['span.id'], tx.trace.root.getSpanId())
+      assert.equal(metadata['entity.name'], 'New Relic for Node.js tests')
+      assert.equal(metadata['entity.type'], 'SERVICE')
+      assert.ok(!metadata['entity.guid'])
+      assert.equal(metadata.hostname, agent.config.getHostnameSafe())
+      end()
+    })
+  })
+
+  await t.test('should not include service links when passing true', (t, end) => {
+    const { agent } = t.nr
+    helper.runInTransaction(agent, (tx) => {
+      const metadata = agent.getLinkingMetadata(true)
+      assert.ok(metadata['trace.id'], tx.traceId)
+      assert.ok(metadata['span.id'], tx.trace.root.getSpanId())
+      assert.ok(!metadata['entity.name'])
+      assert.ok(!metadata['entity.type'])
+      assert.ok(!metadata['entity.guid'])
+      assert.ok(!metadata.hostname)
+      end()
+    })
+  })
+
+  await t.test('should return service linking metadata', (t) => {
+    const { agent } = t.nr
+    agent.config.entity_guid = 'guid'
+    const metadata = agent.getServiceLinkingMetadata()
+    assert.equal(metadata['entity.name'], 'New Relic for Node.js tests')
+    assert.equal(metadata['entity.type'], 'SERVICE')
+    assert.equal(metadata['entity.guid'], 'guid')
+    assert.equal(metadata.hostname, agent.config.getHostnameSafe())
+  })
+})

test/unit/aggregators/log-aggregator.test.js

@@ -19,10 +19,19 @@ test('Log Aggregator', async (t) => {
     ctx.nr = {}
 
     ctx.nr.txReturn = undefined
+    ctx.nr.commonAttrs = {
+      'entity.guid': 'MTkwfEFQTXxBUFBMSUNBVElPTnwyMjUzMDY0Nw',
+      'hostname': 'test-host',
+      'entity.name': 'unit-test',
+      'entity.type': 'SERVICE'
+    }
     ctx.nr.agent = {
       getTransaction() {
         return ctx.nr.txReturn
       },
+      getServiceLinkingMetadata() {
+        return ctx.nr.commonAttrs
+      },
       collector: {},
       metrics: new Metrics(5, {}, {}),
       harvester: { add() {} },
@@ -42,12 +51,8 @@ test('Log Aggregator', async (t) => {
       'level': 30,
       'timestamp': '1649689872369',
       'pid': 4856,
-      'hostname': 'test-host',
-      'entity.name': 'unit-test',
-      'entity.type': 'SERVICE',
       'trace.id': '2f93639c684a2dd33c28345173d218b8',
       'span.id': 'a136d77f2a5b997b',
-      'entity.guid': 'MTkwfEFQTXxBUFBMSUNBVElPTnwyMjUzMDY0Nw',
       'message': 'unit test msg'
     }
   })
@@ -59,7 +64,7 @@ test('Log Aggregator', async (t) => {
   })
 
   await t.test('toPayload() should return json format of data', (t) => {
-    const { logEventAggregator, log } = t.nr
+    const { logEventAggregator, log, commonAttrs } = t.nr
     const logs = []
 
     for (let i = 0; i <= 8; i += 1) {
@@ -70,26 +75,28 @@ test('Log Aggregator', async (t) => {
     }
     const payload = logEventAggregator._toPayloadSync()
     assert.equal(payload.length, 1)
-    assert.deepStrictEqual(payload, [{ logs: logs.reverse() }])
+    assert.deepStrictEqual(payload, [{ common: { attributes: commonAttrs }, logs: logs.reverse() }])
   })
 
   await t.test(
     'toPayload() should execute formatter function when an entry in aggregator is a function',
     (t) => {
-      const { logEventAggregator, log } = t.nr
+      const { commonAttrs, logEventAggregator, log } = t.nr
       const log2 = JSON.stringify(log)
       function formatLog() {
         return JSON.parse(log2)
       }
       logEventAggregator.add(log)
       logEventAggregator.add(formatLog)
       const payload = logEventAggregator._toPayloadSync()
-      assert.deepStrictEqual(payload, [{ logs: [log, JSON.parse(log2)] }])
+      assert.deepStrictEqual(payload, [
+        { common: { attributes: commonAttrs }, logs: [log, JSON.parse(log2)] }
+      ])
     }
   )
 
   await t.test('toPayload() should only return logs that have data', (t) => {
-    const { logEventAggregator, log } = t.nr
+    const { commonAttrs, logEventAggregator, log } = t.nr
     const log2 = JSON.stringify(log)
     function formatLog() {
       return JSON.parse(log2)
@@ -101,7 +108,9 @@ test('Log Aggregator', async (t) => {
     logEventAggregator.add(formatLog)
     logEventAggregator.add(formatLog2)
     const payload = logEventAggregator._toPayloadSync()
-    assert.deepStrictEqual(payload, [{ logs: [log, JSON.parse(log2)] }])
+    assert.deepStrictEqual(payload, [
+      { common: { attributes: commonAttrs }, logs: [log, JSON.parse(log2)] }
+    ])
   })
 
   await t.test('toPayload() should return nothing with no log event data', (t) => {

test/versioned/bunyan/helpers.js

@@ -8,7 +8,7 @@
 const assert = require('node:assert')
 
 const helpers = module.exports
-const { CONTEXT_KEYS, validateLogLine } = require('../../lib/logging-helper')
+const { CONTEXT_KEYS, validateLogLine, validateCommonAttrs } = require('../../lib/logging-helper')
 
 /**
  * Provides a mocked-up writable stream that can be provided to Bunyan for easier testing
@@ -56,9 +56,7 @@ helpers.originalMsgAssertion = function originalMsgAssertion({
   hostname
 }) {
   CONTEXT_KEYS.forEach((key) => {
-    if (key !== 'hostname') {
-      assert.equal(logLine[key], undefined, `should not have ${key}`)
-    }
+    assert.equal(logLine[key], undefined, `should not have ${key}`)
   })
 
   assert.ok(logLine.time, 'should include timestamp')
@@ -103,4 +101,8 @@ helpers.logForwardingMsgAssertion = function logForwardingMsgAssertion(logLine,
     assert.equal(typeof logLine['trace.id'], 'string', 'msg in trans should have trace id')
     assert.equal(typeof logLine['span.id'], 'string', 'msg in trans should have span id')
   }
+
+  const [payload] = agent.logs._toPayloadSync()
+  const commonAttrs = payload.common.attributes
+  validateCommonAttrs({ commonAttrs, config: agent.config })
 }

test/versioned/pino/helpers.js

@@ -25,9 +25,7 @@ helpers.originalMsgAssertion = function originalMsgAssertion({
   hostname
 }) {
   CONTEXT_KEYS.forEach((key) => {
-    if (key !== 'hostname') {
-      assert.equal(logLine[key], undefined, `should not have ${key}`)
-    }
+    assert.equal(logLine[key], undefined, `should not have ${key}`)
   })
 
   assert.ok(logLine.time, 'should include timestamp')