Provide method to check for updates to support bug 885641

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

unspecified
mozilla25
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

52.69 KB, patch
rstrong
: review+
Details | Diff | Splinter Review
This will allow the code in bug 885641 to be simplified and better support reporting in telemetry.
Created attachment 766989 [details] [diff] [review]
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
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #766989 - Flags: review?(netzen)
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.
Attachment #766989 - Flags: review?(taras.mozilla)
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.
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

5 years ago
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.
Attachment #766989 - Flags: review?(taras.mozilla) → review+
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
Attachment #766989 - Flags: review?(netzen) → review+
Taras, can I get a response for the question in comment #4?
Flags: needinfo?(taras.mozilla)

Comment 8

5 years ago
(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
Flags: needinfo?(taras.mozilla)
Created attachment 768790 [details] [diff] [review]
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.
Attachment #766989 - Attachment is obsolete: true
Attachment #768790 - Flags: review?(netzen)
Created attachment 768798 [details] [diff] [review]
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.
Attachment #768798 - Flags: feedback?(netzen)
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 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
Attachment #768790 - Flags: review?(netzen) → review-
Attachment #768798 - Flags: feedback?(netzen) → feedback+
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?
Attachment #768790 - Attachment is obsolete: true
Attachment #768798 - Attachment is obsolete: true
Attachment #769076 - Flags: review?(netzen)
(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 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
Attachment #769076 - Flags: review?(netzen)
Created attachment 772253 [details] [diff] [review]
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.
Attachment #769076 - Attachment is obsolete: true
Attachment #772253 - Flags: review?(netzen)
Comment on attachment 772253 [details] [diff] [review]
1. telemetry additions part1 rev3

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

Interdiff looks good
Attachment #772253 - Flags: review?(netzen) → review+
Created attachment 772440 [details] [diff] [review]
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.
Attachment #772440 - Flags: review?(netzen)
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(...
Attachment #772440 - Flags: review?(netzen) → review+
(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
My bad, ignore the nit.
Created attachment 773509 [details] [diff] [review]
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
Attachment #772253 - Attachment is obsolete: true
Attachment #772440 - Attachment is obsolete: true
Attachment #773509 - Flags: review+
Created attachment 773768 [details] [diff] [review]
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.
Attachment #773768 - Flags: review?(netzen)
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.
Attachment #773768 - Flags: review?(netzen) → review+
Created attachment 774081 [details] [diff] [review]
combined patch with comments addressed

Carrying forward r+

I went with 50 for n_values
Attachment #773509 - Attachment is obsolete: true
Attachment #773768 - Attachment is obsolete: true
Attachment #774081 - Flags: review+
Created attachment 774086 [details] [diff] [review]
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?
Attachment #774081 - Attachment is obsolete: true
Attachment #774086 - Flags: review+
Created attachment 774089 [details] [diff] [review]
combined patch with comments addressed rev3

Talked with bbondy on irc and decided to go with 100 for UPDATER_SERVICE_ERROR_CODE
Attachment #774086 - Attachment is obsolete: true
Attachment #774089 - Flags: review+
Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/8375418166cd
Flags: in-testsuite+
Target Milestone: --- → mozilla25
https://hg.mozilla.org/mozilla-central/rev/8375418166cd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 763243
You need to log in before you can comment on or make changes to this bug.