Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 24 additions & 14 deletions packages/dd-trace/src/profiling/profiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 = []

Expand Down
6 changes: 3 additions & 3 deletions packages/dd-trace/src/profiling/profilers/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: why defer profiler creation here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It operates on the detached instance of EventSerializer, so any errors encountered should be nonfatal for the profiler operation. Since we no longer treat errors during encoding as fatal, moving it into encoding phase upholds the non-fatality of any errors that might arise. Yes, all it does is create a Profile object, and its constructor has not much in way of being able to throw errors, but on principle it's better it's there.

}

encode (profile) {
return pprof.encode(profile)
return pprof.encode(profile())
}
}

Expand Down
46 changes: 33 additions & 13 deletions packages/dd-trace/test/profiling/profiler.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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 () => {
Expand Down Expand Up @@ -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 })

Expand Down
Loading