Closed Bug 668392 Opened 8 years ago Closed 8 years ago

Include enabled addons + persona in telemetry

Categories

(Toolkit :: Telemetry, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
firefox8 --- unaffected
firefox9 --- unaffected

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

(Whiteboard: [inbound])

Attachments

(1 file, 2 obsolete files)

Attached patch addons + personas (obsolete) — Splinter Review
No description provided.
Attachment #543007 - Flags: review?(dtownsend)
Attachment #543007 - Attachment is patch: true
ouch, you really want to fail silently in XPIProvider?
(In reply to comment #1)
> ouch, you really want to fail silently in XPIProvider?

I don't see why not
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 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.
Attachment #543007 - Flags: review?(dtownsend) → review-
(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.
(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.
Attached patch patch with nonworking test (obsolete) — Splinter Review
> 
> 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.
Attachment #543007 - Attachment is obsolete: true
Attachment #548638 - Flags: feedback?(dtownsend)
(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.
Thanks, that little detail solved it.
Attachment #548638 - Attachment is obsolete: true
Attachment #551159 - Flags: review?(dtownsend)
Attachment #548638 - Flags: feedback?(dtownsend)
Attachment #551159 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/fdffde7c3b14
Assignee: nobody → tglek
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
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.
Keywords: privacy
(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.
I'll buy that. Sid will still want a privacy-team review.
(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.
Depends on: 685373
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
And by ff8 I meant ff9
Backed out of aurora8 as well:

http://hg.mozilla.org/releases/mozilla-aurora/rev/0ce62710d317
Target Milestone: mozilla8 → mozilla10
Depends on: 685880
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f24bb32f3103

btw, the checkin comment is missing bug number...
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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.
(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.
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.
(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 :(
Backed out + relanded with bug# on inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/69f7d8cc0c00
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/69f7d8cc0c00
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Flags: sec-review+
You need to log in before you can comment on or make changes to this bug.