Disable OOM monitoring by default on Windows (DataDog#3974)

OOM monitoring currently does not work on Windows (process aborts before being able to send a last profile). This commit disables it by default on Windows.
mhazy · Jan 18, 2024 · cbdab26 · cbdab26
1 parent 81e3d68
commit cbdab26
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 62 deletions.
diff --git a/integration-tests/profiler/profiler.spec.js b/integration-tests/profiler/profiler.spec.js
@@ -360,9 +360,7 @@ describe('profiler', () => {
         })
         return checkProfiles(agent, proc, timeout, ['space'], true)
       })
-    }
 
-    if (process.platform !== 'win32') { // PROF-8905
       it('sends a heap profile on OOM with external process and exits successfully', async () => {
         proc = fork(oomTestFile, {
           cwd,
@@ -375,23 +373,21 @@ describe('profiler', () => {
         })
         return checkProfiles(agent, proc, timeout, ['space'], false, 2)
       })
-    }
 
-    it('sends a heap profile on OOM with async callback', async () => {
-      proc = fork(oomTestFile, {
-        cwd,
-        execArgv: oomExecArgv,
-        env: {
-          ...oomEnv,
-          DD_PROFILING_EXPERIMENTAL_OOM_HEAP_LIMIT_EXTENSION_SIZE: 10000000,
-          DD_PROFILING_EXPERIMENTAL_OOM_MAX_HEAP_EXTENSION_COUNT: 1,
-          DD_PROFILING_EXPERIMENTAL_OOM_EXPORT_STRATEGIES: 'async'
-        }
+      it('sends a heap profile on OOM with async callback', async () => {
+        proc = fork(oomTestFile, {
+          cwd,
+          execArgv: oomExecArgv,
+          env: {
+            ...oomEnv,
+            DD_PROFILING_EXPERIMENTAL_OOM_HEAP_LIMIT_EXTENSION_SIZE: 10000000,
+            DD_PROFILING_EXPERIMENTAL_OOM_MAX_HEAP_EXTENSION_COUNT: 1,
+            DD_PROFILING_EXPERIMENTAL_OOM_EXPORT_STRATEGIES: 'async'
+          }
+        })
+        return checkProfiles(agent, proc, timeout, ['space'], true)
       })
-      return checkProfiles(agent, proc, timeout, ['space'], true)
-    })
 
-    if (process.platform !== 'win32') { // PROF-8905
       it('sends heap profiles on OOM with multiple strategies', async () => {
         proc = fork(oomTestFile, {
           cwd,
@@ -405,9 +401,7 @@ describe('profiler', () => {
         })
         return checkProfiles(agent, proc, timeout, ['space'], true, 2)
       })
-    }
 
-    if (process.platform !== 'win32') { // PROF-8905
       it('sends a heap profile on OOM in worker thread and exits successfully', async () => {
         proc = fork(oomTestFile, [1, 50], {
           cwd,

diff --git a/packages/dd-trace/src/profiling/config.js b/packages/dd-trace/src/profiling/config.js
@@ -97,11 +97,15 @@ class Config {
     // depending on those (code hotspots and endpoint collection) need to default
     // to false on Windows.
     const samplingContextsAvailable = process.platform !== 'win32'
-    function checkOptionAllowed (option, description) {
-      if (option && !samplingContextsAvailable) {
+    function checkOptionAllowed (option, description, condition) {
+      if (option && !condition) {
         throw new Error(`${description} not supported on ${process.platform}.`)
       }
     }
+    function checkOptionWithSamplingContextAllowed (option, description) {
+      checkOptionAllowed(option, description, samplingContextsAvailable)
+    }
+
     this.flushInterval = flushInterval
     this.uploadTimeout = uploadTimeout
     this.sourceMap = sourceMap
@@ -110,7 +114,7 @@ class Config {
       DD_PROFILING_ENDPOINT_COLLECTION_ENABLED,
       DD_PROFILING_EXPERIMENTAL_ENDPOINT_COLLECTION_ENABLED, samplingContextsAvailable))
     logExperimentalVarDeprecation('ENDPOINT_COLLECTION_ENABLED')
-    checkOptionAllowed(this.endpointCollectionEnabled, 'Endpoint collection')
+    checkOptionWithSamplingContextAllowed(this.endpointCollectionEnabled, 'Endpoint collection')
 
     this.pprofPrefix = pprofPrefix
     this.v8ProfilerBugWorkaroundEnabled = isTrue(coalesce(options.v8ProfilerBugWorkaround,
@@ -127,8 +131,13 @@ class Config {
       new AgentExporter(this)
     ], this)
 
+    // OOM monitoring does not work well on Windows, so it is disabled by default.
+    const oomMonitoringSupported = process.platform !== 'win32'
+
     const oomMonitoringEnabled = isTrue(coalesce(options.oomMonitoring,
-      DD_PROFILING_EXPERIMENTAL_OOM_MONITORING_ENABLED, true))
+      DD_PROFILING_EXPERIMENTAL_OOM_MONITORING_ENABLED, oomMonitoringSupported))
+    checkOptionWithSamplingContextAllowed(oomMonitoringEnabled, 'OOM monitoring', oomMonitoringSupported)
+
     const heapLimitExtensionSize = coalesce(options.oomHeapLimitExtensionSize,
       Number(DD_PROFILING_EXPERIMENTAL_OOM_HEAP_LIMIT_EXTENSION_SIZE), 0)
     const maxHeapExtensionCount = coalesce(options.oomMaxHeapExtensionCount,
@@ -158,17 +167,17 @@ class Config {
       DD_PROFILING_TIMELINE_ENABLED,
       DD_PROFILING_EXPERIMENTAL_TIMELINE_ENABLED, false))
     logExperimentalVarDeprecation('TIMELINE_ENABLED')
-    checkOptionAllowed(this.timelineEnabled, 'Timeline view')
+    checkOptionWithSamplingContextAllowed(this.timelineEnabled, 'Timeline view')
 
     this.codeHotspotsEnabled = isTrue(coalesce(options.codeHotspotsEnabled,
       DD_PROFILING_CODEHOTSPOTS_ENABLED,
       DD_PROFILING_EXPERIMENTAL_CODEHOTSPOTS_ENABLED, samplingContextsAvailable))
     logExperimentalVarDeprecation('CODEHOTSPOTS_ENABLED')
-    checkOptionAllowed(this.codeHotspotsEnabled, 'Code hotspots')
+    checkOptionWithSamplingContextAllowed(this.codeHotspotsEnabled, 'Code hotspots')
 
     this.cpuProfilingEnabled = isTrue(coalesce(options.cpuProfilingEnabled,
       DD_PROFILING_EXPERIMENTAL_CPU_ENABLED, false))
-    checkOptionAllowed(this.cpuProfilingEnabled, 'CPU profiling')
+    checkOptionWithSamplingContextAllowed(this.cpuProfilingEnabled, 'CPU profiling')
 
     this.profilers = ensureProfilers(profilers, this)
   }

diff --git a/packages/dd-trace/test/profiling/config.spec.js b/packages/dd-trace/test/profiling/config.spec.js
@@ -12,6 +12,7 @@ const SpaceProfiler = require('../../src/profiling/profilers/space')
 const { ConsoleLogger } = require('../../src/profiling/loggers/console')
 
 const samplingContextsAvailable = process.platform !== 'win32'
+const oomMonitoringSupported = process.platform !== 'win32'
 
 describe('config', () => {
   let Config
@@ -250,12 +251,12 @@ describe('config', () => {
     expect(config.profilers[0].endpointCollectionEnabled()).false
   })
 
-  function optionOnlyWorksWithSamplingContexts (property, name) {
+  function optionOnlyWorksWithGivenCondition (property, name, condition) {
     const options = {
       [property]: true
     }
 
-    if (samplingContextsAvailable) {
+    if (condition) {
       // should silently succeed
       // eslint-disable-next-line no-new
       new Config(options)
@@ -266,6 +267,10 @@ describe('config', () => {
     }
   }
 
+  function optionOnlyWorksWithSamplingContexts (property, name) {
+    optionOnlyWorksWithGivenCondition(property, name, samplingContextsAvailable)
+  }
+
   it('should only allow code hotspots on supported platforms', () => {
     optionOnlyWorksWithSamplingContexts('codeHotspotsEnabled', 'Code hotspots')
   })
@@ -282,6 +287,10 @@ describe('config', () => {
     optionOnlyWorksWithSamplingContexts('timelineEnabled', 'Timeline view')
   })
 
+  it('should only allow OOM monitoring on supported platforms', () => {
+    optionOnlyWorksWithGivenCondition('oomMonitoring', 'OOM monitoring', oomMonitoringSupported)
+  })
+
   it('should support tags', () => {
     const tags = {
       env: 'dev'
@@ -349,43 +358,49 @@ describe('config', () => {
   it('should enable OOM heap profiler by default and use process as default strategy', () => {
     const config = new Config()
 
-    expect(config.oomMonitoring).to.deep.equal({
-      enabled: true,
-      heapLimitExtensionSize: 0,
-      maxHeapExtensionCount: 0,
-      exportStrategies: ['process'],
-      exportCommand: [
-        process.execPath,
-        path.normalize(path.join(__dirname, '../../src/profiling', 'exporter_cli.js')),
-        'http://localhost:8126/',
-        `host:${config.host},service:node,snapshot:on_oom`,
-        'space'
-      ]
-    })
-  })
-
-  it('should support OOM heap profiler configuration', () => {
-    process.env = {
-      DD_PROFILING_EXPERIMENTAL_OOM_MONITORING_ENABLED: '1',
-      DD_PROFILING_EXPERIMENTAL_OOM_HEAP_LIMIT_EXTENSION_SIZE: '1000000',
-      DD_PROFILING_EXPERIMENTAL_OOM_MAX_HEAP_EXTENSION_COUNT: '2',
-      DD_PROFILING_EXPERIMENTAL_OOM_EXPORT_STRATEGIES: 'process,async,process'
+    if (oomMonitoringSupported) {
+      expect(config.oomMonitoring).to.deep.equal({
+        enabled: oomMonitoringSupported,
+        heapLimitExtensionSize: 0,
+        maxHeapExtensionCount: 0,
+        exportStrategies: ['process'],
+        exportCommand: [
+          process.execPath,
+          path.normalize(path.join(__dirname, '../../src/profiling', 'exporter_cli.js')),
+          'http://localhost:8126/',
+          `host:${config.host},service:node,snapshot:on_oom`,
+          'space'
+        ]
+      })
+    } else {
+      expect(config.oomMonitoring.enabled).to.be.false
     }
+  })
 
-    const config = new Config({})
+  if (oomMonitoringSupported) {
+    it('should support OOM heap profiler configuration', function () {
+      process.env = {
+        DD_PROFILING_EXPERIMENTAL_OOM_MONITORING_ENABLED: '1',
+        DD_PROFILING_EXPERIMENTAL_OOM_HEAP_LIMIT_EXTENSION_SIZE: '1000000',
+        DD_PROFILING_EXPERIMENTAL_OOM_MAX_HEAP_EXTENSION_COUNT: '2',
+        DD_PROFILING_EXPERIMENTAL_OOM_EXPORT_STRATEGIES: 'process,async,process'
+      }
 
-    expect(config.oomMonitoring).to.deep.equal({
-      enabled: true,
-      heapLimitExtensionSize: 1000000,
-      maxHeapExtensionCount: 2,
-      exportStrategies: ['process', 'async'],
-      exportCommand: [
-        process.execPath,
-        path.normalize(path.join(__dirname, '../../src/profiling', 'exporter_cli.js')),
-        'http://localhost:8126/',
-        `host:${config.host},service:node,snapshot:on_oom`,
-        'space'
-      ]
+      const config = new Config({})
+
+      expect(config.oomMonitoring).to.deep.equal({
+        enabled: true,
+        heapLimitExtensionSize: 1000000,
+        maxHeapExtensionCount: 2,
+        exportStrategies: ['process', 'async'],
+        exportCommand: [
+          process.execPath,
+          path.normalize(path.join(__dirname, '../../src/profiling', 'exporter_cli.js')),
+          'http://localhost:8126/',
+          `host:${config.host},service:node,snapshot:on_oom`,
+          'space'
+        ]
+      })
     })
-  })
+  }
 })