feat: Removed ability to specify profiling.sample_interval (#3844)

Author: bizob2828

lib/agent.js

@@ -309,7 +309,7 @@ function Agent(config) {
       config,
       delay: config.profiling.delay,
       duration: config.profiling.duration,
-      periodMs: config.profiling.sample_interval,
+      periodMs: DEFAULT_HARVEST_INTERVAL_MS,
       method: 'pprof_data',
       isAsync: !config.serverless_mode.enabled,
       enabled: (config) => config.profiling.enabled && config.serverless_mode.enabled === false

lib/aggregators/profiling-aggregator.js

@@ -28,7 +28,7 @@ class ProfilingAggregator extends BaseAggregator {
     opts.method = opts.method || 'pprof_data'
     super(opts, collector, harvester)
     this.agent = agent
-    this.profilingManager = new ProfilingManager(agent)
+    this.profilingManager = new ProfilingManager({ agent, samplingInterval: opts.periodMs })
     this.pprofData = null
   }
 
@@ -77,7 +77,7 @@ class ProfilingAggregator extends BaseAggregator {
     try {
       profilingData = await this.profilingManager.collect()
     } catch (err) {
-      logger.error(err, 'Failed to collect profiilng data')
+      logger.error(err, 'Failed to collect profiling data')
     }
 
     for (const pprofData of profilingData) {

lib/config/default.js

@@ -1261,13 +1261,6 @@ defaultConfig.definition = () => {
         formatter: boolean,
         default: false
       },
-      /**
-       * Harvest and ingest profile data every n milliseconds
-       */
-      sample_interval: {
-        formatter: int,
-        default: 100
-      },
       /**
        * List of profile type names to enable. Only cpu and heap profiles are currently supported
        */

lib/profiling/index.js

@@ -8,10 +8,11 @@ const defaultLogger = require('#agentlib/logger.js').child({ component: 'profili
 const { createProfilingDurationMetric } = require('./metrics')
 
 class ProfilingManager {
-  constructor(agent, { logger = defaultLogger } = {}) {
+  constructor({ agent, samplingInterval }, { logger = defaultLogger } = {}) {
     this.logger = logger
     this.metrics = agent.metrics
     this.config = agent.config.profiling
+    this.samplingInterval = samplingInterval
     this.profilers = new Map()
     this.startTime = null
   }
@@ -24,7 +25,7 @@ class ProfilingManager {
 
     if (this.config.include.includes('cpu') && !this.profilers.has('CpuProfiler')) {
       const { CpuProfiler } = require('./profilers')
-      this.profilers.set('CpuProfiler', new CpuProfiler({ logger: this.logger }))
+      this.profilers.set('CpuProfiler', new CpuProfiler({ logger: this.logger, samplingInterval: this.samplingInterval }))
     }
   }
 

lib/profiling/profilers/cpu.js

@@ -8,9 +8,12 @@ const BaseProfiler = require('./base')
 
 class CpuProfiler extends BaseProfiler {
   #pprof
-  constructor({ logger }) {
+  #durationMillis
+  #intervalMicros = (1e3 / 99) * 1000 // samples at 99hz(99 times per second)
+  constructor({ logger, samplingInterval }) {
     super({ logger })
     this.#pprof = require('@datadog/pprof')
+    this.#durationMillis = samplingInterval
   }
 
   start() {
@@ -19,9 +22,10 @@ class CpuProfiler extends BaseProfiler {
       return
     }
 
+    this.logger.trace(`Starting CpuProfiler, sample every ${this.#intervalMicros}hz for ${this.#durationMillis} ms.`)
     this.#pprof.time.start({
-      durationMillis: 60 * 1e3, // 1 min
-      intervalMicros: (1e3 / 99) * 1000
+      durationMillis: this.#durationMillis,
+      intervalMicros: this.#intervalMicros
     })
   }
 

lib/profiling/profilers/heap.js

@@ -9,7 +9,7 @@ const BaseProfiler = require('../profilers/base')
 
 class HeapProfiler extends BaseProfiler {
   #pprof
-  #intervalBytes = 524288
+  #intervalBytes = 524288 // captures stack trace every 512 kb of allocated memory
   #stackDepth = 64
   constructor({ logger }) {
     super({ logger })
@@ -18,6 +18,7 @@ class HeapProfiler extends BaseProfiler {
 
   start() {
     try {
+      this.logger.trace(`Starting HeapProfiler, sample every ${this.#intervalBytes} bytes with a stack depth of ${this.#stackDepth}`)
       this.#pprof.heap.start(this.#intervalBytes, this.#stackDepth)
     } catch (error) {
       this.logger.error({ error }, 'Failed to start HeapProfiler')

test/unit/agent/agent.test.js

@@ -959,7 +959,6 @@ test('should set up delay + duration for profilingAggregator', async (t) => {
     profiling: {
       enabled: true,
       duration: 300,
-      sample_interval: 50,
       delay: 50
     },
     ...collector.agentConfig,
@@ -972,6 +971,10 @@ test('should set up delay + duration for profilingAggregator', async (t) => {
     }
   }
   const agent = helper.loadMockedAgent(config, false)
+  // change period of profiling aggregator as faking timers is a pain for this
+  // and the default is 1 min
+  const profilingAggregator = agent.harvester.aggregators.find((aggregator) => aggregator.constructor.name === 'ProfilingAggregator')
+  profilingAggregator.periodMs = 50
 
   t.after(() => {
     helper.unloadAgent(agent)

test/unit/config/config-defaults.test.js

@@ -350,7 +350,6 @@ test('with default properties', async (t) => {
     assert.equal(configuration.profiling.enabled, false)
     assert.equal(configuration.profiling.delay, 0)
     assert.equal(configuration.profiling.duration, 0)
-    assert.equal(configuration.profiling.sample_interval, 100)
     assert.deepEqual(configuration.profiling.include, ['cpu', 'heap'])
   })
 

test/unit/config/config-env.test.js

@@ -485,15 +485,13 @@ test('when overriding configuration values via environment variables', async (t)
       NEW_RELIC_PROFILING_INCLUDE: ['heap'],
       NEW_RELIC_PROFILING_DELAY: 100,
       NEW_RELIC_PROFILING_DURATION: 20000,
-      NEW_RELIC_PROFILING_SAMPLE_INTERVAL: 150
     }
 
     idempotentEnv(env, (tc) => {
       assert(tc.profiling.enabled, true)
       assert.deepStrictEqual(tc.profiling.include, ['heap'])
       assert.equal(tc.profiling.delay, 100)
       assert.equal(tc.profiling.duration, 20000)
-      assert.equal(tc.profiling.sample_interval, 150)
       end()
     })
   })

test/unit/lib/profiling/index.test.js

@@ -25,11 +25,13 @@ test.beforeEach((ctx) => {
 
   const cpuProfiler = createProfiler({ sandbox, name: 'cpu' })
   const heapProfiler = createProfiler({ sandbox, name: 'heap' })
+  const profilingManager = new ProfilingManager({ agent, samplingInterval: 60_000 }, { logger })
   ctx.nr = {
     agent,
     cpuProfiler,
     heapProfiler,
     logger,
+    profilingManager,
     sandbox
   }
 })
@@ -41,34 +43,29 @@ test.afterEach((ctx) => {
 
 describe('constructor', () => {
   test('should initialize with agent config', (t) => {
-    const { agent } = t.nr
-    const profilingManager = new ProfilingManager(agent)
+    const { profilingManager } = t.nr
 
     assert.ok(profilingManager.config, 'should have config')
     assert.deepStrictEqual(profilingManager.config, t.nr.agent.config.profiling, 'should store agent config')
   })
 
   test('should initialize empty profilers map', (t) => {
-    const { agent } = t.nr
-    const profilingManager = new ProfilingManager(agent)
+    const { profilingManager } = t.nr
 
     assert.strictEqual(profilingManager.profilers.size, 0, 'profilers map should be empty')
   })
 })
 
 describe('register', () => {
   test('should be a no-op by default', (t) => {
-    const { agent } = t.nr
-    const profilingManager = new ProfilingManager(agent)
-
+    const { profilingManager } = t.nr
     profilingManager.register()
 
     assert.strictEqual(profilingManager.profilers.size, 0, 'should not add any profilers')
   })
 
   test('should register the heap and cpu profilers only once', (t) => {
-    const { agent } = t.nr
-    const profilingManager = new ProfilingManager(agent)
+    const { profilingManager } = t.nr
     profilingManager.config.include = ['heap']
 
     profilingManager.register()
@@ -85,8 +82,7 @@ describe('register', () => {
 
 describe('start', () => {
   test('should warn when no profilers are registered', (t) => {
-    const { agent, logger } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { profilingManager, logger } = t.nr
 
     const started = profilingManager.start()
     assert.equal(started, false)
@@ -100,8 +96,7 @@ describe('start', () => {
   })
 
   test('should start all registered profilers', (t) => {
-    const { agent, cpuProfiler, heapProfiler, logger } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { cpuProfiler, heapProfiler, profilingManager, logger } = t.nr
     profilingManager.profilers.set('cpu', cpuProfiler)
     profilingManager.profilers.set('heap', heapProfiler)
     const started = profilingManager.start()
@@ -122,8 +117,7 @@ describe('start', () => {
 
 describe('stop', () => {
   test('should warn when no profilers are registered', (t) => {
-    const { agent, logger } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { logger, profilingManager } = t.nr
     profilingManager.stop()
 
     assert.equal(logger.warn.callCount, 1)
@@ -135,8 +129,7 @@ describe('stop', () => {
   })
 
   test('should stop all registered profilers', (t) => {
-    const { agent, cpuProfiler, heapProfiler, logger } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { cpuProfiler, heapProfiler, logger, profilingManager } = t.nr
     profilingManager.profilers.set('cpu', cpuProfiler)
     profilingManager.profilers.set('heap', heapProfiler)
     profilingManager.stop()
@@ -148,8 +141,7 @@ describe('stop', () => {
   })
 
   test('should log supportability metric for profiling duration', (t) => {
-    const { agent, cpuProfiler, heapProfiler, logger, sandbox } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { agent, profilingManager, cpuProfiler, heapProfiler, sandbox } = t.nr
     profilingManager.profilers.set('cpu', cpuProfiler)
     profilingManager.profilers.set('heap', heapProfiler)
 
@@ -168,8 +160,7 @@ describe('stop', () => {
 
 describe('collect', (t) => {
   test('should warn and return empty array when no profilers are registered', async (t) => {
-    const { agent, logger } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { profilingManager, logger } = t.nr
 
     const results = await profilingManager.collect()
     assert.equal(results.length, 0, 'should return empty array')
@@ -182,8 +173,7 @@ describe('collect', (t) => {
   })
 
   test('should collect data from all registered profilers', async (t) => {
-    const { agent, cpuProfiler, heapProfiler, logger } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { profilingManager, cpuProfiler, heapProfiler, logger } = t.nr
 
     const expectedCpuData = { type: 'cpu', data: Buffer.from('cpu-profile-data') }
     const expectedHeapData = { type: 'heap', data: Buffer.from('heap-profile-data') }
@@ -203,8 +193,7 @@ describe('collect', (t) => {
   })
 
   test('should handle profilers returning undefined or null', async (t) => {
-    const { agent, cpuProfiler, heapProfiler, logger } = t.nr
-    const profilingManager = new ProfilingManager(agent, { logger })
+    const { profilingManager, cpuProfiler, heapProfiler, logger } = t.nr
 
     const expectedCpuData = undefined
     const expectedHeapData = null