fix: some guardrail telemetry should only be sent once (#6029)

Fixes bug in the guardrail telemetry `hasSeen` function which meant that all
telemetry was always sent even though `abort` and `abort.integration` should
only be sent once.

Author: watson

.github/workflows/platform.yml

@@ -283,6 +283,15 @@ jobs:
       - run: yarn test:integration
       - run: yarn test:integration:esbuild
 
+  unit-guardrails:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+      - uses: ./.github/actions/node/active-lts
+      - uses: ./.github/actions/install
+      - run: yarn test:trace:guardrails:ci
+      - uses: codecov/codecov-action@18283e04ce6e62d37312384ff67231eb8fd56d24 # v5.4.3
+
   # We'll run these separately for earlier (i.e. unsupported) versions
   integration-guardrails:
     strategy:

integration-tests/helpers/index.js

@@ -58,34 +58,35 @@ let sandbox
 // This _must_ be used with the useSandbox function
 async function runAndCheckWithTelemetry (filename, expectedOut, expectedTelemetryPoints, expectedSource) {
   const cwd = sandbox.folder
-  const cleanup = telemetryForwarder(expectedTelemetryPoints)
+  const cleanup = telemetryForwarder(expectedTelemetryPoints.length > 0)
   const pid = await runAndCheckOutput(filename, cwd, expectedOut, expectedSource)
   const msgs = await cleanup()
   if (expectedTelemetryPoints.length === 0) {
     // assert no telemetry sent
-    try {
-      assert.deepStrictEqual(msgs.length, 0)
-    } catch (e) {
-      // This console.log is useful for debugging telemetry. Plz don't remove.
-      // eslint-disable-next-line no-console
-      console.error('Expected no telemetry, but got:\n', msgs.map(msg => JSON.stringify(msg[1].points)).join('\n'))
-      throw e
-    }
-    return
+    assert.strictEqual(msgs.length, 0, `Expected no telemetry, but got:\n${
+      msgs.map(msg => JSON.stringify(msg[1].points)).join('\n')
+    }`)
+  } else {
+    assertTelemetryPoints(pid, msgs, expectedTelemetryPoints)
   }
+}
+
+function assertTelemetryPoints (pid, msgs, expectedTelemetryPoints) {
   let points = []
   for (const [telemetryType, data] of msgs) {
     assert.strictEqual(telemetryType, 'library_entrypoint')
     assert.deepStrictEqual(data.metadata, meta(pid))
     points = points.concat(data.points)
   }
-  let expectedPoints = getPoints(...expectedTelemetryPoints)
-  // We now have to sort both the expected and actual telemetry points.
-  // This is because data can come in in any order.
-  // We'll just contatenate all the data together for each point and sort them.
-  points = points.map(p => p.name + '\t' + p.tags.join(',')).sort().join('\n')
-  expectedPoints = expectedPoints.map(p => p.name + '\t' + p.tags.join(',')).sort().join('\n')
-  assert.strictEqual(points, expectedPoints)
+  const expectedPoints = getPoints(...expectedTelemetryPoints)
+  // Sort since data can come in in any order.
+  assert.deepStrictEqual(points.sort(pointsSorter), expectedPoints.sort(pointsSorter))
+
+  function pointsSorter (a, b) {
+    a = a.name + '\t' + a.tags.join(',')
+    b = b.name + '\t' + b.tags.join(',')
+    return a === b ? 0 : a < b ? -1 : 1
+  }
 
   function getPoints (...args) {
     const expectedPoints = []
@@ -94,7 +95,7 @@ async function runAndCheckWithTelemetry (filename, expectedOut, expectedTelemetr
       if (!currentPoint.name) {
         currentPoint.name = 'library_entrypoint.' + arg
       } else {
-        currentPoint.tags = arg.split(',')
+        currentPoint.tags = arg.split(',').filter(Boolean)
         expectedPoints.push(currentPoint)
         currentPoint = {}
       }
@@ -239,7 +240,7 @@ async function createSandbox (dependencies = [], isGitRepo = false,
   }
 }
 
-function telemetryForwarder (expectedTelemetryPoints) {
+function telemetryForwarder (shouldExpectTelemetryPoints = true) {
   process.env.DD_TELEMETRY_FORWARDER_PATH =
     path.join(__dirname, '..', 'telemetry-forwarder.sh')
   process.env.FORWARDER_OUT = path.join(__dirname, `forwarder-${Date.now()}.out`)
@@ -257,7 +258,7 @@ function telemetryForwarder (expectedTelemetryPoints) {
     try {
       msgs = fs.readFileSync(process.env.FORWARDER_OUT, 'utf8').trim().split('\n')
     } catch (e) {
-      if (expectedTelemetryPoints.length && e.code === 'ENOENT' && retries < 10) {
+      if (shouldExpectTelemetryPoints && e.code === 'ENOENT' && retries < 10) {
         return tryAgain()
       }
       return []
@@ -438,6 +439,8 @@ module.exports = {
   assertObjectContains,
   assertUUID,
   spawnProc,
+  telemetryForwarder,
+  assertTelemetryPoints,
   runAndCheckWithTelemetry,
   createSandbox,
   curl,

package.json

@@ -27,6 +27,8 @@
     "test:eslint-rules": "node eslint-rules/*.test.mjs",
     "test:trace:core": "tap packages/dd-trace/test/*.spec.js \"packages/dd-trace/test/{ci-visibility,datastreams,encode,exporters,opentelemetry,opentracing,plugins,service-naming,standalone,telemetry}/**/*.spec.js\"",
     "test:trace:core:ci": "npm run test:trace:core -- --coverage --nyc-arg=--include=\"packages/dd-trace/src/**/*.js\"",
+    "test:trace:guardrails": "mocha -r \"packages/dd-trace/test/setup/mocha.js\" \"packages/dd-trace/test/guardrails/**/*.spec.js\"",
+    "test:trace:guardrails:ci": "nyc --no-clean --include \"packages/dd-trace/src/guardrails/**/*.js\" -- npm run test:trace:guardrails",
     "test:instrumentations": "mocha -r 'packages/dd-trace/test/setup/mocha.js' \"packages/datadog-instrumentations/test/@($(echo $PLUGINS)).spec.js\"",
     "test:instrumentations:ci": "yarn services && nyc --no-clean --include \"packages/datadog-instrumentations/src/@($(echo $PLUGINS)).js\" --include \"packages/datadog-instrumentations/src/@($(echo $PLUGINS))/**/*.js\" -- npm run test:instrumentations",
     "test:instrumentations:misc": "mocha -r 'packages/dd-trace/test/setup/mocha.js' 'packages/datadog-instrumentations/test/*/**/*.spec.js'",

packages/dd-trace/src/guardrails/telemetry.js

@@ -25,18 +25,23 @@ var metadata = {
   pid: process.pid
 }
 
-var seen = []
-function hasSeen (point) {
+var seen = {}
+function shouldSend (point) {
   if (point.name === 'abort') {
     // This one can only be sent once, regardless of tags
-    return seen.indexOf('abort') !== -1
-  }
-  if (point.name === 'abort.integration') {
+    if (seen.abort) {
+      return false
+    }
+    seen.abort = true
+  } else if (point.name === 'abort.integration') {
     // For now, this is the only other one we want to dedupe
     var compiledPoint = point.name + point.tags.join('')
-    return seen.indexOf(compiledPoint) !== -1
+    if (seen[compiledPoint]) {
+      return false
+    }
+    seen[compiledPoint] = true
   }
-  return false
+  return true
 }
 
 function sendTelemetry (name, tags) {
@@ -47,7 +52,7 @@ function sendTelemetry (name, tags) {
   if (['1', 'true', 'True'].indexOf(process.env.DD_INJECT_FORCE) !== -1) {
     points = points.filter(function (p) { return ['error', 'complete'].indexOf(p.name) !== -1 })
   }
-  points = points.filter(function (p) { return !hasSeen(p) })
+  points = points.filter(function (p) { return shouldSend(p) })
   for (var i = 0; i < points.length; i++) {
     points[i].name = 'library_entrypoint.' + points[i].name
   }

packages/dd-trace/test/guardrails/telemetry.spec.js

@@ -0,0 +1,64 @@
+'use strict'
+
+process.env.DD_INJECTION_ENABLED = 'true'
+
+const { telemetryForwarder, assertTelemetryPoints } = require('../../../../integration-tests/helpers')
+
+describe('sendTelemetry', () => {
+  let cleanup, sendTelemetry
+
+  beforeEach(() => {
+    cleanup = telemetryForwarder()
+    sendTelemetry = proxyquire('../src/guardrails/telemetry', {})
+  })
+
+  it('should send telemetry', async () => {
+    sendTelemetry([
+      { name: 'abort', tags: ['1'] },
+      { name: 'abort.integration', tags: ['2'] },
+      { name: 'abort.integration', tags: ['3'] },
+      { name: 'foo', tags: ['4'] }
+    ])
+    const msgs = await cleanup()
+    assertTelemetryPoints(process.pid, msgs, [
+      'abort', '1',
+      'abort.integration', '2',
+      'abort.integration', '3',
+      'foo', '4'
+    ])
+  })
+
+  describe('no duplicates', () => {
+    it('should not send `abort` more than once in the same call', async () => {
+      sendTelemetry([
+        { name: 'abort', tags: ['1'] },
+        { name: 'abort', tags: ['2'] }
+      ])
+      const msgs = await cleanup()
+      assertTelemetryPoints(process.pid, msgs, ['abort', '1'])
+    })
+
+    it('should not send `abort` more than once in a different call', async () => {
+      sendTelemetry('abort', ['1'])
+      sendTelemetry('abort', ['2'])
+      const msgs = await cleanup()
+      assertTelemetryPoints(process.pid, msgs, ['abort', '1'])
+    })
+
+    it('should not send `abort.integration` more than once if tags are the same in the same call', async () => {
+      sendTelemetry([
+        { name: 'abort.integration', tags: ['1'] },
+        { name: 'abort.integration', tags: ['1'] }
+      ])
+      const msgs = await cleanup()
+      assertTelemetryPoints(process.pid, msgs, ['abort.integration', '1'])
+    })
+
+    it('should not send `abort.integration` more than once if tags are the same in a different call', async () => {
+      sendTelemetry('abort.integration', ['1'])
+      sendTelemetry('abort.integration', ['1'])
+      const msgs = await cleanup()
+      assertTelemetryPoints(process.pid, msgs, ['abort.integration', '1'])
+    })
+  })
+})