Make sure space profiler (almost) always goes first (#6808)

Author: szegedi

packages/dd-trace/src/profiling/config.js

@@ -269,29 +269,51 @@ module.exports = { Config }
 function getProfilers ({
   DD_PROFILING_HEAP_ENABLED, DD_PROFILING_WALLTIME_ENABLED, DD_PROFILING_PROFILERS
 }) {
-  // First consider "legacy" DD_PROFILING_PROFILERS env variable, defaulting to wall + space
+  // First consider "legacy" DD_PROFILING_PROFILERS env variable, defaulting to space + wall
   // Use a Set to avoid duplicates
-  const profilers = new Set((DD_PROFILING_PROFILERS ?? 'wall,space').split(','))
+  // NOTE: space profiler is very deliberately in the first position. This way
+  // when profilers are stopped sequentially one after the other to create
+  // snapshots the space profile won't include memory taken by profiles created
+  // before it in the sequence. That memory is ultimately transient and will be
+  // released when all profiles are subsequently encoded.
+  const profilers = new Set((DD_PROFILING_PROFILERS ?? 'space,wall').split(','))
+
+  let spaceExplicitlyEnabled = false
+  // Add/remove space depending on the value of DD_PROFILING_HEAP_ENABLED
+  if (DD_PROFILING_HEAP_ENABLED != null) {
+    if (isTrue(DD_PROFILING_HEAP_ENABLED)) {
+      if (!profilers.has('space')) {
+        profilers.add('space')
+        spaceExplicitlyEnabled = true
+      }
+    } else if (isFalse(DD_PROFILING_HEAP_ENABLED)) {
+      profilers.delete('space')
+    }
+  }
 
   // Add/remove wall depending on the value of DD_PROFILING_WALLTIME_ENABLED
   if (DD_PROFILING_WALLTIME_ENABLED != null) {
     if (isTrue(DD_PROFILING_WALLTIME_ENABLED)) {
       profilers.add('wall')
     } else if (isFalse(DD_PROFILING_WALLTIME_ENABLED)) {
       profilers.delete('wall')
+      profilers.delete('cpu') // remove alias too
     }
   }
 
-  // Add/remove wall depending on the value of DD_PROFILING_HEAP_ENABLED
-  if (DD_PROFILING_HEAP_ENABLED != null) {
-    if (isTrue(DD_PROFILING_HEAP_ENABLED)) {
-      profilers.add('space')
-    } else if (isFalse(DD_PROFILING_HEAP_ENABLED)) {
-      profilers.delete('space')
+  const profilersArray = [...profilers]
+  // If space was added through DD_PROFILING_HEAP_ENABLED, ensure it is in the
+  // first position. Basically, the only way for it not to be in the first
+  // position is if it was explicitly specified in a different position in
+  // DD_PROFILING_PROFILERS.
+  if (spaceExplicitlyEnabled) {
+    const spaceIdx = profilersArray.indexOf('space')
+    if (spaceIdx > 0) {
+      profilersArray.splice(spaceIdx, 1)
+      profilersArray.unshift('space')
     }
   }
-
-  return [...profilers]
+  return profilersArray
 }
 
 function getExportStrategy (name, options) {

packages/dd-trace/test/profiling/config.spec.js

@@ -52,9 +52,9 @@ describe('config', () => {
 
     expect(config.logger).to.be.an.instanceof(ConsoleLogger)
     expect(config.exporters[0]).to.be.an.instanceof(AgentExporter)
-    expect(config.profilers[0]).to.be.an.instanceof(WallProfiler)
-    expect(config.profilers[0].codeHotspotsEnabled()).to.equal(samplingContextsAvailable)
-    expect(config.profilers[1]).to.be.an.instanceof(SpaceProfiler)
+    expect(config.profilers[0]).to.be.an.instanceof(SpaceProfiler)
+    expect(config.profilers[1]).to.be.an.instanceof(WallProfiler)
+    expect(config.profilers[1].codeHotspotsEnabled()).to.equal(samplingContextsAvailable)
     expect(config.v8ProfilerBugWorkaroundEnabled).true
     expect(config.cpuProfilingEnabled).to.equal(samplingContextsAvailable)
     expect(config.uploadCompression.method).to.equal('gzip')
@@ -177,6 +177,40 @@ describe('config', () => {
     expect(config.profilers[0]).to.be.an.instanceOf(SpaceProfiler)
   })
 
+  it('should ensure space profiler is ordered first with DD_PROFILING_HEAP_ENABLED', () => {
+    process.env = {
+      DD_PROFILING_PROFILERS: 'wall',
+      DD_PROFILING_HEAP_ENABLED: '1'
+    }
+    const options = {
+      logger: nullLogger
+    }
+
+    const config = new Config(options)
+
+    expect(config.profilers).to.be.an('array')
+    expect(config.profilers.length).to.equal(2 + (samplingContextsAvailable ? 1 : 0))
+    expect(config.profilers[0]).to.be.an.instanceOf(SpaceProfiler)
+    expect(config.profilers[1]).to.be.an.instanceOf(WallProfiler)
+  })
+
+  it('should ensure space profiler order is preserved when explicitly set with DD_PROFILING_PROFILERS', () => {
+    process.env = {
+      DD_PROFILING_PROFILERS: 'wall,space',
+      DD_PROFILING_HEAP_ENABLED: '1'
+    }
+    const options = {
+      logger: nullLogger
+    }
+
+    const config = new Config(options)
+
+    expect(config.profilers).to.be.an('array')
+    expect(config.profilers.length).to.equal(2 + (samplingContextsAvailable ? 1 : 0))
+    expect(config.profilers[0]).to.be.an.instanceOf(WallProfiler)
+    expect(config.profilers[1]).to.be.an.instanceOf(SpaceProfiler)
+  })
+
   it('should be able to read some env vars', () => {
     const oldenv = process.env
     process.env = {