Don't stop the profiler if encoding a profile fails (#4779)

DataDog · Oct 16, 2024 · 59eb9a7 · 59eb9a7
1 parent 8969e05
commit 59eb9a7
Show file tree

Hide file tree

Showing 3 changed files with 60 additions and 30 deletions.
diff --git a/packages/dd-trace/src/profiling/profiler.js b/packages/dd-trace/src/profiling/profiler.js
@@ -146,6 +146,10 @@ class Profiler extends EventEmitter {
     const encodedProfiles = {}
 
     try {
+      if (Object.keys(this._config.profilers).length === 0) {
+        throw new Error('No profile types configured.')
+      }
+
       // collect profiles synchronously so that profilers can be safely stopped asynchronously
       for (const profiler of this._config.profilers) {
         const profile = profiler.profile(restart, startDate, endDate)
@@ -156,33 +160,39 @@ class Profiler extends EventEmitter {
         profiles.push({ profiler, profile })
       }
 
+      if (restart) {
+        this._capture(this._timeoutInterval, endDate)
+      }
+
       // encode and export asynchronously
       for (const { profiler, profile } of profiles) {
-        encodedProfiles[profiler.type] = await profiler.encode(profile)
-        this._logger.debug(() => {
-          const profileJson = JSON.stringify(profile, (key, value) => {
-            return typeof value === 'bigint' ? value.toString() : value
+        try {
+          encodedProfiles[profiler.type] = await profiler.encode(profile)
+          this._logger.debug(() => {
+            const profileJson = JSON.stringify(profile, (key, value) => {
+              return typeof value === 'bigint' ? value.toString() : value
+            })
+            return `Collected ${profiler.type} profile: ` + profileJson
           })
-          return `Collected ${profiler.type} profile: ` + profileJson
-        })
+        } catch (err) {
+          // If encoding one of the profile types fails, we should still try to
+          // encode and submit the other profile types.
+          this._logError(err)
+        }
       }
 
-      if (restart) {
-        this._capture(this._timeoutInterval, endDate)
+      if (Object.keys(encodedProfiles).length > 0) {
+        await this._submit(encodedProfiles, startDate, endDate, snapshotKind)
+        profileSubmittedChannel.publish()
+        this._logger.debug('Submitted profiles')
       }
-      await this._submit(encodedProfiles, startDate, endDate, snapshotKind)
-      profileSubmittedChannel.publish()
-      this._logger.debug('Submitted profiles')
     } catch (err) {
       this._logError(err)
       this._stop()
     }
   }
 
   _submit (profiles, start, end, snapshotKind) {
-    if (!Object.keys(profiles).length) {
-      return Promise.reject(new Error('No profiles to submit'))
-    }
     const { tags } = this._config
     const tasks = []
 

diff --git a/packages/dd-trace/src/profiling/profilers/events.js b/packages/dd-trace/src/profiling/profilers/events.js
@@ -330,13 +330,13 @@ class EventsProfiler {
     if (!restart) {
       this.stop()
     }
-    const profile = this.eventSerializer.createProfile(startDate, endDate)
+    const thatEventSerializer = this.eventSerializer
     this.eventSerializer = new EventSerializer()
-    return profile
+    return () => thatEventSerializer.createProfile(startDate, endDate)
   }
 
   encode (profile) {
-    return pprof.encode(profile)
+    return pprof.encode(profile())
   }
 }
 

diff --git a/packages/dd-trace/test/profiling/profiler.spec.js b/packages/dd-trace/test/profiling/profiler.spec.js
@@ -181,6 +181,21 @@ describe('profiler', function () {
     })
 
     it('should stop when capturing failed', async () => {
+      wallProfiler.profile.throws(new Error('boom'))
+
+      await profiler._start({ profilers, exporters, logger })
+
+      clock.tick(interval)
+
+      sinon.assert.calledOnce(wallProfiler.stop)
+      sinon.assert.calledOnce(spaceProfiler.stop)
+      sinon.assert.calledOnce(consoleLogger.error)
+      sinon.assert.notCalled(wallProfiler.encode)
+      sinon.assert.notCalled(spaceProfiler.encode)
+      sinon.assert.notCalled(exporter.export)
+    })
+
+    it('should not stop when encoding failed', async () => {
       const rejected = Promise.reject(new Error('boom'))
       wallProfiler.encode.returns(rejected)
 
@@ -190,9 +205,25 @@ describe('profiler', function () {
 
       await rejected.catch(() => {})
 
-      sinon.assert.calledOnce(wallProfiler.stop)
-      sinon.assert.calledOnce(spaceProfiler.stop)
+      sinon.assert.notCalled(wallProfiler.stop)
+      sinon.assert.notCalled(spaceProfiler.stop)
       sinon.assert.calledOnce(consoleLogger.error)
+      sinon.assert.calledOnce(exporter.export)
+    })
+
+    it('should not stop when exporting failed', async () => {
+      const rejected = Promise.reject(new Error('boom'))
+      exporter.export.returns(rejected)
+
+      await profiler._start({ profilers, exporters, logger })
+
+      clock.tick(interval)
+
+      await rejected.catch(() => {})
+
+      sinon.assert.notCalled(wallProfiler.stop)
+      sinon.assert.notCalled(spaceProfiler.stop)
+      sinon.assert.calledOnce(exporter.export)
     })
 
     it('should flush when the interval is reached', async () => {
@@ -270,17 +301,6 @@ describe('profiler', function () {
       sinon.assert.calledWithMatch(submit, 'Submitted profiles')
     })
 
-    it('should skip submit with no profiles', async () => {
-      const start = new Date()
-      const end = new Date()
-      try {
-        await profiler._submit({}, start, end)
-        throw new Error('should have got exception from _submit')
-      } catch (err) {
-        expect(err.message).to.equal('No profiles to submit')
-      }
-    })
-
     it('should have a new start time for each capture', async () => {
       await profiler._start({ profilers, exporters })