708134 - Add-on sync AMO downloads should not skew metrics

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P2)

Description

[Gregory Szorc [:gps]]

This is a follow-up to add-on sync to address the problem of downloads from AMO skewing download metrics.

We don't want automatic add-on installs via Sync to increment the download count. This likely involves changing the "src=api" query string parameter to "src=sync" or something similar.

Comment 1

[Gregory Szorc [:gps]]

This is targeted for Firefox 11 because we don't want this to go to the masses as stable and skew metrics. I'm hoping to get this fixed while 11 is still in Aurora.

Comment 2

[Wil Clouser [:clouserw]]

Daniel on the metrics team handles the processing here.

Comment 3

[Justin Scott [:fligtar]]

This is mainly needed for add-on installs; updates are pulled from the mirrors and don't get counted in metrics at all.

If possible, it would also be good to add a parameter to the post-install add-on metadata ping so it can be differentiated from the post-install ping of a real installation. Thanks!

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

At the moment, the location of the XPI is found out via querying AMO's API, via AddonRepository (RepositoryAddon.sourceURI). That URL has ?src=api in it... doing a string replace on that will work and is easily contained within the sync code. Although it does feel rather awkward, IMO.

Gregory mentioned maybe switching to using Addon.sourceURI - for addons that have been updated since they were installed, this will point to the location of the last update, which isn't tracked (see comment 3). That kinda solves not counting those installs in normal metrics... but it means we can never have metrics for how many addons are installed via sync (which we want want to do in the future). For addons that haven't been updated, it will point to a URL that does count towards the download count, and that URL should have a src=whatever parameter.

Rewriting URLs is fine when it's a AMO URL, but what to do if it's not a AMO URL? IIRC, we don't support syncing those addons now, but plan to in the future. Rewriting those URLs could potentially break them.

As for the post-install metadata ping... that seems like it would be doable, but awkward - kinda feels like contaminating the AddonsManager API :\ AMO is queried as soon as the XPI is downloaded and the install.rdf manifest can be read - ie, before the install starts. Would need to add a flag to AddonManager.getInstallForURL(), and pass that all the way through to eventually where AddonRepoistory.cacheAddons() is called. If you do want to do that, having it generic and work like the AddonManager.UPDATE_WHEN_* constants would be nice.

Comment 5

[Daniel Einspanjer [:dre] [:deinspanjer]]

When the src param changes, just remember to add the new one to the valid list in the amo db table so we pick it up and export the id.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Part 1: Rewrite sourceURI when installing add-ons through Sync, v1

This patch changes the src query string parameter on sourceURI when an add-on is installed by Sync.

I'm not 100% confident I changed the right value. Blair can verify that.

I also haven't explicitly tested through the browser. But, the patch includes an xpcshell test verifying the URL rewriting does work, so I have no reason to believe it won't work in the real world.

Comment 7

[Gregory Szorc [:gps]]

fligtar: can you provide guidance on your long-term vision for the install ping? Currently, the request is just a repository search: it contains a list (likely of 1 element) of the public add-on IDs and the API version. We're trying to figure out what exactly we need to add to the request.

Should we add a parameter that says how the add-on was installed? Should we add a one-off identifying Sync as the source?

To further complicate things, the API sending this metadata ping is the same one used for the daily ping AFAICT (AddonRepository.getAddonsByIDs). It is the same one that was patched in bug 682356 and is now being reverted in bug 716736. What about recurring pings from Sync installs? Do these need to identify they came from Sync (necessitating new storage support for this field)?

As far as metrics go, is there some kind of logic on the server that distinguishes between the daily "performance ping" and the "install ping?" If so, perhaps now is a good time to explicitly call out the differences between these pings and expose the differences through different APIs in the client? Currently, everything is amassed in one function and it is impossible to tell from looking at the code what the expectations are.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Gregory Szorc [:gps] from comment #7)
> To further complicate things, the API sending this metadata ping is the same
> one used for the daily ping AFAICT (AddonRepository.getAddonsByIDs). It is
> the same one that was patched in bug 682356 and is now being reverted in bug
> 716736.

If it's good enough to differentiate metadata pings related to the periodic background update check, and *everything* else (installs, installs done by Sync, addons querying for their own reasons, etc), then that kills two birds with one much simpler stone.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 587531 [details] [diff] [review]
Part 1: Rewrite sourceURI when installing add-ons through Sync, v1

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

::: services/sync/modules/engines/addons.js
@@ +975,5 @@
> +          }
> +
> +          let params = addon.sourceURI.query.split("&");
> +          for (let [index, param] in Iterator(params)) {
> +            if (param.substr(0, 4) == "src=") {

.indexOf("src") == 0, please.

@@ +976,5 @@
> +
> +          let params = addon.sourceURI.query.split("&");
> +          for (let [index, param] in Iterator(params)) {
> +            if (param.substr(0, 4) == "src=") {
> +              params[index] = "src=sync";

*momentary double-take about whether JS allows modification during iteration*

Is that the param that metrics wants?

Is there ever likely to be some other kind of attribute here? E.g., Sync version, storage version of add-on sync, discriminating between different products? "sync" seems like an awfully large net to cast. Pretty soon you'll be in Python-land with "sync2".

@@ +980,5 @@
> +              params[index] = "src=sync";
> +            }
> +          }
> +
> +          addon.sourceURI.query = params.join("&");

Oh, if only JS had a proper URL-managing library.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Part 1: Rewrite sourceURI when installing add-ons through Sync, v2

Use indexOf and Array.map in response to feedback.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 587772 [details] [diff] [review]
Part 1: Rewrite sourceURI when installing add-ons through Sync, v2

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

In getInstallFromSearchResult(), you'll need to make it so it never uses AddonSearchResult.install, which will be using the old URL (with src=api).

Comment 12

[Gregory Szorc [:gps]]

Attachment: Part 1: Rewrite sourceURI when installing add-ons through Sync, v3

Good catch, Blair! I made the modification so we always obtain the AddonInstall explicitly. And, I added a test to ensure the change is reflected in the AddonInstall.sourceURI.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 587914 [details] [diff] [review]
Part 1: Rewrite sourceURI when installing add-ons through Sync, v3

rnewman: when you r+, please specify how you would like me to land this. I'd like to keep the bug around to track the metadata ping issue. But I don't want the patch to linger either. Since the metadata ping issue fix is stalled (waiting on fligtar), I think I'd be OK with filing a follow-up for that. If so, I'll commit minus the "Part 1" in the commit message, pending r+ of course.

Comment 14

[Richard Newman [:rnewman]]

Sorry, missed the question.

Land in m-i, follow-up for ping.

Comment 15

[Gregory Szorc [:gps]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf79cb6e973

Will file follow-up to deal with ping.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/6bf79cb6e973

Comment 17

[Justin Scott [:fligtar]]

(In reply to Gregory Szorc [:gps] from comment #7)

The metadata ping and install ping are pretty much the same, and the way we tell them apart for metrics purposes is that the metadata ping will have the default theme's GUID in it, while an install ping will only have the add-on that was just installed. Explicitly differentiating them would be helpful, especially for other apps like mobile which doesn't have a default theme so we can't tell the difference at all.

My main concern regarding install sources was with the API download URL, which should indicate it was downloaded via sync and not the user. A parameter on the metadata install lookup would be interesting but isn't necessary -- we don't currently use that ping for anything metrics-wise and simply filter them out of the daily metadata background checks.

Comment 18

[Tracy Walker [:tracy]]

gps:  from QA pov, what should be looking for to verify this bug?  STR's much appreciated

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 587914 [details] [diff] [review]
Part 1: Rewrite sourceURI when installing add-ons through Sync, v3

[Approval Request Comment]
Regression caused by (bug #): 534956 (initial add-on sync landing)
User impact if declined: No user impact. AMO metrics would be skewed.
Testing completed (on m-c, etc.): Code has baked in 12 and 13 for a few weeks.
Risk to taking this patch (and alternatives if risky): Not much.
String changes made by this patch: None

I should have asked for 11 approval shortly after this landed. I forgot. Firefox 11 (currently Aurora) is still impacted. If we don't take this patch, AMO metrics could be skewed for add-on sync users using Firefox 11.

Comment 20

[Gregory Szorc [:gps]]

(In reply to Tracy Walker [:tracy] from comment #18)
> gps:  from QA pov, what should be looking for to verify this bug?  STR's
> much appreciated

If you attempt to sync an add-on installed from AMO, the first time you install the add-on, its XPI URL will have 'src=api' in the query string or something similar. I believe the URL will be dumped to the Error Console if extensions.logging.enabled is true. When this add-on is synced, the 'src' query string argument should be set to 'sync'.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 587914 [details] [diff] [review]
Part 1: Rewrite sourceURI when installing add-ons through Sync, v3

[Triage Comment]
This is blocking 725083 due to bitrot, is considered low-risk, prevents skewing of metrics, and has STR for QA to verify. Approved for Beta 11. Please land today.

Comment 22

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d26f36024d3c