RUM-13679: Delete Perfetto result file after writing to batch - #3486
Conversation
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Fixes a disk space leak in the continuous profiling pipeline by moving Perfetto trace file lifecycle management into ProfilingDataWriter (read → write to batch → delete), and adjusting ProfilingFeature behavior/logging so continuous results with no pending RUM events still trigger cleanup.
Changes:
ProfilingFeaturenow always forwards continuous profiling results to the writer (even with empty pending RUM events) and logs “not uploaded” instead of “dropped”.ProfilingDataWriterdeletes the Perfetto result file after the write path completes (and when the profiling feature isn’t available).- Tests updated/added to assert file deletion and updated behavior/log messages.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| features/dd-sdk-android-profiling/src/main/java/com/datadog/android/profiling/internal/ProfilingFeature.kt | Always writes continuous results (enabling cleanup) and adjusts user log messaging. |
| features/dd-sdk-android-profiling/src/main/java/com/datadog/android/profiling/internal/ProfilingDataWriter.kt | Implements file deletion as part of the writer’s lifecycle responsibilities. |
| features/dd-sdk-android-profiling/src/test/kotlin/com/datadog/android/profiling/ProfilingFeatureTest.kt | Updates test expectation: writer invoked even when no pending RUM events; asserts new log message. |
| features/dd-sdk-android-profiling/src/test/kotlin/com/datadog/android/profiling/internal/ProfilingDataWriterTest.kt | Adds assertions for Perfetto file deletion across multiple scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| feature.withWriteContext { context, writeScope -> | ||
| writeScope { writer -> | ||
| synchronized(this) { | ||
| buildRawBatchEvent( | ||
| context = context, | ||
| profilingResult = profilingResult, | ||
| longTaskEvents = longTasks, | ||
| anrEvents = anrEvents, | ||
| vitalEvents = vitalEvents | ||
| )?.let { | ||
| writer.write( | ||
| event = it, | ||
| batchMetadata = null, | ||
| eventType = EventType.DEFAULT | ||
| ) | ||
| } | ||
| safeDelete(profilingResult.resultFilePath) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Currently we don't have a retry mechanism for the writing failure, it will be implemented later as described above, in this case, instead of filling up the user's storage, we need to delete the file even though the write fails.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/continuous-profiling #3486 +/- ##
================================================================
+ Coverage 72.36% 72.40% +0.04%
================================================================
Files 979 979
Lines 36242 36258 +16
Branches 6034 6035 +1
================================================================
+ Hits 26225 26250 +25
+ Misses 8359 8355 -4
+ Partials 1658 1653 -5
🚀 New features to boost your workflow:
|
What does this PR do?
Fixes a disk space leak in the continuous profiling pipeline (RUM-13679).
ProfilingManagerwrites profile to the app's data directory. Before this change, that file was never deleted.ProfilingDataWriternow owns the full file lifecycle: read → batch write → delete.The file deletion will happen in such cases:
The file will be kept in these cases:
Next steps
The orphaned-file problem (process kill, SDK teardown race) requires a complementary retry mechanism later
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)