Closed Bug 1168835 Opened 5 years ago Closed 5 years ago

Implement pending-pings cleanup and quota

Categories

(Toolkit :: Telemetry, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla42
Iteration:
42.2 - Jul 27
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [unifiedTelemetry] [uplift4])

Attachments

(3 files, 9 obsolete files)

10.32 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
25.31 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
13.74 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
This comes from the new ping expiry rules here:
https://docs.google.com/document/d/1K_N3Vkxdm-Ham_GhuXhMsC7-qz2ksDKzAtz5J0gBnag/edit

We need to extend the Telemetry cleanup task to implement a storage quota for pending pings.
Blocks: 1156353
Depends on: 1162538
Whiteboard: [b5] [unifiedTelemetry]
Whiteboard: [b5] [unifiedTelemetry] → [b5] [unifiedTelemetry] [uplift2]
Assignee: nobody → gfritzsche
Priority: -- → P1
Assignee: gfritzsche → alessio.placitelli
This patch:

- adds the quota limit for pending pings, removing oldest first
- removes the old pruning logic (LRU, expired pings)
Attachment #8634276 - Flags: feedback?(gfritzsche)
Attached patch part 2 - Add test coverage (obsolete) — Splinter Review
This patch adds test coverage for pending pings quota enforcement.
Attachment #8634278 - Flags: feedback?(gfritzsche)
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [40b7] [unifiedTelemetry] [uplift2]
Attachment #8634278 - Flags: feedback?(gfritzsche)
Comment on attachment 8634276 [details] [diff] [review]
part 1 - Enforce the quota limit for pending pings.

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +84,1 @@
>  const OVERDUE_PING_FILE_AGE = 7 * 24 * 60 * MS_IN_A_MINUTE; // 1 week

We only use this inside _checkPendingPings() now to keep the probes working as before, so lets move it in there to avoid confusion.

@@ +84,4 @@
>  const OVERDUE_PING_FILE_AGE = 7 * 24 * 60 * MS_IN_A_MINUTE; // 1 week
>  
>  // Maximum number of pings to save.
>  const MAX_LRU_PINGS = 50;

Unused?

@@ -549,5 @@
>  
>      return this._logger;
>    },
>  
> -  get discardedPingsCount() {

You need to remove the public getter above too.
TelemetrySession uses this, you need to update it too.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +49,5 @@
>  
>  // Maximum space the archive can take on disk (in Bytes).
>  const ARCHIVE_QUOTA_BYTES = 120 * 1024 * 1024; // 120 MB
> +// Maximum space the outgoing pings can take on disk (in Bytes).
> +const PENDING_PINGS_QUOTA_BYTES = 15 * 1024 * 1024; // 15 MB

This will affect Android, we need to implement different quotas on Android and Desktop AFAIR.
Archiving was different as we only turned this on for Fx Desktop.

@@ +482,5 @@
>    // Tracks the pending pings in a Map of (id -> {timestampCreated, type}).
>    // We use this to cache info on pending pings to avoid scanning the disk more than once.
>    _pendingPings: new Map(),
> +  // Track the pending pings enforce quota task.
> +  _enforcePendingPingsQuotaTask: null,

I don't see you waiting for it on shutdown.
Attachment #8634276 - Flags: feedback?(gfritzsche)
Comment on attachment 8634276 [details] [diff] [review]
part 1 - Enforce the quota limit for pending pings.

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

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +830,5 @@
> +  /**
> +   * Enforce a disk quota for the pending pings.
> +   * @return {Promise} Resolved when the quota check is complete.
> +   */
> +  _enforcePendingPingsQuota: Task.async(function*() {

What about tracking measures in histograms here as we do for the archive quota enforcement?
(In reply to Georg Fritzsche [:gfritzsche] [away jul 13-17] from comment #4)
> Comment on attachment 8634276 [details] [diff] [review]
> part 1 - Enforce the quota limit for pending pings.
> 
> Review of attachment 8634276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryStorage.jsm
> @@ +830,5 @@
> > +  /**
> > +   * Enforce a disk quota for the pending pings.
> > +   * @return {Promise} Resolved when the quota check is complete.
> > +   */
> > +  _enforcePendingPingsQuota: Task.async(function*() {
> 
> What about tracking measures in histograms here as we do for the archive
> quota enforcement?

Suggesting the TELEMETRY_PENDING_* here.
Thanks Georg for the feedback. Yoric, do you think you may have some time to review this?
Attachment #8634276 - Attachment is obsolete: true
Attachment #8634741 - Flags: review?(dteller)
Attached patch part 2 - Add test coverage - v2 (obsolete) — Splinter Review
Attachment #8634278 - Attachment is obsolete: true
Attachment #8634743 - Flags: review?(dteller)
Attached patch Part 3 - Add telemetry probes (obsolete) — Splinter Review
This patch adds the following probes (plus test coverage):

===> TELEMETRY_PENDING_PINGS_SIZE_MB
===> TELEMETRY_PENDING_PINGS_EVICTED_OVER_QUOTA
===> TELEMETRY_PENDING_EVICTING_OVER_QUOTA_MS
===> TELEMETRY_PENDING_CHECKING_OVER_QUOTA_MS
Attachment #8634745 - Flags: review?(dteller)
(In reply to Georg Fritzsche [:gfritzsche] [away jul 13-17] from comment #3)
> Comment on attachment 8634276 [details] [diff] [review]
> part 1 - Enforce the quota limit for pending pings.
> 
> Review of attachment 8634276 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: toolkit/components/telemetry/TelemetryStorage.jsm
> @@ +49,5 @@
> >  
> >  // Maximum space the archive can take on disk (in Bytes).
> >  const ARCHIVE_QUOTA_BYTES = 120 * 1024 * 1024; // 120 MB
> > +// Maximum space the outgoing pings can take on disk (in Bytes).
> > +const PENDING_PINGS_QUOTA_BYTES = 15 * 1024 * 1024; // 15 MB
> 
> This will affect Android, we need to implement different quotas on Android
> and Desktop AFAIR.
> Archiving was different as we only turned this on for Fx Desktop.

The document in the first comment reports: "Limit outgoing ping storage to 15MB? (around 1000 release pings?)" and "15MB for persisted pings on all platforms", so I assumed this was the way to go.

:rnewman, do you think it's reasonable to have a 15MB quota on Android as well for pending/outgoing pings or we should define a lower quota for mobile?
Flags: needinfo?(rnewman)
Comment on attachment 8634741 [details] [diff] [review]
part 1 - Enforce the quota limit for pending pings. - v2

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

Just a quick look here.

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +84,1 @@
>  const OVERDUE_PING_FILE_AGE = 7 * 24 * 60 * MS_IN_A_MINUTE; // 1 week

Is this still required externally? If not lets move it into _checkPendingPings too.
Also, the comment is pretty outdated.

@@ +150,5 @@
>  
>    /**
>     * Age in ms of a pending ping to be considered overdue.
>     */
>    get OVERDUE_PING_FILE_AGE() {

Is this still used externally?

@@ +572,5 @@
>      this._log.info("_checkPendingPings - pending ping count: " + infos.length);
>      if (infos.length == 0) {
>        this._log.trace("_checkPendingPings - no pending pings");
>        return;
>      }

While we are doing changes here, we should also submit the ages of the persisted pending pings at startup.
This can be a separate patch or a follow-up bug though.

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +507,5 @@
>      // If the archive cleaning task is running, block on it. It should bail out as soon
>      // as possible.
>      yield this._cleanArchiveTask;
> +    // If the pending pings quota task is running, block on it. It should bail out as soon
> +    // as possible.

This is nearly verbatim the same comment as above.
Just change the above one to "If the tasks for archive cleaning or pending ping quota are still running, ..."?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +3,5 @@
>  
>  /**
>   * This test case populates the profile with some fake stored
> + * pings, and checks that pings that are considered "overdue"
> + * trigger a send of all overdue and recent pings.

We don't have any "overdue" trigger anymore.
We now always start sending "immediately after delayed init", subject to a 10-pings-per-min limit.

@@ +272,5 @@
>  });
>  
>  /**
> + * Create some recent and overdue pings. The overdue pings should
> + * trigger a send of all recent and overdue pings.

Minus the triggering.
Comment on attachment 8634743 [details] [diff] [review]
part 2 - Add test coverage - v2

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

::: toolkit/components/telemetry/tests/unit/head.js
@@ +285,5 @@
>    Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
>    // Telemetry archiving should be on.
>    Services.prefs.setBoolPref("toolkit.telemetry.archive.enabled", true);
> +  // Disable geoip lookup, so we don't get unwanted network connections.
> +  Services.prefs.setCharPref("browser.search.geoip.url", "");

If this is still happening, this should get an xpcshell-wide fix?
Comment on attachment 8634745 [details] [diff] [review]
Part 3 - Add telemetry probes

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

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +893,5 @@
> +    // Save the time it takes to check if the pending pings are over-quota.
> +    Telemetry.getHistogramById("TELEMETRY_PENDING_CHECKING_OVER_QUOTA_MS")
> +             .add(Math.round(Policy.now().getTime() - startTimeStamp));
> +
> +    let submitProbes = (sizeInMB, evictedPings, elapsedMs) => {

recordHistograms?
Comment on attachment 8634741 [details] [diff] [review]
part 1 - Enforce the quota limit for pending pings. - v2

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

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +79,5 @@
>  // This exponential backoff will be reset by external ping submissions & idle-daily.
>  const SEND_MAXIMUM_BACKOFF_DELAY_MS = 120 * MS_IN_A_MINUTE;
>  
> +// Files that are older than OVERDUE_PING_FILE_AGE, indicate that we need to send all
> +// of our pings ASAP.

Nit: The sentence is slightly awkward.
Maybe "If we have files older than ..., we need to send ..."

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +481,5 @@
>  
>    // Tracks the pending pings in a Map of (id -> {timestampCreated, type}).
>    // We use this to cache info on pending pings to avoid scanning the disk more than once.
>    _pendingPings: new Map(),
> +  // Track the pending pings enforce quota task.

Nit: newline above the comment, please.

@@ +506,5 @@
>      yield this._abortedSessionSerializer.flushTasks();
>      // If the archive cleaning task is running, block on it. It should bail out as soon
>      // as possible.
>      yield this._cleanArchiveTask;
> +    // If the pending pings quota task is running, block on it. It should bail out as soon

Nit: I'm not sure what "should" means in this context. Is it something that needs to be enforced somewhere?

@@ +863,5 @@
> +
> +      // Get the size for this ping.
> +      const fileSize = yield getPendingPingSize(ping.id);
> +      if (!fileSize) {
> +        this._log.warn("_enforcePendingPingsQuota - Unable to find the size of ping " + ping.id);

Could a file possibly have size 0? What's going to happen in such case?
Attachment #8634741 - Flags: review?(dteller) → review+
Comment on attachment 8634743 [details] [diff] [review]
part 2 - Add test coverage - v2

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

Stealing, r=me with the issues below addressed.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +351,5 @@
> +add_task(function* test_pendingPingsQuota() {
> +  const PING_TYPE = "foo";
> +  const PREF_FHR_UPLOAD = "datareporting.healthreport.uploadEnabled";
> +
> +  // Disable upload so pings don't get sent.

I think your actual goal here is to avoid new pending pings getting saved, so you get a predictable ping count and size, right?
Please change the comment here accordingly.

@@ +404,5 @@
> +    pingsSizeInBytes += pingSize;
> +  }
> +
> +  // Sort the list by last modification date.
> +  pendingPingsInfo.sort((a, b) => b.timestamp - a.timestamp);

Why do you need to sort them, you are pushing them on the array in a well defined order?

@@ +412,5 @@
> +  fakePendingPingsQuota(testQuotaInBytes);
> +
> +  // The storage prunes pending pings until we reach 90% of the requested storage quota.
> +  // Based on that, find how many pings should be kept.
> +  const safeQuotaSize = testQuotaInBytes * 0.9;

Do we run into potential rounding issues here, inviting intermittent failures at the size check below?
Attachment #8634743 - Flags: review?(dteller) → review+
(In reply to Georg Fritzsche [:gfritzsche] [away jul 13-17] from comment #11)
> Comment on attachment 8634743 [details] [diff] [review]
> part 2 - Add test coverage - v2
> 
> Review of attachment 8634743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/tests/unit/head.js
> @@ +285,5 @@
> >    Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
> >    // Telemetry archiving should be on.
> >    Services.prefs.setBoolPref("toolkit.telemetry.archive.enabled", true);
> > +  // Disable geoip lookup, so we don't get unwanted network connections.
> > +  Services.prefs.setCharPref("browser.search.geoip.url", "");
> 
> If this is still happening, this should get an xpcshell-wide fix?

... i.e. move this to testing/xpcshell/head.js
Comment on attachment 8634745 [details] [diff] [review]
Part 3 - Add telemetry probes

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

Stealing.

::: toolkit/components/telemetry/Histograms.json
@@ +4659,5 @@
> +    "alert_emails": ["telemetry-client-dev@mozilla.com"],
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "high": "30",
> +    "n_buckets": 15,

I think we want more detail here than 2MB steps, suggesting 1MB steps here.
The histogram simulator can be useful here btw:
http://development.telemetry-dashboard.divshot.io/histogram-simulator/#low=1&high=30&n_buckets=15&kind=linear&generate=custom

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +55,5 @@
>  // This special value is submitted when the archive is outside of the quota.
>  const ARCHIVE_SIZE_PROBE_SPECIAL_VALUE = 300;
>  
> +// This special value is submitted when the pending pings is outside of the quota.
> +const PENDING_PINGS_SIZE_PROBE_SPECIAL_VALUE = 30;

Why do we need this special value? Everything above the "high" value of a histogram gets submitted into the "overflow bucket" anyway.

@@ +924,5 @@
>      }
> +
> +    const endTimeStamp = Policy.now().getTime();
> +    submitProbes(PENDING_PINGS_SIZE_PROBE_SPECIAL_VALUE, pingsToPurge.length,
> +                 Math.ceil(endTimeStamp - startTimeStamp));

startTimeStamp is not reset here, so this - incorrectly - includes the time required for quota-checking too.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
@@ +458,5 @@
> +  h = Telemetry.getHistogramById("TELEMETRY_PENDING_PINGS_EVICTED_OVER_QUOTA").snapshot();
> +  Assert.equal(h.sum, pingsOutsideQuota.length,
> +               "Telemetry must correctly report the over quota pings evicted from the pending pings directory.");
> +  h = Telemetry.getHistogramById("TELEMETRY_PENDING_PINGS_SIZE_MB").snapshot();
> +  Assert.equal(h.sum, 30, "Pending pings quota was hit, a special size must be reported.");

Can you also test the duration histograms?
Attachment #8634745 - Flags: review?(dteller)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #13)
> @@ +863,5 @@
> > +
> > +      // Get the size for this ping.
> > +      const fileSize = yield getPendingPingSize(ping.id);
> > +      if (!fileSize) {
> > +        this._log.warn("_enforcePendingPingsQuota - Unable to find the size of ping " + ping.id);
> 
> Could a file possibly have size 0? What's going to happen in such case?

Thanks for the review Yoric!

I've filed bug 1185506 to verify how we deal with broken/corrupted pending pings. The worst case is that we remove the corrupted pending pings when we reach the quota, as oldest gets removed first.
Attachment #8634741 - Attachment is obsolete: true
Attachment #8636021 - Flags: review+
Attachment #8634743 - Attachment is obsolete: true
Attachment #8636042 - Flags: review+
This also fixes |TelemetryStorage._enforceArchiveQuota| reporting the incorrect time it takes to evict pings.
Attachment #8634745 - Attachment is obsolete: true
Attachment #8636044 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] [away jul 13-17] from comment #16)
> ::: toolkit/components/telemetry/Histograms.json
> @@ +4659,5 @@
> > +    "alert_emails": ["telemetry-client-dev@mozilla.com"],
> > +    "expires_in_version": "never",
> > +    "kind": "linear",
> > +    "high": "30",
> > +    "n_buckets": 15,
> 
> I think we want more detail here than 2MB steps, suggesting 1MB steps here.
> The histogram simulator can be useful here btw:
> http://development.telemetry-dashboard.divshot.io/histogram-simulator/
> #low=1&high=30&n_buckets=15&kind=linear&generate=custom

Thanks, this is a great tool!

> ::: toolkit/components/telemetry/TelemetryStorage.jsm
> @@ +55,5 @@
> >  // This special value is submitted when the archive is outside of the quota.
> >  const ARCHIVE_SIZE_PROBE_SPECIAL_VALUE = 300;
> >  
> > +// This special value is submitted when the pending pings is outside of the quota.
> > +const PENDING_PINGS_SIZE_PROBE_SPECIAL_VALUE = 30;
> 
> Why do we need this special value? Everything above the "high" value of a
> histogram gets submitted into the "overflow bucket" anyway.

Because we stop scanning the pending pings directory for ping sizes when we reach the quota. I've added a few comments to clarify this.

> @@ +924,5 @@
> >      }
> > +
> > +    const endTimeStamp = Policy.now().getTime();
> > +    submitProbes(PENDING_PINGS_SIZE_PROBE_SPECIAL_VALUE, pingsToPurge.length,
> > +                 Math.ceil(endTimeStamp - startTimeStamp));
> 
> startTimeStamp is not reset here, so this - incorrectly - includes the time
> required for quota-checking too.

Good catch. We had the same issue for the archive, fixed.

> ::: toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js
> @@ +458,5 @@
> > +  h = Telemetry.getHistogramById("TELEMETRY_PENDING_PINGS_EVICTED_OVER_QUOTA").snapshot();
> > +  Assert.equal(h.sum, pingsOutsideQuota.length,
> > +               "Telemetry must correctly report the over quota pings evicted from the pending pings directory.");
> > +  h = Telemetry.getHistogramById("TELEMETRY_PENDING_PINGS_SIZE_MB").snapshot();
> > +  Assert.equal(h.sum, 30, "Pending pings quota was hit, a special size must be reported.");
> 
> Can you also test the duration histograms?

Mh, I'm not quite sure there's an "easy" way to change the |Policy.getTime| while the |_enforceQuota| function is in progress. We tried to do the same thing for the archive quota but it wasn't really worth the effort.
Comment on attachment 8636044 [details] [diff] [review]
Part 3 - Add telemetry probes - v2

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

r=me with the below addressed.

::: toolkit/components/telemetry/TelemetrySend.jsm
@@ +582,5 @@
>      this._overduePingCount = overduePings.length;
> +
> +    // Submit the age of the pending pings.
> +    for (let pingInfo of infos) {
> +      const ageInDays = Utils.millisecondsToDays(now.getTime() - pingInfo.lastModificationDate);

Consider jumping clocks here or other issues.
Do we need to Math.abs() here or does the histogram bucketing take care of that for us?

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +779,5 @@
>  
>      this._log.info("_enforceArchiveQuota - archive size: " + archiveSizeInBytes + "bytes"
>                     + ", safety quota: " + SAFE_QUOTA + "bytes");
>  
> +    startTimeStamp = Policy.now().getTime();

We are breaking the histogram development here, do we track per-histogram erratas somewhere too?
Vladan should know.
Attachment #8636044 - Flags: review?(gfritzsche) → review+
Discussed with Dexter on IRC. 

I can't see a world in which we'd be willing to upload 15MB of data. So let's cap this to 200K or so on mobile, and keep an eye on the meta-telemetry to see how often we hit that cap. We can adjust as needed.
Flags: needinfo?(rnewman)
Updated the quota for Android/Gonk.
Attachment #8636021 - Attachment is obsolete: true
Attachment #8636081 - Flags: review+
Attachment #8636044 - Attachment is obsolete: true
Attachment #8636082 - Flags: review+
As discussed over irc with rnewman, gfritzsche, bumping the quota for mobile to 1mb.
Attachment #8636081 - Attachment is obsolete: true
Attachment #8636097 - Flags: review+
Rebased.
Attachment #8636082 - Attachment is obsolete: true
Attachment #8636098 - Flags: review+
(In reply to Georg Fritzsche [:gfritzsche] [away until july 22] from comment #22)
> Comment on attachment 8636044 [details] [diff] [review]
> Part 3 - Add telemetry probes - v2
> 
> Review of attachment 8636044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the below addressed.
> 
> ::: toolkit/components/telemetry/TelemetrySend.jsm
> @@ +582,5 @@
> >      this._overduePingCount = overduePings.length;
> > +
> > +    // Submit the age of the pending pings.
> > +    for (let pingInfo of infos) {
> > +      const ageInDays = Utils.millisecondsToDays(now.getTime() - pingInfo.lastModificationDate);
> 
> Consider jumping clocks here or other issues.
> Do we need to Math.abs() here or does the histogram bucketing take care of
> that for us?

A negative value would get us into the "underflow" bucket, but I've added the |abs()| to be safe.

> ::: toolkit/components/telemetry/TelemetryStorage.jsm
> @@ +779,5 @@
> >  
> >      this._log.info("_enforceArchiveQuota - archive size: " + archiveSizeInBytes + "bytes"
> >                     + ", safety quota: " + SAFE_QUOTA + "bytes");
> >  
> > +    startTimeStamp = Policy.now().getTime();
> 
> We are breaking the histogram development here, do we track per-histogram
> erratas somewhere too?
> Vladan should know.

I'll update https://wiki.mozilla.org/Telemetry/Errata once my Wiki account is enabled.
Whiteboard: [40b7] [unifiedTelemetry] [uplift2] → [40b7] [unifiedTelemetry] [uplift4]
Iteration: --- → 42.2 - Jul 27
Flags: qe-verify-
Flags: in-testsuite+
Whiteboard: [40b7] [unifiedTelemetry] [uplift4] → [unifiedTelemetry] [uplift4]
Comment on attachment 8636097 [details] [diff] [review]
part 1 - Enforce the quota limit for pending pings. - v4

Approval Request Comment
[Feature/regressing bug #]:
Telemetry Unification
[User impact if declined]:
This is a shipping requirement for Unified Telemetry. It updates the local ping persistence logic to an approach that loses us less pings.
It is part of a mostly fixup & diagnostic uplift batch for 41: http://bit.ly/1LYhA16
[Describe test coverage new/current, TreeHerder]:
We have automated test-coverage, it baked on Nightly, we tracked metrics for this via Telemetry.
[Risks and why]:
Low general risk of hidden issues, but we haven't seen anything crop up on Nightly here.
[String/UUID change made/needed]:
None.
Attachment #8636097 - Flags: approval-mozilla-aurora?
Comment on attachment 8636042 [details] [diff] [review]
part 2 - Add test coverage - v3

Approval Request Comment
[Feature/regressing bug #]:
Telemetry Unification
[User impact if declined]:
This is a shipping requirement for Unified Telemetry. This adds the tests for this change.
It is part of a mostly fixup & diagnostic uplift batch for 41: http://bit.ly/1LYhA16
[Describe test coverage new/current, TreeHerder]:
We have automated test-coverage, it baked on Nightly, we tracked metrics for this via Telemetry.
[Risks and why]:
Low general risk of hidden issues, but we haven't seen anything crop up on Nightly here.
[String/UUID change made/needed]:
None.
Comment on attachment 8636042 [details] [diff] [review]
part 2 - Add test coverage - v3

Approval Request Comment
[Feature/regressing bug #]:
Telemetry Unification
[User impact if declined]:
This is a shipping requirement for Unified Telemetry. It updates the local ping persistence logic to an approach that loses us less pings.
It is part of a mostly fixup & diagnostic uplift batch for 41: http://bit.ly/1LYhA16
[Describe test coverage new/current, TreeHerder]:
We have automated test-coverage, it baked on Nightly, we tracked metrics for this via Telemetry.
[Risks and why]:
Low general risk of hidden issues, but we haven't seen anything crop up on Nightly here.
[String/UUID change made/needed]:
None.
Attachment #8636042 - Flags: approval-mozilla-aurora?
Comment on attachment 8636098 [details] [diff] [review]
Part 3 - Add telemetry probes - v3

Approval Request Comment
[Feature/regressing bug #]:
Telemetry Unification
[User impact if declined]:
This is a shipping requirement for Unified Telemetry. It updates the local ping persistence logic to an approach that loses us less pings.
It is part of a mostly fixup & diagnostic uplift batch for 41: http://bit.ly/1LYhA16
[Describe test coverage new/current, TreeHerder]:
We have automated test-coverage, it baked on Nightly, we tracked metrics for this via Telemetry.
[Risks and why]:
Low general risk of hidden issues, but we haven't seen anything crop up on Nightly here.
[String/UUID change made/needed]:
None.
Attachment #8636098 - Flags: approval-mozilla-aurora?
Comment on attachment 8636042 [details] [diff] [review]
part 2 - Add test coverage - v3

Unified Telemetry is a FF41 feature. It has been in m-c for a few week and includes tests. Let's uplift to Aurora.
Attachment #8636042 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8636097 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8636098 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.