Remove full directories when deleting over-quota archived pings

NEW
Assigned to

Status

()

P3
normal
4 years ago
2 hours ago

People

(Reporter: Dexter, Assigned: kaioaugusto.8, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected)

Details

(Whiteboard: [measurement:client][lang=js][good next bug])

Attachments

(1 attachment, 1 obsolete attachment)

59 bytes, text/x-review-board-request
Details
(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 1120356
Depends on: 1162538
Blocks: 1122482
No longer blocks: 1120356
Whiteboard: [b5] [unifiedTelemetry]
Whiteboard: [b5] [unifiedTelemetry]
(Reporter)

Updated

3 years ago
Blocks: 1201022
No longer blocks: 1122482
(Reporter)

Updated

3 years ago
Priority: -- → P4
Whiteboard: [measurement:client]
Assignee: nobody → amiyaguchi
I'll take this one. Is `tests/unit/test_TelemetryController.js` the appropriate place to put unit tests for pruning old archived pings?
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 2

2 years ago
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
Flags: needinfo?(alessio.placitelli)
Anthony, are you working on this?
Flags: needinfo?(amiyaguchi)
I'm not currently active on this bug, I'll unassign myself to reflect that.
Assignee: amiyaguchi → nobody
Flags: needinfo?(amiyaguchi)
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?
Flags: needinfo?(existential.defeat)

Comment 7

2 years ago
(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!
Flags: needinfo?(existential.defeat) → needinfo?(gfritzsche)
Assignee: nobody → existential.defeat
Flags: needinfo?(gfritzsche)

Comment 8

2 years ago
Created attachment 8851563 [details] [diff] [review]
added a clause for deleting archive directory
Attachment #8851563 - Flags: review?(alessio.placitelli)
(Reporter)

Comment 9

2 years ago
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! ;)
Attachment #8851563 - Flags: review?(alessio.placitelli)
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Attachment #8851563 - Attachment is obsolete: true
(Reporter)

Comment 11

2 years ago
mozreview-review
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*.
Attachment #8852169 - Flags: review?(alessio.placitelli)

Updated

28 days ago
Assignee: existential.defeat → nobody
Mentor: alessio.placitelli
Whiteboard: [measurement:client] → [measurement:client][lang=js][good next bug]

Updated

28 days ago
Priority: P4 → P3
(Assignee)

Comment 12

16 hours ago

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

(Reporter)

Comment 13

2 hours ago

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

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

Sure, please do!

Assignee: nobody → kaioaugusto.8
You need to log in before you can comment on or make changes to this bug.