668392 - Include enabled addons + persona in telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[(dormant account)]

Attachment: addons + personas

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

ouch, you really want to fail silently in XPIProvider?

Comment 2

[(dormant account)]

(In reply to comment #1)
> ouch, you really want to fail silently in XPIProvider?

I don't see why not

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

hmm, mama told me never to leave catch-all exceptions not handled. But I'm just wondering if for some reason the addon list will not serialize and we'll have no notification, but I guess we can catch it from the data entries anyway. nevermind

Comment 4

[Dave Townsend [:mossop]]

Comment on attachment 543007 [details] [diff] [review]
addons + personas

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

Should add some tests to this.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +1739,5 @@
> +    
> +    try {
> +      const TelemetryPing = Cc["@mozilla.org/base/telemetry-ping;1"].getService(Ci.nsIObserver);
> +      TelemetryPing.observe(null, "Add-ons", data);
> +    } catch (e) { }

The previous block has no catch block because it is an expected condition (Sometimes crash reporting is disabled in which case annotating throws). I don't think this new bit can ever throw can it? If not the try...catch seems unnecessary.

Comment 5

[(dormant account)]

(In reply to comment #4)
> Comment on attachment 543007 [details] [diff] [review] [review]
> addons + personas
> 
> Review of attachment 543007 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Should add some tests to this.

Dave, I'm stuck on this, could use some help.

Testing the persona bit is tricky. LightweightThemeManager tests benefit from having the ability to initialize the addon/etc manager. This is a good chunk of code. Do you expect me to copy code from addon manager test harness or can we skip this?

Testing the addon list is just a matter of simulating the observer call, that's no problem.
> 
> ::: toolkit/mozapps/extensions/XPIProvider.jsm
> @@ +1739,5 @@
> > +    
> > +    try {
> > +      const TelemetryPing = Cc["@mozilla.org/base/telemetry-ping;1"].getService(Ci.nsIObserver);
> > +      TelemetryPing.observe(null, "Add-ons", data);
> > +    } catch (e) { }
> 
> The previous block has no catch block because it is an expected condition
> (Sometimes crash reporting is disabled in which case annotating throws). I
> don't think this new bit can ever throw can it? If not the try...catch seems
> unnecessary.

I will get rid of that in the next patch once I know how resolve the lwt problem above.

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> (In reply to comment #4)
> > Comment on attachment 543007 [details] [diff] [review] [review] [review]
> > addons + personas
> > 
> > Review of attachment 543007 [details] [diff] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > Should add some tests to this.
> 
> Dave, I'm stuck on this, could use some help.
> 
> Testing the persona bit is tricky. LightweightThemeManager tests benefit
> from having the ability to initialize the addon/etc manager. This is a good
> chunk of code. Do you expect me to copy code from addon manager test harness
> or can we skip this?

I don't think you'd need to copy much code at all to test this. Since the telemetry test already creates an nsIXULAppInfo I think really the only code you need to initialise the add-ons manager are these 5 lines: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/head_addons.js#298

After that you ought to just be able to set a persona using LightweightThemeManager.currentTheme = { ... }.

> Testing the addon list is just a matter of simulating the observer call,
> that's no problem.

So I was actually more concerned with verifying that the add-ons manager is calling the telemetry service with the list of add-ons, but given how the code is (and the fact we already test that the crash reporting annotation works) I wonder if that is really a worthwhile use of time, maybe not.

Comment 7

[(dormant account)]

Attachment: patch with nonworking test

> 
> I don't think you'd need to copy much code at all to test this. Since the
> telemetry test already creates an nsIXULAppInfo I think really the only code
> you need to initialise the add-ons manager are these 5 lines:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/xpcshell/head_addons.js#298
> 
> After that you ought to just be able to set a persona using
> LightweightThemeManager.currentTheme = { ... }.
> 
It seems so simple but I tried that and it didn't work. 

See the do_check_true(LightweightThemeManager.currentTheme != undefined) in the test.

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> Created attachment 548638 [details] [diff] [review] [diff] [details] [review]
> patch with nonworking test
> 
> > 
> > I don't think you'd need to copy much code at all to test this. Since the
> > telemetry test already creates an nsIXULAppInfo I think really the only code
> > you need to initialise the add-ons manager are these 5 lines:
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > test/xpcshell/head_addons.js#298
> > 
> > After that you ought to just be able to set a persona using
> > LightweightThemeManager.currentTheme = { ... }.
> > 
> It seems so simple but I tried that and it didn't work. 
> 
> See the do_check_true(LightweightThemeManager.currentTheme != undefined) in
> the test.

Sorry for the long delay here. I forgot one other thing. The add-ons manager needs a defined profile directory so put do_get_profile(); at the start of run_test and things will work right.

Comment 9

[(dormant account)]

Attachment: With working test

Thanks, that little detail solved it.

Comment 10

[(dormant account)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdffde7c3b14

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/fdffde7c3b14

Comment 12

[Daniel Veditz [:dveditz]]

This kind of data verges on "personally identifiable" and is quite a bit different from the type of anonymous statistical data presented when we did the security review of the feature. In fact this is the type of thing that prompted us to request that new types of data seek a review before being added -- did that happen? Maybe it did, we've got a big team now, but I don't see any bugs/wiki pages linked here to indicate that.

Some of that information can already be gleaned from AMO update check where it's already covered by our privacy policy.

Comment 13

[(dormant account)]

(In reply to Daniel Veditz from comment #12)
> This kind of data verges on "personally identifiable" and is quite a bit
> different from the type of anonymous statistical data presented when we did
> the security review of the feature. In fact this is the type of thing that
> prompted us to request that new types of data seek a review before being
> added -- did that happen? Maybe it did, we've got a big team now, but I
> don't see any bugs/wiki pages linked here to indicate that.

I did not request security review for this since it's already sent in AMO ping. As the privacy policy goes this seems covered under "user interface features, memory, and hardware configuration."

 We aren't collecting persona + addons as an alternative to AMO. The idea is to correlate slow performance in certain metrics with installed addons. This can not be done through AMO check.

Comment 14

[Daniel Veditz [:dveditz]]

I'll buy that. Sid will still want a privacy-team review.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Taras Glek (:taras) from comment #13)
> I did not request security review for this since it's already sent in AMO
> ping. As the privacy policy goes this seems covered under "user interface
> features, memory, and hardware configuration."

This collection does not jive with the current opt-in string. We need to adjust the opt in string so that it is clear we could collect things like the add-on list.  This opt-in UI adjustment is bug 685373.

Comment 16

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/f2da319b3f6a

Backed out as privacy opt-in needs to be updated for this. Will reland for ff8 once privacy stuff is ready.

Comment 17

[(dormant account)]

And by ff8 I meant ff9

Comment 18

[LegNeato]

Backed out of aurora8 as well:

http://hg.mozilla.org/releases/mozilla-aurora/rev/0ce62710d317

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

We updated the opt-in string in bug 685373 to include "browser customizations" which encompasses add-ons and persona.  If we still find value in collecting this, it could probably go back in.

Comment 20

[(dormant account)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f24bb32f3103

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f24bb32f3103

btw, the checkin comment is missing bug number...

Comment 22

[Daniel C]

Yes, I think this needs to be backed out and re-checked in with the bug number (that seems to be the usual practice).

I'm also not overly comfortable with this change. I'd be very surprised if there is even one other user with the same combination of addons as me and so this is effectively a unique ID for me that spans sessions. The telemetry reporters before this checkin were much less identifiable.

Seeing as there is no way to choose which reporters to opt out of, this may drive people to disable telemetry altogether.

Comment 23

[(dormant account)]

(In reply to Daniel Cater from comment #22)
> Yes, I think this needs to be backed out and re-checked in with the bug
> number (that seems to be the usual practice).

I'll fix it when I get a chance.

> 
> I'm also not overly comfortable with this change. I'd be very surprised if
> there is even one other user with the same combination of addons as me and
> so this is effectively a unique ID for me that spans sessions. The telemetry
> reporters before this checkin were much less identifiable.
> 
> Seeing as there is no way to choose which reporters to opt out of, this may
> drive people to disable telemetry altogether.

We need to be able to correlate crappy performance with addons. We would've not put this in if we didn't believe this was critical to improve Firefox.

The upside is that data is transmitted over https and we do not send data if certificates do not line up.

Comment 24

[Tom Lowenthal]

Perhaps it would be worth trying to address Daniel's legitimate concern by providing an interface where users can opt into or out of specific reporters, perhaps in about:telemetry or so? I don't think this should be a blocker by any means, but it might ber a desirable enhancement.

Comment 25

[(dormant account)]

(In reply to Tom Lowenthal [:StrangeCharm] from comment #24)
> Perhaps it would be worth trying to address Daniel's legitimate concern by
> providing an interface where users can opt into or out of specific
> reporters, perhaps in about:telemetry or so? I don't think this should be a
> blocker by any means, but it might ber a desirable enhancement.

Brilliant idea, added to bug 661881. Now to get someone to do a UI for us :(

Comment 26

[(dormant account)]

Backed out + relanded with bug# on inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/69f7d8cc0c00

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/69f7d8cc0c00