Closed Bug 1120380 Opened 9 years ago Closed 9 years ago

Update retention logic for Telemetry pings

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set
normal

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: gfritzsche, Assigned: Dexter)

References

Details

(Whiteboard: [uplift])

Attachments

(3 files, 8 obsolete files)

22.50 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
5.73 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
15.04 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
We should not lose all ping data even in case of crashes.
To do that we should consider caching the ping data on disk regularly and recover that on startup.
Blocks: 1122481
Summary: Implement Telemetry ping caching → Update retention logic for Telemetry pings
No longer blocks: 1120356
Isn't this a duplicate of bug 1133536?
No, this one was about retaining different kind of pings with their respective retention period (and the description here was wrong).
Benjamin, do we even need to retain any other ping types (other then the Telemetry session ping) on disk after sending them? (e.g. Loop ping etc.)
Flags: needinfo?(benjamin)
Yes.
Flags: needinfo?(benjamin)
For now we want to default to retaining all ping types for 180 days.
Lets go with that for now and worry about implementing more generic ping retention solutions later.
Blocks: 1120356
No longer blocks: 1122481
Depends on: 1137252
Bug 1150134 already adds scanning the archived pings, we should base this bug on it.

Here we have to:
* make sure to remove pings after the retention period
* update the TelemetryPing interfaces (they talk of 14 days and mention retention period options)
Depends on: 1150134
No longer depends on: 1137252
Depends on: 1137252
Assignee: nobody → alessio.placitelli
As discussed over IRC, we also need to add a histogram for the archive size as part of this bug.
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> Here we have to:
> * make sure to remove pings after the retention period

This is affected by the first parts of this implementation design discussion:
https://docs.google.com/document/d/19tBB2SYfO3Q4wglq32CLyT4BVJZbA7aHjEshXCLV9_4/edit

The parts about the storage quota will be handled by a different bug.
Blocks: 1162538
This patch:

- Moves the ping cache to TelemetryStorage.
- Introduces a cleanup task which purges directories older than 180 days.
- Updates the ping cache.
Attachment #8603398 - Flags: review?(gfritzsche)
Attachment #8603399 - Flags: review?(gfritzsche)
Status: NEW → ASSIGNED
Whiteboard: [uplift]
Comment on attachment 8603398 [details] [diff] [review]
part 1 - Move the archived pings cache to TelemetryStorage and enable pruning

Review of attachment 8603398 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryArchive.jsm
@@ +89,5 @@
>      return this._logger;
>    },
>  
>    _testReset: function() {
> +    return TelemetryStorage.invalidateArchiveCache();

Can't we just have TelemetryController.reset() call this now?
I think there is no need to have anything exposed on TelemetryArchive now.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +996,5 @@
>  
> +        // Purge the pings archive by removing outdated pings. We don't wait for this
> +        // task to complete, but block on it during shutdown.
> +        let cleanupPromise = TelemetryStorage.cleanPingsArchive();
> +        this._shutdownBarrier.client.addBlocker("Waiting for archive cleanup", cleanupPromise);

We already do call from TelemetryController shutdown |TelemetryStorage.shutdown()|, seems like that function should just handle it.

@@ +1000,5 @@
> +        this._shutdownBarrier.client.addBlocker("Waiting for archive cleanup", cleanupPromise);
> +
> +        // When testing wait for the archive cleanup to complete.
> +        if (this._testMode) {
> +          yield cleanupPromise;

Why do we need this?
If any tests really need to explicitly wait for a running cleanup task to finish (why?), they can block on TelemetryStorage.testCleanupTaskPromise or something.

This should not affect anything else, otherwise i think we have a bug or should fix the tests.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +114,5 @@
> +   * This will scan the archive directory.
> +   *
> +   * @return {Promise} Resolved when the cleanup task completes.
> +   */
> +   cleanPingsArchive: function() {

runCleanPingArchiveTask()?

@@ +115,5 @@
> +   *
> +   * @return {Promise} Resolved when the cleanup task completes.
> +   */
> +   cleanPingsArchive: function() {
> +    return TelemetryStorageImpl.cleanPingsArchive();

Nit: indentation is off.

@@ +122,5 @@
> +  /**
> +   * Invalidate the archive cache, forcing the rescanning of the pings archive once
> +   * |loadArchivedPingList| is called again. This is used for testing by TelemetryArchive.
> +   */
> +  invalidateArchiveCache: function() {

Let's prefix this so it's clear that it's test-only.
_testInvalidateArchiveCache?

@@ +372,5 @@
> +   */
> +  _isOldArchiveDir: function(aDirName, aTodayTimestamp) {
> +    // Get a date object from an archived ping directory name.
> +    let [year, month] = aDirName.split("-");
> +    const dirDate = new Date(parseInt(year), parseInt(month) - 1, 1, 0, 0, 0);

What if aDirName is not in the format "YYYY-MM"?
At least its not documented as requiring that input.

@@ +384,5 @@
> +   * Removes a directory, logging failures.
> +   * @param {String} aPath The path to the directory to remove.
> +   * @return {Promise} A promise resolved when the directory is removed.
> +   */
> +  _trashDirectory: Task.async(function*(aPath) {

Why not just _removeDirectory()?
I only see one caller - why do we need this as a separate function instead of just doing |try { removeDir() } catch (ex) { log(..., ex) }| directly?

@@ +401,5 @@
> +   *
> +   * @return {Promise} Resolved when the cleanup task completes.
> +   */
> +  cleanPingsArchive: Task.async(function*() {
> +    this._log.trace("cleanPingsArchive");

We said in the implementation doc:
"the [cleanup] task should honor a shutdown flag and quickly bail out if that flag is set, to speed up shutdown".
I don't see this happening here.

@@ +407,5 @@
> +    if (!(yield OS.File.exists(gPingsArchivePath))) {
> +      return;
> +    }
> +
> +    const todayMs = Policy.now().getTime();

|today| seems ambigous, |now|?

@@ +414,5 @@
> +
> +    // Walk through the monthly subdirs of the form <YYYY-MM>/
> +    for (let dir of subdirs) {
> +      const dirRegEx = /^[0-9]{4}-[0-9]{2}$/;
> +      if (!dirRegEx.test(dir.name)) {

For readability, this might be nice to factor out into isValidArchiveDir() or so.

@@ +425,5 @@
> +        continue;
> +      }
> +
> +      // This archive directory is older than 180 days, remove it.
> +      yield this._trashDirectory(dir.path);

Might be clearer in one step above?
if (this._isOldArchiveDir()) {
  yield removeDir();
}

@@ +428,5 @@
> +      // This archive directory is older than 180 days, remove it.
> +      yield this._trashDirectory(dir.path);
> +    }
> +
> +    // If the archive directory was already scanned, filter the ping archive cache.

That is not in the implementation doc.
Vladan wanted to avoid the cost of removing individual pings by just throwing out monthly dirs unless we hit the storage quota.

@@ +466,5 @@
>     */
>    loadArchivedPingList: Task.async(function*() {
>      this._log.trace("loadArchivedPingList");
>  
> +    if (this._scannedArchiveDirectory) {

Hm, not really a new issue, but this has a race condition:
If a scanning task is already running when we call this, we start a new scan.
We should return a promise that is fulfilled when the already running task finished instead.
Attachment #8603398 - Flags: review?(gfritzsche)
Comment on attachment 8603399 [details] [diff] [review]
part 2 - Add test coverage for the archive cleanup

Review of attachment 8603399 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +18,5 @@
>  });
>  
> +// Write invalid pings into the archive with both valid and invalid names.
> +let writeToArchivedDir = Task.async(function*(dirname, filename, content) {
> +  const dirPath = OS.Path.join(gPingsArchivePath, dirname);

Comment before this?

@@ +20,5 @@
> +// Write invalid pings into the archive with both valid and invalid names.
> +let writeToArchivedDir = Task.async(function*(dirname, filename, content) {
> +  const dirPath = OS.Path.join(gPingsArchivePath, dirname);
> +  yield OS.File.makeDir(dirPath, { ignoreExisting: true });
> +  const filePath = OS.Path.join(dirPath, filename);

Blank line before this and a comment?

@@ +134,5 @@
> +  const FAKE_ID2 = "20000000-0123-0123-0123-0123456789a2";
> +  const FAKE_ID3 = "20000000-0123-0123-0123-0123456789a3";
> +  const FAKE_TYPE = "foo";
> +
> +  // This one should be pruned.

"Create a ping which should be pruned because it is past the retention period."

@@ +137,5 @@
> +
> +  // This one should be pruned.
> +  let dateCreated = new Date(2010, 1, 1, 1, 0, 0);
> +  yield writeToArchivedDir("2010-02", dateCreated.getTime() + "." + FAKE_ID1 + "." + FAKE_TYPE + ".json", "{}");
> +  // This one should be kept.

Similar as above.

@@ +140,5 @@
> +  yield writeToArchivedDir("2010-02", dateCreated.getTime() + "." + FAKE_ID1 + "." + FAKE_TYPE + ".json", "{}");
> +  // This one should be kept.
> +  dateCreated = new Date(2010, 2, 1, 1, 0, 0);
> +  yield writeToArchivedDir("2010-03", dateCreated.getTime() + "." + FAKE_ID2 + "." + FAKE_TYPE + ".json", "{}");
> +  // This one should be kept.

Similar as above.
Attachment #8603399 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> @@ +137,5 @@
> > +
> > +  // This one should be pruned.
> > +  let dateCreated = new Date(2010, 1, 1, 1, 0, 0);
> > +  yield writeToArchivedDir("2010-02", dateCreated.getTime() + "." + FAKE_ID1 + "." + FAKE_TYPE + ".json", "{}");
> > +  // This one should be kept.
> 
> Similar as above.

One more thought: why are we writing directly, manually, to the archive dir?
Using fakeNow() and the archiving API we should be able to avoid that (and have less test code that depends on implementation details).
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Comment on attachment 8603398 [details] [diff] [review]
> part 1 - Move the archived pings cache to TelemetryStorage and enable pruning
> 
> Review of attachment 8603398 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1000,5 @@
> > +        this._shutdownBarrier.client.addBlocker("Waiting for archive cleanup", cleanupPromise);
> > +
> > +        // When testing wait for the archive cleanup to complete.
> > +        if (this._testMode) {
> > +          yield cleanupPromise;
> 
> Why do we need this?
> If any tests really need to explicitly wait for a running cleanup task to
> finish (why?), they can block on TelemetryStorage.testCleanupTaskPromise or
> something.
> 
> This should not affect anything else, otherwise i think we have a bug or
> should fix the tests.

We just need to wait for the cleanup to complete in test_PingAPI.js, where we introduced |test_archiveCleanup|.
Since the initialization is not waiting for the cleanup to terminate, we need to be sure that task has finished before checking for retained/removed pings.

> @@ +401,5 @@
> > +   *
> > +   * @return {Promise} Resolved when the cleanup task completes.
> > +   */
> > +  cleanPingsArchive: Task.async(function*() {
> > +    this._log.trace("cleanPingsArchive");
> 
> We said in the implementation doc:
> "the [cleanup] task should honor a shutdown flag and quickly bail out if
> that flag is set, to speed up shutdown".
> I don't see this happening here.

Sorry, that patch was updated before the change to the design document and I didn't update it.

> @@ +428,5 @@
> > +      // This archive directory is older than 180 days, remove it.
> > +      yield this._trashDirectory(dir.path);
> > +    }
> > +
> > +    // If the archive directory was already scanned, filter the ping archive cache.
> 
> That is not in the implementation doc.
> Vladan wanted to avoid the cost of removing individual pings by just
> throwing out monthly dirs unless we hit the storage quota.

We're not performing I/O and hitting the disk there. We're just making sure that the archive cache is coherent: if the cleanup is run after the archive is scanned, we would remove the ping files and still reference them in the cache. I'm happy to drop or discuss this further with you and Vladan.
I've added the comments and used fakeNow+promiseArchivePing to generate the pings for the tests.
Attachment #8603399 - Attachment is obsolete: true
Attachment #8606922 - Flags: review?(gfritzsche)
(In reply to Alessio Placitelli [:Dexter] from comment #14)
> > @@ +428,5 @@
> > > +      // This archive directory is older than 180 days, remove it.
> > > +      yield this._trashDirectory(dir.path);
> > > +    }
> > > +
> > > +    // If the archive directory was already scanned, filter the ping archive cache.
> > 
> > That is not in the implementation doc.
> > Vladan wanted to avoid the cost of removing individual pings by just
> > throwing out monthly dirs unless we hit the storage quota.
> 
> We're not performing I/O and hitting the disk there. We're just making sure
> that the archive cache is coherent: if the cleanup is run after the archive
> is scanned, we would remove the ping files and still reference them in the
> cache. I'm happy to drop or discuss this further with you and Vladan.

Ok. So, as implemented this will remove entries from the cache that are newer than the newest removed monthly dir.
Lets fix that.
Thanks Georg, I've addressed your concerns in this patch.

Please note that since we introduced TelemetryUtils lately, I'm relying on areTimesClose instead of introducing yet another date comparison helper.
Attachment #8603398 - Attachment is obsolete: true
Attachment #8606998 - Flags: review?(gfritzsche)
Comment on attachment 8606998 [details] [diff] [review]
part 1 - Move the archived pings cache to TelemetryStorage and enable pruning - v2

Review of attachment 8606998 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good from the approach now, i'm mostly commenting on some details below.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +77,5 @@
> + * This is a policy object used to override behavior for testing.
> + */
> +let Policy = {
> +  now: () => new Date(),
> +}

Nit: missing semicolon

@@ +137,5 @@
> +  },
> +
> +  /**
> +   * Invalidate the archive cache, forcing the rescanning of the pings archive once
> +   * |loadArchivedPingList| is called again. This is used for testing by TelemetryArchive.

drop "by TelemetryArchive".

@@ +144,5 @@
> +    return TelemetryStorageImpl.testInvalidateArchiveCache();
> +  },
> +
> +  /**
> +   * Test method to get the archive clean task promise.

"that allows waiting on the archive clean task to finish"

@@ +147,5 @@
> +  /**
> +   * Test method to get the archive clean task promise.
> +   */
> +  testCleanupTaskPromise: function() {
> +    return TelemetryStorageImpl._pendingArchiveCleanTask;

This returns null when the cleanup task is not running.
You will get failures when you try to TelemetryStorage.testCleanupTaskPromise().then(...)

@@ +442,5 @@
> +  // Whether we already scanned the archived pings on disk.
> +  _scannedArchiveDirectory: false,
> +
> +  // Track the shutdown process to bail out of the clean up task quickly.
> +  _shutdown: false,

What about resets in tests? I don't see this getting reset, opening us up to race conditions / intermittent failures.
We may want to change testInvalidateArchiveCache() into a _reset() or testReset() for this.

@@ +579,5 @@
> +
> +    // Walk through the monthly subdirs of the form <YYYY-MM>/
> +    for (let dir of subdirs) {
> +      if (this._shutdown) {
> +        this._log.warn("cleanArchiveTask - Terminating the clean up task due to shutdown");

It's expected behavior, so that doesn't need to be a warning - info or trace is enough.

@@ +601,5 @@
> +          if (archiveDate > newestRemovedMonth) {
> +            newestRemovedMonth = archiveDate;
> +          }
> +        } catch (ex) {
> +          this._log.error("cleanArchiveTask - Unable to remove " + aPath, ex);

aPath?

@@ +608,5 @@
> +    }
> +
> +    // If the archive directory was already scanned, filter the ping archive cache.
> +    if (this._scannedArchiveDirectory && newestRemovedMonth) {
> +      let toRemove = [];

Why are we using this instead of directly deleting the entries?
From a casual reading here:
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%mapiteratorprototype%.next
... i don't see any issues with that.

@@ +626,5 @@
> +  }),
> +
> +  /**
> +   * Invalidate the archive cache, forcing the rescanning of the pings archive once
> +   * |loadArchivedPingList| is called again. This is used for testing by TelemetryArchive.

Not "by TelemetryArchive"

@@ +659,5 @@
> +    this._log.trace("_scanArchive");
> +
> +    if (this._scannedArchiveDirectory) {
> +      this._log.trace("_scanArchive - Archive already scanned, hitting cache.");
> +      return this._archivedPings;

Why not do that check in loadArchivedPingList() before spawning any task?

@@ -485,5 @@
>      if (!(yield OS.File.exists(gPingsArchivePath))) {
>        return new Map();
>      }
>  
> -    let archivedPings = new Map();

I think we should still accumulate the scanned data in a local variable and assign that to this._archivedPings at the end.
Otherwise that member behaves unpredictably.

@@ +1114,5 @@
> + * @return {Object} A Date object.
> + */
> +function getDateFromArchiveDir(aDirName) {
> +  let [year, month] = aDirName.split("-");
> +  return new Date(parseInt(year), parseInt(month) - 1, 1, 0, 0, 0);

What about say "2000-50"?
Attachment #8606998 - Flags: review?(gfritzsche) → feedback+
Thanks for your time, Georg. This patch addresses your comments.
Attachment #8606998 - Attachment is obsolete: true
Attachment #8607380 - Flags: review?(gfritzsche)
Comment on attachment 8607380 [details] [diff] [review]
part 1 - Move the archived pings cache to TelemetryStorage and enable pruning - v3

Review of attachment 8607380 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below fixed.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +173,5 @@
>     * Used only for testing purposes.
>     */
>    reset: function() {
>      Impl._clientID = null;
> +    TelemetryStorage.reset();

Hm, i don't like how we don't wait active tasks, but i guess that's not a new issue with our reset() usage in tests.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +127,5 @@
> +  },
> +
> +  /**
> +   * Clean the pings archive by removing old pings.
> +   * This will scan the archive directory.

You mean that this will trigger a scan too?
I think that's incidental, no need to document this here.

@@ +436,5 @@
> +  _archivedPings: new Map(),
> +  // Track the archive loading task to prevent multiple tasks from being executed.
> +  _pendingArchiveLoadingTask: null,
> +  // Track the archive cleanup task.
> +  _pendingArchiveCleanTask: null,

Let's keep the naming consistent with the task methods here, i.e. cleanArchiveTask vs. archiveCleanTask & scanArchiveTask vs. archiveLoadingTask.

Nit: I think the "pending" prefix is confusing and not correct here (that is a pending or *active* task). We could just drop the prefix.

@@ +557,5 @@
> +    // Since there's no archive cleaning task running, start it.
> +    this._pendingArchiveCleanTask = this.cleanArchiveTask();
> +    // Make sure to clear |_pendingArchiveCleanTask| once done.
> +    let clear = () => this._pendingArchiveCleanTask = null;
> +    this._pendingArchiveCleanTask.then(clear, clear);

You want:
this._pendingArchiveCleanTask = this.cleanArchiveTask().then(clear, clear)

@@ +651,5 @@
> +      return Promise.resolve(this._archivedPings);
> +    }
> +
> +    // Since there's no archive loading task running, start it.
> +    this._pendingArchiveLoadingTask = this._scanArchive();

... = this._scanArchive().then(clear, clear)

@@ +1115,5 @@
> + * @return {Object} A Date object or null if the dir name is not valid.
> + */
> +function getDateFromArchiveDir(aDirName) {
> +  let [year, month] = aDirName.split("-");
> +  if (month < 1 || month > 12) {

You need to parseInt() first and should add a NaN check for both year and month.
Attachment #8607380 - Flags: review?(gfritzsche) → review+
Comment on attachment 8606922 [details] [diff] [review]
part 2 - Add test coverage for the archive cleanup - v2

Review of attachment 8606922 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +152,5 @@
> +  const FAKE_TYPE = "foo";
> +
> +  // Create a ping which should be pruned because it is past the retention period.
> +  let dateCreated = new Date(2010, 1, 1, 1, 0, 0);
> +  yield TelemetryArchive.promiseArchivePing({ id: FAKE_ID1, creationDate: dateCreated.getTime(), type: FAKE_TYPE });

Why doesn't this use TelemetryController.submitExternalPing() + fakeNow()?
This uses implementation details again (top-level ping format details) that it doesn't have to.
Attachment #8606922 - Flags: review?(gfritzsche) → feedback+
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> Bug 1150134 already adds scanning the archived pings, we should base this
> bug on it.
> 
> Here we have to:
> * make sure to remove pings after the retention period
> * update the TelemetryPing interfaces (they talk of 14 days and mention
> retention period options)

I'm missing the second part here. Want to address that in a separate patch here or a follow-up bug?
(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> > Bug 1150134 already adds scanning the archived pings, we should base this
> > bug on it.
> > 
> > Here we have to:
> > * make sure to remove pings after the retention period
> > * update the TelemetryPing interfaces (they talk of 14 days and mention
> > retention period options)
> 
> I'm missing the second part here. Want to address that in a separate patch
> here or a follow-up bug?

This will be part of a separate patch on this bug.
As discussed over IRC, |_scanArchive| was changed back to work directly on the archived pings map.
Attachment #8607380 - Attachment is obsolete: true
Attachment #8607496 - Flags: review+
This patch changes the test to use submitExternalPing and fakeNow.
Attachment #8606922 - Attachment is obsolete: true
Attachment #8607497 - Flags: review?(gfritzsche)
Comment on attachment 8607497 [details] [diff] [review]
part 2 - Add test coverage for the archive cleanup - v3

Review of attachment 8607497 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_PingAPI.js
@@ +144,5 @@
>              "Should have rejected invalid ping");
>  });
>  
> +add_task(function* test_archiveCleanup() {
> +  const FAKE_TYPE = "foo";

Nit: PING_TYPE and PING_ID1 etc. - they are not really fake.
Attachment #8607497 - Flags: review?(gfritzsche) → review+
Attachment #8607497 - Attachment is obsolete: true
Attachment #8607525 - Flags: review+
Whops, forgot to update the comments.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fd96b3084ee
Attachment #8607525 - Attachment is obsolete: true
Attachment #8607526 - Flags: review+
Comment on attachment 8607542 [details] [diff] [review]
part 3 - Update TelemetryController with the correct retention days default

Review of attachment 8607542 [details] [diff] [review]:
-----------------------------------------------------------------

We don't honor this at all now, it would seem easier/clearer to just remove the retention options.
Same for the callers, they don't need to pass anything now.
Attachment #8607542 - Flags: review?(gfritzsche)
Attachment #8607598 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
Comment on attachment 8607496 [details] [diff] [review]
part 1 - Move the archived pings cache to TelemetryStorage and enable pruning - v4

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8607496 - Flags: approval-mozilla-aurora?
Comment on attachment 8607526 [details] [diff] [review]
part 2 - Add test coverage for the archive cleanup - v4

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8607526 - Flags: approval-mozilla-aurora?
Comment on attachment 8607598 [details] [diff] [review]
part 3 - Update TelemetryController with the correct retention days default - v2

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8607598 - Flags: approval-mozilla-aurora?
Comment on attachment 8607496 [details] [diff] [review]
part 1 - Move the archived pings cache to TelemetryStorage and enable pruning - v4

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.
Attachment #8607496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8607526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8607598 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.