refactor: Updated Samplers class to normalize logger messages, short circuit when applicable and remove optional chaining checks (#3546)

Author: bizob2828

lib/samplers/index.js

@@ -44,16 +44,20 @@ class Samplers {
    * @param {Transaction} params.transaction The transaction to apply the sampling decision to.
    */
   applyDefaultDecision({ transaction }) {
-    if (!transaction) {
-      logger.trace('Both full and partial granularity samplers are disabled. No transaction provided to apply default sampling decision.')
-      return
-    }
-
-    logger.trace('Both full and partial granularity samplers are disabled. Applying default sampling decision of not sampled and priority 0 for transaction %s', transaction?.id)
+    logger.trace('Both full and partial granularity samplers are disabled. Applying default sampling decision of not sampled and priority 0 for transaction %s', transaction.id)
     transaction.sampled = false
     transaction.priority = 0
   }
 
+  /**
+   * Determines if the partial granularity sampler should run based on the transaction's sampled value and if partial granularity is enabled.
+   * @param {Transaction} transaction The transaction to check.
+   * @returns {boolean} True if the partial granularity sampler should run, false otherwise.
+   */
+  shouldRunPartialSampling(transaction) {
+    return transaction.sampled !== true && this.partialEnabled
+  }
+
   /**
    * Applies the root sampler's sampling decision to the transaction
    * if priority has not already been set. If both full and partial granularity
@@ -62,20 +66,29 @@ class Samplers {
    * @param {Transaction} transaction The transaction to apply the sampling decision to.
    */
   applySamplingDecision({ transaction }) {
-    if (transaction?.priority === null) {
-      if (this.fullEnabled === false && this.partialEnabled === false) {
-        this.applyDefaultDecision({ transaction })
-        return
-      }
+    if (!transaction) {
+      logger.trace('No transaction provided to applySamplingDecision, not applying sampler.')
+      return
+    }
 
-      if (this.fullEnabled) {
-        this.root.applySamplingDecision({ transaction, isFullTrace: true })
-        logger.trace('Ran full granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.root.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
-      }
-      if (!transaction?.sampled && this.partialEnabled) {
-        this.partialRoot.applySamplingDecision({ transaction })
-        logger.trace('Ran partial granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.partialRoot.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
-      }
+    if (transaction.priority !== null) {
+      logger.trace('Transaction %s already has a sampling decision, not applying sampler.', transaction.id)
+      return
+    }
+
+    if (this.fullEnabled === false && this.partialEnabled === false) {
+      this.applyDefaultDecision({ transaction })
+      return
+    }
+
+    if (this.fullEnabled) {
+      this.root.applySamplingDecision({ transaction, isFullTrace: true })
+      logger.trace('Ran full granularity applySamplingDecision %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.root.toString(), transaction.id, transaction.sampled, transaction.priority)
+    }
+
+    if (this.shouldRunPartialSampling(transaction)) {
+      this.partialRoot.applySamplingDecision({ transaction })
+      logger.trace('Ran partial granularity applySamplingDecision %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.partialRoot.toString(), transaction.id, transaction.sampled, transaction.priority)
     }
   }
 
@@ -90,30 +103,26 @@ class Samplers {
    * @param {TraceState} params.tracestate The W3C tracestate object.
    */
   applyDTSamplingDecision({ transaction, traceparent, tracestate }) {
+    if (!transaction) {
+      logger.trace('No transaction provided to applyDTSamplingDecision, not applying sampler.')
+      return
+    }
+
     if (this.fullEnabled === false && this.partialEnabled === false) {
       this.applyDefaultDecision({ transaction })
       return
     }
 
-    // Decide sampling from w3c data by supplying tracestate to sampler
-    if (traceparent?.isSampled === true) {
-      if (this.fullEnabled) {
-        this.remoteParentSampled.applySamplingDecision({ transaction, tracestate, isFullTrace: true })
-        logger.trace('Ran DT Sampling full granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.remoteParentSampled.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
-      }
-      if (!transaction?.sampled && this.partialEnabled) {
-        this.partialRemoteParentSampled.applySamplingDecision({ transaction, tracestate })
-        logger.trace('Ran DT Sampling partial granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.partialRemoteParentSampled.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
-      }
-    } else if (traceparent?.isSampled === false) {
-      if (this.fullEnabled) {
-        this.remoteParentNotSampled.applySamplingDecision({ transaction, tracestate, isFullTrace: true })
-        logger.trace('Ran DT Sampling full granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.remoteParentNotSampled.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
-      }
-      if (!transaction?.sampled && this.partialEnabled) {
-        this.partialRemoteParentNotSampled.applySamplingDecision({ transaction, tracestate })
-        logger.trace('Ran DT Sampling partial granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', this.partialRemoteParentNotSampled.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
-      }
+    if (this.fullEnabled) {
+      const sampler = traceparent?.isSampled ? this.remoteParentSampled : this.remoteParentNotSampled
+      sampler.applySamplingDecision({ transaction, tracestate, isFullTrace: true })
+      logger.trace('Ran full granularity applyDTSamplingDecision %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', sampler.toString(), transaction.id, transaction.sampled, transaction.priority)
+    }
+
+    if (this.shouldRunPartialSampling(transaction)) {
+      const partialSampler = traceparent?.isSampled ? this.partialRemoteParentSampled : this.partialRemoteParentNotSampled
+      partialSampler.applySamplingDecision({ transaction, tracestate })
+      logger.trace('Ran partial granularity applyDTSamplingDecision %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', partialSampler.toString(), transaction.id, transaction.sampled, transaction.priority)
     }
   }
 
@@ -130,28 +139,33 @@ class Samplers {
    * @param {boolean} params.isSampled The sampled value from the legacy New Relic headers.
    */
   applyLegacyDTSamplingDecision({ transaction, isSampled }) {
+    if (!transaction) {
+      logger.trace('No transaction provided to applyLegacyDTSamplingDecision, not applying sampler.')
+      return
+    }
+
     if (this.fullEnabled === false && this.partialEnabled === false) {
       this.applyDefaultDecision({ transaction })
       return
     }
 
-    const sampler = isSampled ? this.remoteParentSampled : this.remoteParentNotSampled
-    const partialSampler = isSampled ? this.partialRemoteParentSampled : this.partialRemoteParentNotSampled
     if (this.fullEnabled) {
-      if (sampler?.toString() === 'AdaptiveSampler') {
-        logger.trace('Not applying full granularity sampling decision from legacy DT headers for transaction %s because sampler is AdaptiveSampler', transaction?.id)
+      const sampler = isSampled ? this.remoteParentSampled : this.remoteParentNotSampled
+      if (sampler.toString() === 'AdaptiveSampler') {
+        logger.trace('Not running full granularity applyLegacyDTSamplingDecision for transaction %s because sampler is AdaptiveSampler', transaction.id)
       } else {
         sampler.applySamplingDecision({ transaction, isFullTrace: true })
-        logger.trace('Ran Legacy DT Sampling full granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', sampler.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
+        logger.trace('Ran full granularity applyLegacyDTSamplingDecision %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', sampler.toString(), transaction.id, transaction.sampled, transaction.priority)
       }
     }
 
-    if (!transaction.sampled && this.partialEnabled) {
-      if (partialSampler?.toString() === 'AdaptiveSampler') {
-        logger.trace('Not applying partial granularity sampling decision from legacy DT headers for transaction %s because sampler is AdaptiveSampler', transaction?.id)
+    if (this.shouldRunPartialSampling(transaction)) {
+      const partialSampler = isSampled ? this.partialRemoteParentSampled : this.partialRemoteParentNotSampled
+      if (partialSampler.toString() === 'AdaptiveSampler') {
+        logger.trace('Not running partial granularity applyLegacyDTSamplingDecision for transaction %s because sampler is AdaptiveSampler', transaction.id)
       } else {
         partialSampler.applySamplingDecision({ transaction })
-        logger.trace('Ran Legacy DT Sampling partial granularity %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', partialSampler.toString(), transaction?.id, transaction?.sampled, transaction?.priority)
+        logger.trace('Ran partial granularity applyLegacyDTSamplingDecision %s sampler for transaction %s, decision: { sampled: %s, priority: %n}', partialSampler.toString(), transaction.id, transaction.sampled, transaction.priority)
       }
     }
   }
@@ -182,15 +196,17 @@ class Samplers {
    * @returns {AdaptiveSampler} The global AdaptiveSampler instance.
    */
   getAdaptiveSampler(agent) {
-    const config = agent.config
-    if (!this.adaptiveSampler) {
-      this.adaptiveSampler = new AdaptiveSampler({
-        agent,
-        serverless: config.serverless_mode.enabled,
-        period: config.sampling_target_period_in_seconds * 1000,
-        target: config.sampling_target
-      })
+    if (this.adaptiveSampler !== null) {
+      return this.adaptiveSampler
     }
+
+    const config = agent.config
+    this.adaptiveSampler = new AdaptiveSampler({
+      agent,
+      serverless: config.serverless_mode.enabled,
+      period: config.sampling_target_period_in_seconds * 1000,
+      target: config.sampling_target
+    })
     return this.adaptiveSampler
   }
 
@@ -206,9 +222,9 @@ class Samplers {
     const config = agent.config
     let samplerDefinition = null
     if (isPartial) {
-      samplerDefinition = config?.distributed_tracing?.sampler?.partial_granularity?.[sampler]
+      samplerDefinition = config.distributed_tracing.sampler.partial_granularity[sampler]
     } else {
-      samplerDefinition = config?.distributed_tracing?.sampler?.[sampler]
+      samplerDefinition = config.distributed_tracing.sampler[sampler]
     }
 
     // Always on?
@@ -225,7 +241,7 @@ class Samplers {
     if (samplerDefinition?.trace_id_ratio_based) {
       let ratio = samplerDefinition.trace_id_ratio_based?.ratio
       // If both partial and full granularity for a particular section are both set to trace ratio, agent **MUST** set the partial granularity ratio = full granularity ratio + partial granularity ratio
-      if (isPartial && this.fullEnabled && config?.distributed_tracing?.sampler?.[sampler]?.trace_id_ratio_based?.ratio) {
+      if (isPartial && this.fullEnabled && config.distributed_tracing.sampler[sampler]?.trace_id_ratio_based?.ratio) {
         ratio += config.distributed_tracing.sampler[sampler].trace_id_ratio_based.ratio
       }
       return new TraceIdRatioBasedSampler({

lib/samplers/sampler.js

@@ -2,6 +2,7 @@
  * Copyright 2025 New Relic Corporation. All rights reserved.
  * SPDX-License-Identifier: Apache-2.0
  */
+const util = require('node:util')
 
 /**
  * @private
@@ -20,7 +21,8 @@ class Sampler {
    * @param {boolean} params.isFullTrace whether or not full tracing is enabled
    */
   applySamplingDecision({ transaction, tracestate, isFullTrace }) {
-    throw new Error('must implement applySamplingDecision, arguments are: { transaction: %d, tracestate: %s, isFullTrace: %s}', transaction?.id, tracestate, isFullTrace)
+    const formattedError = util.format('must implement applySamplingDecision, arguments are: { transaction: %d, tracestate: %s, isFullTrace: %s}', transaction?.id, tracestate, isFullTrace)
+    throw new Error(formattedError)
   }
 }
 

test/unit/samplers/always-off-sampler.test.js

@@ -0,0 +1,39 @@
+/*
+ * Copyright 2025 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+const test = require('node:test')
+const assert = require('node:assert')
+const AlwaysOffSampler = require('#agentlib/samplers/always-off-sampler.js')
+
+test.beforeEach((ctx) => {
+  const sampler = new AlwaysOffSampler()
+  ctx.nr = { sampler }
+})
+
+test('AlwaysOffSampler should always sample', (t) => {
+  const { sampler } = t.nr
+  const transaction = {}
+  sampler.applySamplingDecision({ transaction, isFullTrace: true })
+  assert.equal(transaction.sampled, false)
+  assert.equal(transaction.priority, 0)
+  assert.equal(transaction.isPartialTrace, false)
+})
+
+test('AlwaysOffSampler should assign isPartialTrace to true when not a fullTrace', (t) => {
+  const { sampler } = t.nr
+  const transaction = {}
+  sampler.applySamplingDecision({ transaction, isFullTrace: false })
+  assert.equal(transaction.sampled, false)
+  assert.equal(transaction.priority, 0)
+  assert.equal(transaction.isPartialTrace, true)
+})
+
+test('AlwaysOffSampler should return null when transaction is not provided', (t) => {
+  const { sampler } = t.nr
+  assert.doesNotThrow(() => {
+    sampler.applySamplingDecision({ transaction: null, isFullTrace: true })
+  })
+})

test/unit/samplers/always-on-sampler.test.js

@@ -0,0 +1,39 @@
+/*
+ * Copyright 2025 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+const test = require('node:test')
+const assert = require('node:assert')
+const AlwaysOnSampler = require('#agentlib/samplers/always-on-sampler.js')
+
+test.beforeEach((ctx) => {
+  const sampler = new AlwaysOnSampler()
+  ctx.nr = { sampler }
+})
+
+test('AlwaysOnSampler should always sample', (t) => {
+  const { sampler } = t.nr
+  const transaction = {}
+  sampler.applySamplingDecision({ transaction, isFullTrace: true })
+  assert.equal(transaction.sampled, true)
+  assert.equal(transaction.priority, 2)
+  assert.equal(transaction.isPartialTrace, false)
+})
+
+test('AlwaysOnSampler should assign isPartialTrace to true when not a fullTrace', (t) => {
+  const { sampler } = t.nr
+  const transaction = {}
+  sampler.applySamplingDecision({ transaction, isFullTrace: false })
+  assert.equal(transaction.sampled, true)
+  assert.equal(transaction.priority, 2)
+  assert.equal(transaction.isPartialTrace, true)
+})
+
+test('AlwaysOnSampler should return null when transaction is not provided', (t) => {
+  const { sampler } = t.nr
+  assert.doesNotThrow(() => {
+    sampler.applySamplingDecision({ transaction: null, isFullTrace: true })
+  })
+})

test/unit/samplers/index.test.js

@@ -540,19 +540,6 @@ test('applyDTSamplingDecision', async (t) => {
     })
   })
 
-  await t.test('should not apply decision when traceparent.isSampled is undefined', (t) => {
-    const samplers = new Samplers(t.nr.agent)
-
-    const transaction = { priority: null, sampled: null }
-    const traceparent = { isSampled: undefined }
-    const tracestate = null
-
-    samplers.applyDTSamplingDecision({ transaction, traceparent, tracestate })
-
-    assert.equal(transaction.priority, null)
-    assert.equal(transaction.sampled, null)
-  })
-
   await t.test('should handle tracestate without intrinsics', (t) => {
     t.nr.agent.config = new Config({
       distributed_tracing: {

test/unit/samplers/sampler.test.js

@@ -0,0 +1,16 @@
+/*
+ * Copyright 2025 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+'use strict'
+const test = require('node:test')
+const assert = require('node:assert')
+const Sampler = require('#agentlib/samplers/sampler.js')
+
+test('should throw error if applySamplingDecision is not implemented', () => {
+  const sampler = new Sampler()
+  assert.throws(() => {
+    sampler.applySamplingDecision({ transaction: { id: 1 }, tracestate: 'tracestate', isFullTrace: true })
+  }, /^Error: must implement applySamplingDecision, arguments are: { transaction: 1, tracestate: tracestate, isFullTrace: true/)
+})