In |TelemetryStorage._enforceArchiveQuota| over quota pings are deleted from the disk one by one. If all the pings within an archive directory should be deleted, it might be faster to just remove the whole directory.

I'll take this one. Is `tests/unit/test_TelemetryController.js` the appropriate place to put unit tests for pruning old archived pings?

Thanks Anthony! The other test that covers archive cleanup lives in test_PingAPI.js, so you could add yours there [1] too. [1] - http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/toolkit/components/telemetry/tests/unit/test_PingAPI.js#191

Anthony, are you working on this?

I'm not currently active on this bug, I'll unassign myself to reflect that.

This is the function in which the changes need to be made: https://dxr.mozilla.org/mozilla-central/rev/39607304b774591fa6e32c4b06158d869483c312/toolkit/components/telemetry/TelemetryStorage.jsm#907 Inside the profile directory, the archive is organized into monthly directories organized: > datareporting/archived/YYYY-MM/ In these, we store individual ping files (<uuid>.<type>.jsonlz4). When all of the ping files in a directory need to be removed, we should just remove the directory instead. This test should first pass with the changes: > mach test test_PingAPI.js After that works, make sure that the rest of the telemetry tests is also passing: > mach test toolkit/components/telemetry/tests/unit

Daria, would you be interested in this bug?

(In reply to Georg Fritzsche [:gfritzsche] [at workweek] from comment #6) > Daria, would you be interested in this bug? Yes, I will try to fix it!

Comment on attachment 8851563 [details] [diff] [review] added a clause for deleting archive directory Hi Daria! In order for me to review your changes, you should submit a patch file. You can find out how to do that by reading: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F Please use MozReview to submit your commit! ;)

Comment on attachment 8852169 [details] Bug 1168728 https://reviewboard.mozilla.org/r/124392/#review127138 This looks like it's removing the whole ping archive, which is undesired. Please check again comment 5 for explainations on what you should be doing and feel free to ask questions if anything is unclear! Make sure to run the test comand before submitting to next patch. All the tests need to pass. In addition to what Georg said, I'd suggest you to run the linting command as well: > ./mach eslint toolkit/components/telemetry This would help you figuring out style issues. ::: commit-message-dd2f2:1 (Diff revision 1) > +Bug 1168728 Please provide a useful commit message. For example, you could use "Bug 1168728 - Remove full directories when deleting over-quota archived pings. r?dexter". See [here](https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions) for details. ::: toolkit/components/telemetry/TelemetryStorage.jsm:992 (Diff revision 1) > startTimeStamp = Policy.now().getTime(); > - let pingsToPurge = pingList.slice(lastPingIndexToKeep + 1); > > + // If the last one is newest and it's outdated, the rest are as well, so we remove the whole directory > + if (pingList.length-1==lastPingIndexToKeep){ > + this._log.trace("_removeArchivedPing - removing directory from: " + gPingsArchivePath); Please try to explain what you're trying to achieve using comments. ::: toolkit/components/telemetry/TelemetryStorage.jsm:993 (Diff revision 1) > - let pingsToPurge = pingList.slice(lastPingIndexToKeep + 1); > > + // If the last one is newest and it's outdated, the rest are as well, so we remove the whole directory > + if (pingList.length-1==lastPingIndexToKeep){ > + this._log.trace("_removeArchivedPing - removing directory from: " + gPingsArchivePath); > + this._archivedPings.clear(); This is removing the whole ping archive directory, which is wrong. As described in comment 5, the pings are stored in subdirectories within the archive, per month. If all the pings for a particular month must be removed then, instead of removing each individual ping, we can remove the whole directory *for that month*.