1137447 - New app update telemetry for patch type (complete or partial), extended error codes, etc.

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

We have most of what bug 1121018 removes. I believe all that is needed is patch type (complete or partial) and at the same time I want to add more telemetry for the ui.

Comment 1

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

Attachment: 1. patch - check telemetry ping

The update check ping will also be sent by UI's and I'll add that later. To keep history from older clients I have changed the code for results that have been broken out into more specific codes.

I'll submit a download result code ping next.

Comment 2

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

Comment on attachment 8570331 [details] [diff] [review]
1. patch - check telemetry ping

typo :(

Comment 3

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

Attachment: 1. patch - check telemetry ping

I combined the patch to remove the no update found from the check ping and instead send that as a boolean for no update found vs. update found.

Comment 4

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

Attachment: 1. patch - check telemetry ping

Fixed some naming

Comment 5

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

Comment on attachment 8570354 [details] [diff] [review]
1. patch - check telemetry ping

Going to hold off on requesting review until after bug 1136358 finishes review

Comment 6

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

Attachment: patch rev1

In errors.h there have been no reports of the error codes in telemetry that I reused.

I went with new telemetry histogram id's since most of the existing histograms changed and this will make it separate the old from the new.

Pushed to try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dbefa5cdcd4

Comment 7

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

Note: the patch requires the patch in bug 1121018.

I don't know at this time if having check codes for the ui will be valuable and if it is aded I'll do it in another bug.

Comment 8

[Brian R. Bondy [:bbondy]]

Comment on attachment 8584439 [details] [diff] [review]
patch rev1

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

::: toolkit/components/telemetry/Histograms.json
@@ -3467,1 @@
>      "expires_in_version": "never",

Is just removing telemetry constants the right way to obsolete them? I thought it was to set expires_in_version?  Will just removing them make them disappear from the dashboard?

::: toolkit/mozapps/update/UpdateTelemetry.jsm
@@ +16,5 @@
> +  // The update check was performed by the call to notify in nsUpdateService.js.
> +  NOTIFY: "NOTIFY",
> +  // The update check was performed by the call to checkForBackgroundUpdates in
> +  // nsUpdateService.js.
> +  EXTERNAL: "EXTERNAL",

Please add some justification on why these things are tracked differently. Do you expect they will differ? Why is it useful to track them differently?

@@ +123,5 @@
> +      if (aCode != this.CHK_NO_UPDATE_FOUND) {
> +        Services.telemetry.getHistogramById(id).add(aCode);
> +        noUpdateFound = false;
> +      }
> +      id = "UPDATE_CHECK_NO_UPDATE_" + aSuffix;

Note that because these strings are computed dynamically it makes the grep test fail. Meaning for someone reading the code wanting to find references to UPDATE_CHECK_NO_UPDATE_EXTERNAL it makes it harder for them.  Would be nice to have the strings explicitly.  Could pass in for example isExternal and then id = isExternal ? "UPDATE_CHECK_NO_UPDATE_EXTERNAL" : "UPDATE_CHECK_NO_UPDATE_NOTIFY". It's not worth changing at this point, but just a thought.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2166,5 @@
>        LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
>            "state");
>        if (update && update.state != STATE_SUCCEEDED) {
>          // Resume download
> +// Possible place where download is stuck?

nit: Indentation, maybe expand on what this comment means more.

@@ +2640,5 @@
>            "update is disabled");
>        return;
>      }
>  
> +#ifdef MOZ_METRO

I'd just remove this, Windows 10 is removing support for new style enabled desktop browsers.  Making any chance of a revival 0.

@@ +4637,2 @@
>      if (update.state == STATE_FAILED &&
> +        (WRITE_ERRORS.indexOf(update.errorCode) != -1 ||

I think the better ES6 way to do this is:

(WRITE_ERRORS.includes(update.errorCode) ||

::: toolkit/mozapps/update/tests/chrome/test_0105_background_restartNotification_VersionCompatAddons.xul
@@ +31,5 @@
> +  debugDump("entering");
> +
> +  Services.prefs.setIntPref(PREF_APP_UPDATE_PROMPTWAITTIME, 1);
> +
> +  let url = URL_HTTP_UPDATE_XML + "?showDetails=1" +

Would be helpful but not required to explicitly add a comment in this file here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/chrome/utils.js#154
...that a URL should not be specified with URL parameters.

::: toolkit/mozapps/update/tests/chrome/utils.js
@@ +1144,5 @@
> +            aAddon.userDisabled = gDisableNoUpdateAddon;
> +          }
> +        }
> +
> +        if (aAddon.name.indexOf("updatecompatibility") == 0) {

Unless we're explicitly trying not to use ES6, please change here and elsewhere:
if (aAddon.name.startsWith("updatecompatibility") {

::: toolkit/mozapps/update/updater/updater.cpp
@@ +487,5 @@
>    NS_tdirent *entry;
>  
>    dir = NS_topendir(path);
>    if (!dir) {
> +    LOG(("ensure_remove_recursive: path is not a directory: " LOG_S \

nit: Here and elsewhere, you don't need the trailing backslash here because of auto string literal concat and whitespace ignored.  Only need it for preprocessor definitions of macros.

Comment 9

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

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Comment on attachment 8584439 [details] [diff] [review]
> patch rev1
> 
> Review of attachment 8584439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ -3467,1 @@
> >      "expires_in_version": "never",
> 
> Is just removing telemetry constants the right way to obsolete them? I
> thought it was to set expires_in_version?  Will just removing them make them
> disappear from the dashboard?
Yes though getting them removed from the data is likely another story. From the "Adding a new Telemetry probe" page
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe

expires_in_version: Required. The version number in which the histogram expires, e.g. "30"; a version number of type "N" and "N.0" is automatically converted to "N.0a1" in order to expire the histogram also in the development channels. A telemetry probe acting on an expired histogram will be considered a non-op. For histograms that never expire the value "never" can be used as in the example above.

> ::: toolkit/mozapps/update/UpdateTelemetry.jsm
> @@ +16,5 @@
> > +  // The update check was performed by the call to notify in nsUpdateService.js.
> > +  NOTIFY: "NOTIFY",
> > +  // The update check was performed by the call to checkForBackgroundUpdates in
> > +  // nsUpdateService.js.
> > +  EXTERNAL: "EXTERNAL",
> 
> Please add some justification on why these things are tracked differently.
> Do you expect they will differ? Why is it useful to track them differently?
Will do

> @@ +123,5 @@
> > +      if (aCode != this.CHK_NO_UPDATE_FOUND) {
> > +        Services.telemetry.getHistogramById(id).add(aCode);
> > +        noUpdateFound = false;
> > +      }
> > +      id = "UPDATE_CHECK_NO_UPDATE_" + aSuffix;
> 
> Note that because these strings are computed dynamically it makes the grep
> test fail. Meaning for someone reading the code wanting to find references
> to UPDATE_CHECK_NO_UPDATE_EXTERNAL it makes it harder for them.  Would be
> nice to have the strings explicitly.  Could pass in for example isExternal
> and then id = isExternal ? "UPDATE_CHECK_NO_UPDATE_EXTERNAL" :
> "UPDATE_CHECK_NO_UPDATE_NOTIFY". It's not worth changing at this point, but
> just a thought.
I hadn't thought of that and it makes sense

> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +2166,5 @@
> >        LOG("UpdateService:_postUpdateProcessing - patch found in downloading " +
> >            "state");
> >        if (update && update.state != STATE_SUCCEEDED) {
> >          // Resume download
> > +// Possible place where download is stuck?
> 
> nit: Indentation, maybe expand on what this comment means more.
Meant to remove that and look into it later.

> @@ +2640,5 @@
> >            "update is disabled");
> >        return;
> >      }
> >  
> > +#ifdef MOZ_METRO
> 
> I'd just remove this, Windows 10 is removing support for new style enabled
> desktop browsers.  Making any chance of a revival 0.
Thanks!

> @@ +4637,2 @@
> >      if (update.state == STATE_FAILED &&
> > +        (WRITE_ERRORS.indexOf(update.errorCode) != -1 ||
> 
> I think the better ES6 way to do this is:
> 
> (WRITE_ERRORS.includes(update.errorCode) ||
It is ES7 ( https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes ) and there is already a comment in the patch of "// Replace with Array.prototype.includes when it has stabilized."

Also, this came up in irc today:
[10:37]	<Mossop>	mconley: JS team are the arbiters of truth
[10:37]	<mconley>	ok, cool
[10:38]	<mconley>	yeah, I was just talking to jorendorff
[10:38]	<mconley>	and he's going to work on a wiki page to lay out what's good to use, and what is not
[10:38]	<mconley>	apparently, all implemented ES6 stuff is OK to use, with the following exceptions:
[10:38]	<mconley>	Classes, Array.prototype.includes, ArrayBuffer.transfer
[10:39]	<mconley>	those are all Nightly-only, and will break on Aurora uplift.


> :::
> toolkit/mozapps/update/tests/chrome/
> test_0105_background_restartNotification_VersionCompatAddons.xul
> @@ +31,5 @@
> > +  debugDump("entering");
> > +
> > +  Services.prefs.setIntPref(PREF_APP_UPDATE_PROMPTWAITTIME, 1);
> > +
> > +  let url = URL_HTTP_UPDATE_XML + "?showDetails=1" +
> 
> Would be helpful but not required to explicitly add a comment in this file
> here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/
> chrome/utils.js#154
> ...that a URL should not be specified with URL parameters.
Not sure what you mean?

> ::: toolkit/mozapps/update/tests/chrome/utils.js
> @@ +1144,5 @@
> > +            aAddon.userDisabled = gDisableNoUpdateAddon;
> > +          }
> > +        }
> > +
> > +        if (aAddon.name.indexOf("updatecompatibility") == 0) {
> 
> Unless we're explicitly trying not to use ES6, please change here and
> elsewhere:
> if (aAddon.name.startsWith("updatecompatibility") {
That can be changed now that it has stabilized. Thanks!

> ::: toolkit/mozapps/update/updater/updater.cpp
> @@ +487,5 @@
> >    NS_tdirent *entry;
> >  
> >    dir = NS_topendir(path);
> >    if (!dir) {
> > +    LOG(("ensure_remove_recursive: path is not a directory: " LOG_S \
> 
> nit: Here and elsewhere, you don't need the trailing backslash here because
> of auto string literal concat and whitespace ignored.  Only need it for
> preprocessor definitions of macros.
Will do

Comment 10

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

Attachment: patch - updated to comments

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7b6c34ec5c

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c7b6c34ec5c