Closed
Bug 886545
Opened 11 years ago
Closed 11 years ago
Provide method to check for updates to support bug 885641
Categories
(Toolkit :: Application Update, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)
References
Details
Attachments
(1 file, 10 obsolete files)
52.69 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
This will allow the code in bug 885641 to be simplified and better support reporting in telemetry.
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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)
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
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•11 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 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
Taras, can I get a response for the question in comment #4?
Flags: needinfo?(taras.mozilla)
Comment 8•11 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)
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
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•11 years ago
|
||
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-
Updated•11 years ago
|
Attachment #768798 -
Flags: feedback?(netzen) → feedback+
Assignee | ||
Comment 13•11 years ago
|
||
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)
Assignee | ||
Comment 14•11 years ago
|
||
Comment 15•11 years ago
|
||
(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•11 years ago
|
||
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)
Assignee | ||
Comment 17•11 years ago
|
||
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 18•11 years ago
|
||
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+
Assignee | ||
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
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+
Assignee | ||
Comment 21•11 years ago
|
||
(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•11 years ago
|
||
My bad, ignore the nit.
Assignee | ||
Comment 23•11 years ago
|
||
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+
Assignee | ||
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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+
Assignee | ||
Comment 26•11 years ago
|
||
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+
Assignee | ||
Comment 27•11 years ago
|
||
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+
Assignee | ||
Comment 28•11 years ago
|
||
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+
Assignee | ||
Comment 29•11 years ago
|
||
Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/8375418166cd
Flags: in-testsuite+
Target Milestone: --- → mozilla25
Comment 30•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•