🐛 [RUM Profiler] Fix stateReason not updated on stop and cleanup tasks accumulation#4171
🐛 [RUM Profiler] Fix stateReason not updated on stop and cleanup tasks accumulation#4171thomasbertet wants to merge 3 commits intomainfrom
Conversation
stopProfilerInstance did not update stateReason when the profiler was already in stopped state. This meant calling stop() after SESSION_EXPIRED left stateReason as 'session-expired', causing SESSION_RENEWED to restart the profiler against the user's intent.
The array was never reset after forEach, causing stale cleanup function references to accumulate across start/stop cycles.
|
/to-staging |
|
View all feedbacks in Devflow UI.
Commit d1b7c242b7 will soon be integrated into staging-08.
Commit d1b7c242b7 has been merged into staging-08 in merge commit a92a03b2bf. Check out the triggered DDCI request. If you need to revert this integration, you can use the following command: |
| instance = { state: 'stopped', stateReason } | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
nitpick (non-blocking): This could be slightly refactored I think:
if (instance.state !== "running") {
if (
// If paused, profiler data was already collected during pause, just update state
instance.state === "paused" ||
// Update stateReason when already stopped and the user explicitly stops the profiler,
// so that SESSION_RENEWED does not override the user's intent.
(instance.state === "stopped" && stateReason === "stopped-by-user")
) {
instance = { state: "stopped", stateReason };
}
return;
}There was a problem hiding this comment.
Done in 9b7dcb8 — merged both branches into a single if (instance.state !== 'running') guard.
| // Wait a bit to ensure profiler doesn't restart | ||
| await new Promise((resolve) => setTimeout(resolve, 100)) | ||
|
|
There was a problem hiding this comment.
suggestion: Let's not wait for arbitrary amount of times in tests. Is it useful to wait at all since start is synchronous?
There was a problem hiding this comment.
Good call — removed the arbitrary wait since start() is synchronous. Done in 9b7dcb8.
Refactor stopProfilerInstance to merge paused and stopped early-return branches into a single guard. Remove arbitrary setTimeout wait in test since start() is synchronous.
Bundles Sizes Evolution
🚀 CPU PerformancePending... 🧠 Memory PerformancePending... |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 9b7dcb8 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
2 participants
Motivation
Two issues found in the profiler state machine after the recent sync state fix (#4152):
stopProfilerInstancedoes not updatestateReasonwhen the profiler is already instoppedstate. IfSESSION_EXPIREDfires first (settingstateReasontosession-expired), then the user callsstop(), the reason stayssession-expired. On the nextSESSION_RENEWED, the profiler restarts against the user's intent.globalCleanupTasksarray is never cleared afterforEachcleanup instopProfiling. Stale function references accumulate across start/stop cycles.Changes
stopProfilerInstance: when already stopped, updatestateReasonif the new reason isstopped-by-user, so thatSESSION_RENEWEDrespects the user's explicit stop.stopProfiling: resetglobalCleanupTasks.length = 0after invoking all cleanup functions.Test instructions
yarn test:unit --spec packages/rum/src/domain/profiling/profiler.spec.tsChecklist