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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
mozilla39
x86_64
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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.
Blocks: 1121018
Status: NEW → ASSIGNED
QA Contact: robert.strong.bugs
Summary: New app update telemetry for patch complete and partial and ui → New app update telemetry for patch type (complete or partial) and ui
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.
Assignee: nobody → robert.strong.bugs
Attachment #8570331 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8570331 [details] [diff] [review]
1. patch - check telemetry ping

typo :(
Attachment #8570331 - Attachment is obsolete: true
Attachment #8570331 - Flags: review?(spohl.mozilla.bugs)
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.
Attachment #8570342 - Flags: review?(spohl.mozilla.bugs)
Fixed some naming
Attachment #8570342 - Attachment is obsolete: true
Attachment #8570342 - Flags: review?(spohl.mozilla.bugs)
Attachment #8570354 - Flags: review?(spohl.mozilla.bugs)
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
Attachment #8570354 - Flags: review?(spohl.mozilla.bugs)
Summary: New app update telemetry for patch type (complete or partial) and ui → New app update telemetry for patch type (complete or partial), ui, extended error codes, etc.
QA Contact: robert.strong.bugs
Posted patch patch rev1 (obsolete) — Splinter Review
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
Attachment #8570354 - Attachment is obsolete: true
Attachment #8584439 - Flags: review?(netzen)
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.
Summary: New app update telemetry for patch type (complete or partial), ui, extended error codes, etc. → New app update telemetry for patch type (complete or partial), extended error codes, etc.
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.
Attachment #8584439 - Flags: review?(netzen) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/5c7b6c34ec5c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Duplicate of this bug: 763246
Duplicate of this bug: 765304
You need to log in before you can comment on or make changes to this bug.