1168728 - Remove full directories when deleting over-quota archived pings

Status: NEW

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

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.

Comment 1

[Anthony Miyaguchi [:amiyaguchi]]

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

Comment 2

[Alessio Placitelli [:Dexter]]

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

Comment 3

[Georg Fritzsche [:gfritzsche]]

Anthony, are you working on this?

Comment 4

[Anthony Miyaguchi [:amiyaguchi]]

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

Comment 5

[Georg Fritzsche [:gfritzsche]]

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

Comment 6

[Georg Fritzsche [:gfritzsche]]

Daria, would you be interested in this bug?

Comment 7

[Daria]

(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 8

[Daria]

Attachment: added a clause for deleting archive directory

Comment 9

[Alessio Placitelli [:Dexter]]

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 10

[Daria]

Attachment: Bug 1168728

Review commit: https://reviewboard.mozilla.org/r/124392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124392/

Comment 11

[Alessio Placitelli [:Dexter]]

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*.

Comment 12

[Kaio Augusto de Camargo]

Can I take this bug? I'd love to help!

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Kaio Augusto de Camargo from comment #12)

Can I take this bug? I'd love to help!

Sure, please do!

Comment 14

[Alessio Placitelli [:Dexter]]

Unassigning due to inactivity.

Comment 15

[Jason Ashton]

I'd like to take this on!