Closed
Bug 716736
Opened 12 years ago
Closed 12 years ago
Re-add performance metrics to AMO ping
Categories
(Toolkit :: Add-ons Manager, defect, P1)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: Unfocused, Assigned: gps)
References
Details
(Whiteboard: [inbound][qa-])
Attachments
(1 file, 3 obsolete files)
18.33 KB,
patch
|
Unfocused
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Bug 682356 landed ~2 months ago, removing the performance data from the metadata ping to AMO (effectively backing out bug 623950), after bug 668392 added the list of addons to telemetry. However, it turns out that the data from Telemetry is insufficient for AMO's needs. So bug 623950 needs to be re-landed (ie, undo the change bug 682356 did), but with the performance data only being sent when it's part of the periodic background update check. Note that bug 682356 landed before the previous merge - so this fix will need to land on Aurora too.
Assignee | ||
Comment 1•12 years ago
|
||
Per Blair's request, this is the original patch from bug 682356 with bit rot removed. xpcshell tests fail. Not sure how horribly broken things are. But, I /think/ this might be a start.
Reporter | ||
Comment 2•12 years ago
|
||
Could you also take into account Dave's suggestion in bug 682356 comment 5? Would make the API a lot cleaner.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•12 years ago
|
||
Any update on this? It needs fixed on Fx11, which is about to go into Beta.
Assignee | ||
Comment 4•12 years ago
|
||
I got sidetracked by urgent Sync bugs and a head cold. I'll try to look at this today. But, I'm still not on top of my game since I'm still not feeling well.
Updated•12 years ago
|
Assignee | ||
Comment 5•12 years ago
|
||
I think this patch addresses everything we talked about on IRC. I basically renamed the old getAddonsByIDs and repopulateCache to internal functions, added a "send performance" argument, then recreated their "public" functions to wrap the new internal one without sending performance data. I also added a backgroundUpdateCheck API and hooked up AddonManager to use that. I restored the deleted test from https://hg.mozilla.org/mozilla-central/rev/259bae11858b under browser_addonrepository_performance.js with minor changes for the new world. xpcshell and chrome mochitests pass on my dev machine.
Attachment #589932 -
Attachment is obsolete: true
Attachment #592895 -
Flags: review?(bmcbride)
Reporter | ||
Comment 6•12 years ago
|
||
Comment on attachment 592895 [details] [diff] [review] Re-add performance data to daily update check, v2 Review of attachment 592895 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +761,5 @@ > var ids = [a.id for each (a in aAddons) if (a.id != hotfixID)]; > > // Repopulate repository cache first, to ensure compatibility overrides > // are up to date before checking for addon updates. > + scope.AddonRepository.backgroundUpdateCheck(ids, function BUC_repopulateCacheCallback() { Rename BUC_repopulateCacheCallback too. ::: toolkit/mozapps/extensions/AddonRepository.jsm @@ +648,5 @@ > + > + if (aSendPerformance) { > + self._beginGetAddons(aAddons, cb, true); > + } else { > + self._beginGetAddons(aAddons, cb, false); Just pass in aSendPerformance directly: self._beginGetAddons(aAddons, cb, aSendPerformance); @@ +764,5 @@ > + /** > + * Begins a search of add-ons, potentially sending performance data. > + * > + * @param aIDs > + * Array of ids to search for Nit: Missing fullstop. @@ +796,5 @@ > + } > + } > + > + let pref = aSendPerformance ? PREF_GETADDONS_BYIDS_PERFORMANCE > + : PREF_GETADDONS_BYIDS; Will need to check whether the PREF_GETADDONS_BYIDS_PERFORMANCE pref has a value (not all applications will need that pref, so we shouldn't require it), and if not then fallback to PREF_GETADDONS_BYIDS. @@ +853,5 @@ > + * data. It is meant to be called as part of the daily update ping. It should > + * not be used for any other purpose. Use repopulateCache instead. > + * > + * @param aIDs > + * Array of add-on IDs to repopulate the cache with Nit: Missing full-stop. ::: toolkit/mozapps/extensions/test/browser/browser_addonrepository_performance.js @@ +58,5 @@ > + "http://127.0.0.1:8888/extensions-dummy/metadata?appOS=%OS%&appVersion=%VERSION%&tMain=%TIME_MAIN%&tFirstPaint=%TIME_FIRST_PAINT%&tSessionRestored=%TIME_SESSION_RESTORED%"); > + > + registerCleanupFunction(function() { > + Services.obs.removeObserver(observe, "http-on-modify-request"); > + Services.prefs.clearUserPref(PREF); Instead of clearing the pref here (which will clear it to the value in firefox.js, which is a URL that will hit the network), add extensions.getAddons.getWithPerformance.url to the gRestorePrefs array in head.js so it gets automatically reset to a URL that won't hit the network: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/head.js#54 Also, that pref will need a value set in automation.py.in: https://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#404 @@ +61,5 @@ > + Services.obs.removeObserver(observe, "http-on-modify-request"); > + Services.prefs.clearUserPref(PREF); > + }); > + > + AddonRepository._beginGetAddons(["test1@tests.mozilla.org"], { If you call AddonRepository.backgroundUpdateCheck here instead of _beginGetAddons, you get some extra test coverage for free.
Attachment #592895 -
Flags: review?(bmcbride) → review-
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Blair McBride (:Unfocused) from comment #6) > Comment on attachment 592895 [details] [diff] [review] > ::: toolkit/mozapps/extensions/AddonRepository.jsm > @@ +648,5 @@ > > + > > + if (aSendPerformance) { > > + self._beginGetAddons(aAddons, cb, true); > > + } else { > > + self._beginGetAddons(aAddons, cb, false); > > Just pass in aSendPerformance directly: self._beginGetAddons(aAddons, cb, > aSendPerformance); /me hangs head in shame. This is what happens when you program with a head cold. > > @@ +61,5 @@ > > + Services.obs.removeObserver(observe, "http-on-modify-request"); > > + Services.prefs.clearUserPref(PREF); > > + }); > > + > > + AddonRepository._beginGetAddons(["test1@tests.mozilla.org"], { > > If you call AddonRepository.backgroundUpdateCheck here instead of > _beginGetAddons, you get some extra test coverage for free. I tried this initially but the test was hanging for some unknown reason. My guess is some hacky event loop spinning would be needed? I don't think it is a big deal because other tests trigger a background update check.
Assignee | ||
Comment 8•12 years ago
|
||
Incorporated feedback.
Attachment #592895 -
Attachment is obsolete: true
Attachment #593199 -
Flags: review?(bmcbride)
Reporter | ||
Comment 9•12 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #7) > > If you call AddonRepository.backgroundUpdateCheck here instead of > > _beginGetAddons, you get some extra test coverage for free. > > I tried this initially but the test was hanging for some unknown reason. My > guess is some hacky event loop spinning would be needed? I don't think it is > a big deal because other tests trigger a background update check. Huh. Would be curious to know why, but then other tests are passing, so ok.
Reporter | ||
Updated•12 years ago
|
Attachment #593199 -
Attachment is patch: true
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 593199 [details] [diff] [review] Re-add performance data to daily update check, v3 Review of attachment 593199 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the following comment addressed. ::: toolkit/mozapps/extensions/AddonRepository.jsm @@ +795,5 @@ > + if (aSendPerformance) { > + try { > + Services.prefs.getCharPref(PREF_GETADDONS_BYIDS_PERFORMANCE); > + pref = PREF_GETADDONS_BYIDS_PERFORMANCE; > + } catch (ex if ex.result == Cr.NS_ERROR_NOT_FOUND) {} Move this to before gathering the performance data - no sense in gathering that data if it's not going to be used. Additional nit: Since you're not actually using the result of getCharPref(), using getPrefType() is much cleaner (and it never throws).
Attachment #593199 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Incorporated feedback. I want an explicit review since we will eventually ask for beta approval.
Attachment #593199 -
Attachment is obsolete: true
Attachment #593269 -
Flags: review?(bmcbride)
Reporter | ||
Comment 12•12 years ago
|
||
Comment on attachment 593269 [details] [diff] [review] Re-add performance data to daily update check, v4 Review of attachment 593269 [details] [diff] [review]: ----------------------------------------------------------------- Good to go!
Attachment #593269 -
Flags: review?(bmcbride) → review+
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d8cbdf14308
Whiteboard: [inbound]
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 593269 [details] [diff] [review] Re-add performance data to daily update check, v4 [Approval Request Comment] Regression caused by (bug #): 682356 User impact if declined: Loss of apparently important performance data from daily AMO pings Testing completed (on m-c, etc.): None yet. Requesting approval as early as possible so things show up on radar. Risk to taking this patch (and alternatives if risky): Should be pretty low risk. Restores previous behavior albeit with a slightly refactored API. Public API didn't change, so add-on developers shouldn't be affected. String changes made by this patch: None. Added a new preference though.
Attachment #593269 -
Flags: approval-mozilla-beta?
Attachment #593269 -
Flags: approval-mozilla-aurora?
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d8cbdf14308
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Reporter | ||
Updated•12 years ago
|
status-firefox10:
affected → ---
Comment 16•12 years ago
|
||
Our next beta build is Tuesday - I'd like this to bake on m-c for a couple of more days before landing there. I'll leave this in the queue for now.
Comment 17•12 years ago
|
||
Comment on attachment 593269 [details] [diff] [review] Re-add performance data to daily update check, v4 [Triage Comment] No issues reported - approving for Beta 11 and Aurora 12.
Attachment #593269 -
Flags: approval-mozilla-beta?
Attachment #593269 -
Flags: approval-mozilla-beta+
Attachment #593269 -
Flags: approval-mozilla-aurora?
Attachment #593269 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 18•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/75812bd77ebc https://hg.mozilla.org/releases/mozilla-aurora/rev/10b4c33374b7 The patch didn't apply cleanly on beta. But, I think it was due to a line offset issue or something weird. It was getting tripped up on a single line addition and there was no context change around the affected line as far as I could tell. Tools.
You need to log in
before you can comment on or make changes to this bug.
Description
•