Last Comment Bug 716736 - Re-add performance metrics to AMO ping
: Re-add performance metrics to AMO ping
Status: RESOLVED FIXED
[inbound][qa-]
:
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: P1 major (vote)
: mozilla13
Assigned To: Gregory Szorc [:gps]
:
Mentors:
Depends on: 718532 735069
Blocks: 682356 723178 723323
  Show dependency treegraph
 
Reported: 2012-01-09 16:27 PST by Blair McBride [:Unfocused] (UNAVAILABLE)
Modified: 2012-03-14 07:55 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Conditionally send metadata info (10.18 KB, patch)
2012-01-19 11:50 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Re-add performance data to daily update check, v2 (15.94 KB, patch)
2012-01-30 15:36 PST, Gregory Szorc [:gps]
blair: review-
Details | Diff | Splinter Review
Re-add performance data to daily update check, v3 (18.27 KB, patch)
2012-01-31 13:04 PST, Gregory Szorc [:gps]
blair: review+
Details | Diff | Splinter Review
Re-add performance data to daily update check, v4 (18.33 KB, patch)
2012-01-31 17:05 PST, Gregory Szorc [:gps]
blair: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-09 16:27:44 PST
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.
Comment 1 Gregory Szorc [:gps] 2012-01-19 11:50:05 PST
Created attachment 589932 [details] [diff] [review]
Conditionally send metadata info

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.
Comment 2 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-19 16:04:39 PST
Could you also take into account Dave's suggestion in bug 682356 comment 5? Would make the API a lot cleaner.
Comment 3 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-29 21:46:59 PST
Any update on this? It needs fixed on Fx11, which is about to go into Beta.
Comment 4 Gregory Szorc [:gps] 2012-01-30 11:35:27 PST
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.
Comment 5 Gregory Szorc [:gps] 2012-01-30 15:36:25 PST
Created attachment 592895 [details] [diff] [review]
Re-add performance data to daily update check, v2

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.
Comment 6 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-30 19:36:10 PST
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.
Comment 7 Gregory Szorc [:gps] 2012-01-31 12:59:45 PST
(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.
Comment 8 Gregory Szorc [:gps] 2012-01-31 13:04:01 PST
Created attachment 593199 [details] [diff] [review]
Re-add performance data to daily update check, v3

Incorporated feedback.
Comment 9 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-31 16:16:35 PST
(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.
Comment 10 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-31 16:50:16 PST
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).
Comment 11 Gregory Szorc [:gps] 2012-01-31 17:05:31 PST
Created attachment 593269 [details] [diff] [review]
Re-add performance data to daily update check, v4

Incorporated feedback. I want an explicit review since we will eventually ask for beta approval.
Comment 12 Blair McBride [:Unfocused] (UNAVAILABLE) 2012-01-31 17:13:39 PST
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!
Comment 14 Gregory Szorc [:gps] 2012-01-31 17:27:59 PST
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.
Comment 15 Ed Morley [:emorley] 2012-02-01 10:54:40 PST
https://hg.mozilla.org/mozilla-central/rev/8d8cbdf14308
Comment 16 Alex Keybl [:akeybl] 2012-02-02 06:50:33 PST
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 Alex Keybl [:akeybl] 2012-02-05 14:07:25 PST
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.
Comment 18 Gregory Szorc [:gps] 2012-02-06 10:21:01 PST
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.

Note You need to log in before you can comment on or make changes to this bug.