682356 - Make AddonRepository.getAddonsByIDs() not send performance data

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Add argument to control sending performance data

Currently, the AddonRepository APIs for searching (specifically getAddonsByIDs()) automatically send performance metrics to the server.

Per discussion in bug 534956 (implementing add-on sync), having Sync query these APIs will have the undesired effect of skewing the metrics data.

AddonRepository.getAddonsByIDs() should be modified so that it optionally doesn't send performance data as part of the request and Sync can call it without skewing metrics data.

The attached patch adds an argument to getAddonsByIDs() to control sending of performance data. Patch notes:

* I purposefully made the API backwards compatible. Typically callbacks are passed last. I didn't want to go down the refactor hole.
* Note that the lack of an argument defaults to a true value.
* Is the '=== undefined' a suitable replacement for checking the length of the arguments pseudo-array?
* I still need to write the additional tests.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Gregory Szorc [:gps] from comment #0)
> * Is the '=== undefined' a suitable replacement for checking the length of
> the arguments pseudo-array?

Yep, that's fine - and preferable to using the arguments pseudo-array.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Add argument to control sending performance data

Here is my latest/greatest patch to make sending performance data optional. After some more inspection and some chatting with Mossop in IRC, I settled on the following:

* Change existing API so sending performance data must be explicitly requested
* Change the callers of getAddonsByIDs() to accept an argument saying whether to send performance data
* Change the daily ping to be the only thing that sends performance data

I had to create a new preference to support this since the code is using string manipulation to build the URI query string. I didn't feel like creeping scope to build the URIs properly.

I have verified the patch with `make -C toolkit/mozapps/extensions/tests xpcshell-tests`:

INFO | Result summary:
INFO | Passed: 204
INFO | Failed: 0
INFO | Todo: 0

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 559235 [details] [diff] [review]
Add argument to control sending performance data

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

Can we get some tests the repopulateCache includes the perf data in the ping and getAddonsByIDs doesn't.

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +588,2 @@
>     */
> +  repopulateCache: function(aIds, aCallback, aSendPerformance) {

We don't need an aSendPerformance argument here, this is what is used for the daily ping so just assume we always want perf data included with it.

@@ +635,2 @@
>     */
> +  cacheAddons: function(aIds, aCallback, aSendPerformance) {

We don't need an argument here either

@@ +737,2 @@
>     */
> +  getAddonsByIDs: function(aIDs, aCallback, aSendPerformance) {

I don't think I want aSendPerformance in the arguments for getAddonsByIDs, it's an internal thing that I want to be able to change without breaking add-on compatibility with this function. As we discussed rename this function to getAddonsByIDsInternal and have a simple getAddonsByIDs that calls it with false as the last argument.

@@ +762,5 @@
> +                                         startupInfo.process;
> +      }
> +    }
> +
> +    let pref = havePerfData ? PREF_GETADDONS_BYIDS_PERF : PREF_GETADDONS_BYIDS;

We should check with the metrics guys on this I think, having data that perf data was missing for the daily ping might be useful.

Comment 4

[Gregory Szorc [:gps]]

(In reply to Dave Townsend (:Mossop) from comment #3)
> Can we get some tests the repopulateCache includes the perf data in the ping
> and getAddonsByIDs doesn't.

FWIW, the existing tests appear to exercise this code, which is why I omitted writing new tests. However, I can see why you would want an explicit test for this change. I'll write some.

> ::: toolkit/mozapps/extensions/AddonRepository.jsm
> @@ +588,2 @@
> >     */
> > +  repopulateCache: function(aIds, aCallback, aSendPerformance) {
> 
> We don't need an aSendPerformance argument here, this is what is used for
> the daily ping so just assume we always want perf data included with it.

I humbly disagree on the grounds of API purity. The API is "repopulate cache" not "repopulate cache and send a daily performance data ping." Couple that with the hidden caveat that sending the performance data more than once a day will mess up metrics on the server side, and I really think it is the best interest of API design to explicitly require the act of sending metrics be specified as an argument, not as a core implementation of the function.

Along that vein, what if someone else wants to use this function and doesn't want to send performance data? Tightly coupling sending performance data with the implementation is constraining to API use (not to mention undocumented) and therefore should be avoided.

That being said, this isn't my module, so I'll defer to the reviewer.

> @@ +635,2 @@
> >     */
> > +  cacheAddons: function(aIds, aCallback, aSendPerformance) {
> 
> We don't need an argument here either

I agree.

> @@ +737,2 @@
> >     */
> > +  getAddonsByIDs: function(aIDs, aCallback, aSendPerformance) {
> 
> I don't think I want aSendPerformance in the arguments for getAddonsByIDs,
> it's an internal thing that I want to be able to change without breaking
> add-on compatibility with this function. As we discussed rename this
> function to getAddonsByIDsInternal and have a simple getAddonsByIDs that
> calls it with false as the last argument.

See my argument above.
 
> @@ +762,5 @@
> > +                                         startupInfo.process;
> > +      }
> > +    }
> > +
> > +    let pref = havePerfData ? PREF_GETADDONS_BYIDS_PERF : PREF_GETADDONS_BYIDS;
> 
> We should check with the metrics guys on this I think, having data that perf
> data was missing for the daily ping might be useful.

Good catch. I did refactor the logic slightly, so it is possible we regressed. Unfortunately, it wasn't caught by a unit test. So, we'll need someone to weigh in. Can you CC the appropriate people?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #4)
> (In reply to Dave Townsend (:Mossop) from comment #3)
> > Can we get some tests the repopulateCache includes the perf data in the ping
> > and getAddonsByIDs doesn't.
> 
> FWIW, the existing tests appear to exercise this code, which is why I
> omitted writing new tests. However, I can see why you would want an explicit
> test for this change. I'll write some.
> 
> > ::: toolkit/mozapps/extensions/AddonRepository.jsm
> > @@ +588,2 @@
> > >     */
> > > +  repopulateCache: function(aIds, aCallback, aSendPerformance) {
> > 
> > We don't need an aSendPerformance argument here, this is what is used for
> > the daily ping so just assume we always want perf data included with it.
> 
> I humbly disagree on the grounds of API purity. The API is "repopulate
> cache" not "repopulate cache and send a daily performance data ping." Couple
> that with the hidden caveat that sending the performance data more than once
> a day will mess up metrics on the server side, and I really think it is the
> best interest of API design to explicitly require the act of sending metrics
> be specified as an argument, not as a core implementation of the function.
> 
> Along that vein, what if someone else wants to use this function and doesn't
> want to send performance data? Tightly coupling sending performance data
> with the implementation is constraining to API use (not to mention
> undocumented) and therefore should be avoided.

I think the aim would be for the API to be designed in such a way that sending the perf data with the ping should never seem optional to a caller. There is only one case where we want to do it and it is only ever called by our internal code. So I don't want to see that argument on any method that others might use. As you say it's possible that repopulateCache might be useful to others so perhaps an alternate solution is to leave that, without the argument and create a new method, maybe backgroundDailyUpdate which is explicitly labelled as only for the add-ons manager to call, that being the only method that sends the perf data.

Comment 6

[Gregory Szorc [:gps]]

Now that telemetry is included in Firefox, do we need this performance data pingback at all?

Comment 7

[Dave Townsend [:mossop]]

As far as I know this set of data isn't in telemetry, but I'm all for removing it if it is no longer needed

Comment 8

[Justin Scott [:fligtar]]

To my knowledge telemetry doesn't include the specific add-ons installed and therefore we can't correlate slow start-up with add-ons, so I think it's still needed.

Comment 9

[(dormant account)]

(In reply to Justin Scott [:fligtar] from comment #8)
> To my knowledge telemetry doesn't include the specific add-ons installed and
> therefore we can't correlate slow start-up with add-ons, so I think it's
> still needed.

It does not yet(until 668392 is relanded). However telemetry is opt-in, so we'll be getting strictly less addon startup impact data. However st

Comment 10

[Dave Townsend [:mossop]]

Spoke with Taras on IRC and now that bug 668392 has landed he's happy to just remove this additional data from the ping, so let's just do that rather than complicate things here.

Comment 11

[Gregory Szorc [:gps]]

Attachment: Remove metrics from metadata requests, v1

This patch effectively reverts bug 623950 now that data is in telemetry (bug 668392).

I didn't revert all of the query string arguments (appOS and appVersion are still there). I'm not sure if those are relevant to the query API or not. It would be trivial to rip those out.

I verified xpcshell tests passed. 2 chrome mochitests failed with "MockFilePicker is undefined" errors. But, I don't think those are related to this patch.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 573601 [details] [diff] [review]
Remove metrics from metadata requests, v1

Looks good to me

Comment 13

[Gregory Szorc [:gps]]

Thanks, Dave.

https://hg.mozilla.org/integration/mozilla-inbound/rev/259bae11858b

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/259bae11858b

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Just found that this was missing from SM (filed bug 713119). Seems mobile/ is also affected:

http://mxr.mozilla.org/comm-central/search?string=time_main

Have fun.

Comment 16

[Justin Scott [:fligtar]]

Wow, this bug considerably changed since the last time I saw it. The only reason our add-on performance data is useful is because we have so much of it. Limiting it to the few users who are using telemetry is effectively removing it entirely.