763022 - Report telemetry data for the number of service related errors people encountered

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

We need to create a telemetry histogram for UPDATE_SERVICE_ERRORS.  It should be a linear histogram with values 0, 1, ... 10 which correspond with the number of errors that install had happen to them.  

The pref that corresponds with this is app.update.service.errors.

We should make this more than 10 buckets though because the max errors can be increased, and we may want to increase it in the future.  (min,max,count) = (1, 30, 31) sounds like fair numbers.

Comment 1

[Brian R. Bondy [:bbondy]]

This telemetry ping should only be sent from Windows machines.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Not yet tested

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 -Tested

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 631707 [details] [diff] [review]
Patch v2 -Tested

># HG changeset patch
># Parent bd34592a580a65cad6be796c3f701af7e4a17b34
># User Brian R. Bondy <[e-mail]>
>Bug 763022 - Report telemetry data for the number of service related errors people encountered
>
>diff --git a/toolkit/components/telemetry/TelemetryHistograms.h b/toolkit/components/telemetry/TelemetryHistograms.h
>--- a/toolkit/components/telemetry/TelemetryHistograms.h
>+++ b/toolkit/components/telemetry/TelemetryHistograms.h
>@@ -312,16 +312,17 @@ HISTOGRAM(PLACES_FRECENCY_CALC_TIME_MS, 
> 
> /**
>  * Updater telemetry.
>  */
> HISTOGRAM(UPDATER_STATUS_CODES, 1, 50, 51, LINEAR, "Updater: the status of the latest update performed")
> HISTOGRAM_BOOLEAN(UPDATER_UPDATES_ENABLED, "Updater: Whether or not updates are enabled")
> HISTOGRAM_BOOLEAN(UPDATER_UPDATES_AUTOMATIC, "Updater: Whether or not updates are automatic")
> HISTOGRAM_BOOLEAN(UPDATER_SERVICE_ENABLED, "Updater: Whether or not the MozillaMaintenance service is enabled")
>+HISTOGRAM(UPDATER_SERVICE_ERRORS, 1, 30, 31, LINEAR, "Updater: The number of service errors that have occurred")
So, what about if / when the bypass UAC vs. staging errors are split separated? Just cross that bridge if / when it happens?

>diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js
>--- a/toolkit/mozapps/update/nsUpdateService.js
>+++ b/toolkit/mozapps/update/nsUpdateService.js
>@@ -1593,16 +1593,18 @@ UpdateService.prototype = {
>       // Report telemetry that we want after each successful update
>       this._sendBoolPrefTelemetryPing(PREF_APP_UPDATE_ENABLED,
>                                       "UPDATER_UPDATES_ENABLED");
>       this._sendBoolPrefTelemetryPing(PREF_APP_UPDATE_AUTO,
>                                       "UPDATER_UPDATES_AUTOMATIC");
> #ifdef XP_WIN
>       this._sendBoolPrefTelemetryPing(PREF_APP_UPDATE_SERVICE_ENABLED,
>                                       "UPDATER_SERVICE_ENABLED");
>+      this._sendIntPrefTelemetryPing(PREF_APP_UPDATE_SERVICE_ERRORS,
>+                                     "UPDATER_SERVICE_ERRORS");
> #endif
> 
>       update.statusText = gUpdateBundle.GetStringFromName("installSuccess");
> 
>       // Update the patch's metadata.
>       um.activeUpdate = update;
>       Services.prefs.setBoolPref(PREF_APP_UPDATE_POSTUPDATE, true);
>       prompter.showUpdateInstalled();
>@@ -1648,16 +1650,35 @@ UpdateService.prototype = {
>       Services.telemetry.getHistogramById(histogram).add(+val);
>     } catch(e) {
>       // Don't allow any exception to be propagated.
>       Components.utils.reportError(e);
>     }
>   },
> 
>   /**
>+   * Submit a telemetry ping with the int value of a pref for a histogram
>+   *
>+   * @param  pref
>+   *         The preference to report
>+   * @param histogram
>+   *         The histogram ID to report to
>+   */
>+  _sendIntPrefTelemetryPing: function AUS__boolTelemetryPing(pref, histogram) {
s/AUS__boolTelemetryPing/AUS__intTelemetryPing/

>+    try {
>+      let val = getPref("getIntPref", pref, 0);
>+      Services.telemetry.getHistogramById(histogram).add(val);
>+    } catch(e) {
>+      // Don't allow any exception to be propagated.
>+      Components.utils.reportError(e);
>+    }
The getPref call shouldn't fail since it is already wrapped in a try catch though I am fine with this as is since we don't want telemetry pings ever breaking app update but please add a comment to this affect so other people don't spend time thinking about why it is in the try catch.

Comment 5

[Brian R. Bondy [:bbondy]]

> So, what about if / when the bypass UAC vs. staging errors are split separated? 
> Just cross that bridge if / when it happens?

In that case I think we'd have an independent error count so we'd have a different histogram for that specifically.  I'll post for it though and link it to the bgupdate max error count bug though.

Comment 6

[Robert Strong (they/them - no direct email)]

When / if it is added the "Updater: The number of service errors that have occurred" text should be clarified.

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3.

Implemented review comments.
Updated message now instead of having to do it later to be more specific:
"Updater: The number of MozillaMaintenance service errors that have occurred"

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4 - Wrong bug patch

The getPref call had a default value of 0 for a boolean pref.  It was fine but replaced it with false instead.

Comment 9

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/187f7d640d5e