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)

defect

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox11 --- fixed
firefox12 --- fixed

People

(Reporter: Unfocused, Assigned: gps)

References

Details

(Whiteboard: [inbound][qa-])

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Conditionally send metadata info (obsolete) — Splinter Review
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.
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
Any update on this? It needs fixed on Fx11, which is about to go into Beta.
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.
Blocks: 682356
No longer depends on: 682356
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)
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-
(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.
Incorporated feedback.
Attachment #592895 - Attachment is obsolete: true
Attachment #593199 - Flags: review?(bmcbride)
(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.
Attachment #593199 - Attachment is patch: true
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+
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)
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+
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?
https://hg.mozilla.org/mozilla-central/rev/8d8cbdf14308
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Blocks: 723323
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 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+
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.
Whiteboard: [inbound] → [inbound][qa-]
Depends on: 735069
Depends on: 718532
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: