886545 - Provide method to check for updates to support bug 885641

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

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

This will allow the code in bug 885641 to be simplified and better support reporting in telemetry.

Comment 1

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

Attachment: Telemetry probe for notify interval in hours

I think I got this right based on
https://developer.mozilla.org/en-US/docs/Performance/Adding_a_new_Telemetry_probe

Comment 2

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

Comment on attachment 766989 [details] [diff] [review]
Telemetry probe for notify interval in hours

Also asking Taras for a sanity check regarding the values used for telemetry.

Comment 3

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

btw: I suspect days vs. hours would be better and would like your opinion of that as well... I mainly wanted to make sure this is correct.

Comment 4

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

Taras, what would be the best way to report the status code from the xhr to telemetry? Nothing jumped out at me when going over Histograms.json
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3449

Comment 5

[(dormant account)]

Comment on attachment 766989 [details] [diff] [review]
Telemetry probe for notify interval in hours

I would prefer to do currentUpdateTime = Date.now()/1000 and do the rest of the math after the subtraction.

Comment 6

[Brian R. Bondy [:bbondy]]

Comment on attachment 766989 [details] [diff] [review]
Telemetry probe for notify interval in hours

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

::: toolkit/components/telemetry/Histograms.json
@@ +2241,5 @@
>    },
> +  "UPDATER_UPDATES_LAST_NOTIFY_INTERVAL": {
> +    "kind": "exponential",
> +    "n_buckets": 100,
> +    "high": "1440",

Seems like a lot of buckets for such a small high value but I'll leave it to your discretion.

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2362,5 @@
> +    // last background check telemetry
> +    if (prefs.prefHasUserValue(PREF_APP_UPDATE_LASTUPDATETIME)) {
> +      let currentUpdateHours = Math.round(Date.now() / (1000 * 60 * 60));
> +      let previousUpdateHours = getPref("getIntPref",
> +                                        PREF_APP_UPDATE_LASTUPDATETIME, 0);

Please call this previousUpdateSeconds

Comment 7

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

Taras, can I get a response for the question in comment #4?

Comment 8

[(dormant account)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #7)
> Taras, can I get a response for the question in comment #4?

if there is a fixed set of status codes, but you can have multiple per session you should use an enum histogram...eg a 3 value enum can map 0:20x, 1:30x, 2:40x

Comment 9

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

Attachment: telemetry additions patch part 1 rev1

I've been spinning on the best way to report data to telemetry given the lack of being able to easily correlate it with other data. For example, I'd like to know the pageid that was last displayed (18 possible values) and the button pushed to exit (4 possible values). For now I think it is best to go with just the pageid mapped to an integer.

Comment 10

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

Attachment: in progress telemetry additions patch part 2 rev1

Forgot to add this in the previous patch and since I suspect there will be more I'm starting a second patch.

Comment 11

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

Comment on attachment 768790 [details] [diff] [review]
telemetry additions patch part 1 rev1

btw: this patch caused timeouts for mochitest-other
https://tbpl.mozilla.org/?tree=Try&rev=2c5d52b01155

I'll figure out what is going on tomorrow

Comment 12

[Brian R. Bondy [:bbondy]]

Comment on attachment 768790 [details] [diff] [review]
telemetry additions patch part 1 rev1

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

If you'd still like to change the prefix/names of the histograms, it would be best to do it before this lands.  I'd like to have one more pass after the changes in the review comments.

::: toolkit/mozapps/update/content/updates.js
@@ +149,5 @@
> +   * @param   pageID
> +   */
> +  _sendLastPageCodePing: function(pageID) {
> +    var pageCode = 0; // dummy page code
> +    switch (state) {

'state' doesn't exist here, I think you mean pageID.

@@ +202,5 @@
> +      case "installed":
> +        pageCode = 17;
> +        break;
> +    }
> +

nit:
A slightly cleaner way without losing the easy lookup aspect would be:
var pageMap = { val1: 1,
                val2: 2,
                val3: 3,
            ...}
Services.telemetry.getHistogramById("UPDATER_WIZ_LAST_PAGE_CODE").add(pageMap[pageId]);

@@ +204,5 @@
> +        break;
> +    }
> +
> +    try {
> +      Services.telemetry.getHistogramById("UPDATER_WIZ_LAST_PAGE_CODE").add(pageCode);    

nit: trailing whitespace

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2258,5 @@
> +   */
> +  _sendDownloaderPing: function AUS__sendDownloaderPing() {
> +    try {
> +      Services.telemetry.getHistogramById("UPDATER_DOWNLOADER_INSTANTIATED").
> +      add(+this._downloader);

nit: I think this 'add' would read easier if it was indented 2 spaces.

@@ +2262,5 @@
> +      add(+this._downloader);
> +      Services.telemetry.getHistogramById("UPDATER_DOWNLOADER_IS_BUSY").
> +      add(+(this._downloader && this._downloader.isBusy));
> +      Services.telemetry.getHistogramById("UPDATER_DOWNLOADER_IS_PATCH_STAGED").
> +      add(+(this._downloader && this._downloader.isBusy));

I think this should be:
add(+(this._downloader && this._downloader.patchIsStaged));

@@ +2263,5 @@
> +      Services.telemetry.getHistogramById("UPDATER_DOWNLOADER_IS_BUSY").
> +      add(+(this._downloader && this._downloader.isBusy));
> +      Services.telemetry.getHistogramById("UPDATER_DOWNLOADER_IS_PATCH_STAGED").
> +      add(+(this._downloader && this._downloader.isBusy));
> +      patchIsStaged

Remove this line.

@@ +2604,5 @@
>            "just download the update");
>        var status = this.downloadUpdate(update, true);
>        if (status == STATE_NONE)
>          cleanupActiveUpdate();
> +      this._sendAddonCheckCodePing(0);

It would be good to have these values defined as const at the top of the file instead of using magic numbers

Comment 13

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

Attachment: telemetry additions part1 rev2

Brian, I think I should protect against negative numbers for the interval... what do you think?

Comment 14

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

Sent to try
https://tbpl.mozilla.org/?tree=Try&rev=d56be57ecae9

Comment 15

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> Created attachment 769076 [details] [diff] [review]
> telemetry additions part1 rev2
> 
> Brian, I think I should protect against negative numbers for the interval...
> what do you think?

Yes sounds good

Comment 16

[Brian R. Bondy [:bbondy]]

Comment on attachment 769076 [details] [diff] [review]
telemetry additions part1 rev2

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

Please fix the below and the negative check and you can r=me with that without me seeing it again.

::: toolkit/mozapps/update/content/updates.js
@@ +168,5 @@
> +                    "finishedBackground": 16,
> +                    "installed": 17 };
> +    try {
> +      Services.telemetry.getHistogramById("UPDATER_WIZ_LAST_PAGE_CODE").
> +        add(pageMap[pageID]);

I'm not sure what histogram.add does with a null value in case a string above ever changes or if there's a new page that we encounter, but I think it'd be safest to do:
 add(pageMap[pageID] || 0);

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2773,5 @@
>      if (this._incompatibleAddons.length > 0 || !gCanApplyUpdates) {
>        LOG("UpdateService:onUpdateEnded - prompting because there are " +
>            "incompatible add-ons");
>        this._showPrompt(this._update);
> +      this._sendAddonCheckCodePing(PING_ADDON_CHECK_INCOMPAT_ADDONS);

PING_ADDON_CHECK_INCOMPAT_ADDONS is undefined

Comment 17

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

Attachment: 1. telemetry additions part1 rev3

Hey Brian, I'd like you to give this another once over vs. just r=you'ing it since I changed it a tad.

Comment 18

[Brian R. Bondy [:bbondy]]

Comment on attachment 772253 [details] [diff] [review]
1. telemetry additions part1 rev3

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

Interdiff looks good

Comment 19

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

Attachment: 2. main patch and more telemetry additions

Hey Brian, I went with additional telemetry histograms for the two types of background checks so we can differentiate between a system that is checking because of bug 885641 from normal notification checks. After thinking about this some more I expanded the add-on compatibility checks to cover the entire check in two histograms (one for the bug 885641 check and one for the normal background check). I think this should cover everything needed for bug 885641 while providing enough data to have a glimmer as to what is going on. If you can think of more data points that would be of value please let me know. I'll add tests for the new method in a separate patch.

Comment 20

[Brian R. Bondy [:bbondy]]

Comment on attachment 772440 [details] [diff] [review]
2. main patch and more telemetry additions

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

::: toolkit/mozapps/update/nsUpdateService.js
@@ +2300,5 @@
>          if (lastUpdateTimeSeconds > currentTimeSeconds) {
>            try {
>              Services.telemetry.
> +              getHistogramById("UPDATER_INVALID_LASTUPDATETIME_" + idSuffix).
> +              add(1);

nit:
I prefer the 'add' method here and elsewhere to be indented similar to how the multi line code like this is indented:

Services.telemetry.
  getHistogramById(...

Comment 21

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

(In reply to Brian R. Bondy [:bbondy] from comment #20)
> Comment on attachment 772440 [details] [diff] [review]
> 2. main patch and more telemetry additions
> 
> Review of attachment 772440 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/update/nsUpdateService.js
> @@ +2300,5 @@
> >          if (lastUpdateTimeSeconds > currentTimeSeconds) {
> >            try {
> >              Services.telemetry.
> > +              getHistogramById("UPDATER_INVALID_LASTUPDATETIME_" + idSuffix).
> > +              add(1);
> 
> nit:
> I prefer the 'add' method here and elsewhere to be indented similar to how
> the multi line code like this is indented:
> 
> Services.telemetry.
>   getHistogramById(...
It is indented as
Services.telemetry.
  getHistogramById("UPDATER_INVALID_LASTUPDATETIME_" + idSuffix).
  add(1);

Do you mean you prefer the 3rd line to be double indented as follows?
Services.telemetry.
  getHistogramById("UPDATER_INVALID_LASTUPDATETIME_" + idSuffix).
    add(1);

I don't think I have seen that format in the tree before

Comment 22

[Brian R. Bondy [:bbondy]]

My bad, ignore the nit.

Comment 23

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

Attachment: 1. main patch with telemetry additions

Combined patch, carrying over r+

I see one more place where we need more telemetry for this to be accurate and I will submit the patch soonish

Comment 24

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

Attachment: 2. more telemetry additions

I consolidated several of the telemetry additions into the background update check telemetry histogram and added several new ones as well. This should provide a better overall picture of the failure cases when performing a background update check. The one issue I see is that some of the telemetry pings could include resume attempts when a check is retried when going from offline to online but I think this is acceptable at least for now.

Comment 25

[Brian R. Bondy [:bbondy]]

Comment on attachment 773768 [details] [diff] [review]
2. more telemetry additions

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

::: toolkit/components/telemetry/Histograms.json
@@ +2242,5 @@
>      "description": "Updater: externally initiated (typically by the application) background update check result code (see PING_BGUC_* constants defined in /toolkit/mozapps/update/nsUpdateService.js)"
>    },
>    "UPDATER_BACKGROUND_CHECK_CODE_NOTIFY": {
>      "kind": "enumerated",
> +    "n_values": 31,

After these histograms land for the first time you can't change n_values without changing the name of the histogram, so it would be best to increase the value here and above in case there are future values added.

Comment 26

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

Attachment: combined patch with comments addressed

Carrying forward r+

I went with 50 for n_values

Comment 27

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

Attachment: combined patch with comments addressed rev2

n_values for UPDATER_WIZ_LAST_PAGE_CODE should also be increased so I went with 25 for it

What about UPDATER_SERVICE_ERROR_CODE?

Comment 28

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

Attachment: combined patch with comments addressed rev3

Talked with bbondy on irc and decided to go with 100 for UPDATER_SERVICE_ERROR_CODE

Comment 29

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

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/8375418166cd

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8375418166cd