Closed Bug 945499 Opened 6 years ago Closed 6 years ago

Switch BrowserUITelemetry from using UITelemetry's event logging to just counting events

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox27 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Keywords: privacy, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Conversations with geekboy in IRC and bug 945347, as well as vladan's concerns in bug 944115 and bug 944116 make me think it'd be better if we forgo UITelemetry's built-in event logging for something a little simpler: just counting events, and reporting the counts.

I think that approach is more privacy-centric / privacy-friendly, and will also reduce the potential packet-size for long-running sessions.
I'm fine with that, with the exception of the two cases we talked about earlier.
(Amount of time spent in customize mode, and clicking on the bookmarks menu after clicking on the the bookmarks button, iirc.)
Attached patch Patch v1 (obsolete) — Splinter Review
Attached patch Patch v1.1Splinter Review
Attachment #8341875 - Attachment is obsolete: true
Attachment #8341880 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8341880 [details] [diff] [review]
Patch v1.1

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

LGTM!

::: browser/modules/BrowserUITelemetry.jsm
@@ +213,5 @@
> +  _countableEvents: {},
> +  _countEvent: function(aCategory, aAction) {
> +    if (!(aCategory in this._countableEvents)) {
> +      this._countableEvents[aCategory] = {};
> +    }

At this point I would stuff the object in an intermediate, ie:

let categoryEvents = this._countableEvents[aCategory];

Just to make the rest of the fn easier to read. But that's just me, can just land as is if you're not bothered.
Attachment #8341880 - Flags: review?(gijskruitbosch+bugs) → review+
Landed on Holly as https://hg.mozilla.org/projects/holly/rev/a7d753c6f061

We'll be doing the same thing for the probes that land on mozilla-central, but I'll do the forward-port in one shot, rather than piecemeal.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
Comment on attachment 8341880 [details] [diff] [review]
Patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

This is to land at the same time as bug 942244. This fixes an issue brought up by the Privacy team where BrowserUITelemetry was using UITelemetry's internal sequential event logging mechanism which was deemed too privacy invasive. This patch switches from using that mechanism to simply counting events.


User impact if declined: 

If bug 942244 lands and this one does not, a sequential log of (whitelisted) user activities in the Firefox Appmenu and during customization will be sent in Telemetry packets, which is not great for privacy.


Testing completed (on m-c, etc.): 

Lots of manual testing on Holly, which has since merged to Aurora.


Risk to taking this patch (and alternatives if risky): 

None.


String or IDL/UUID changes made by this patch:

None.
Attachment #8341880 - Flags: approval-mozilla-beta?
Removing [Australis:P1] since these block a P1 already. Let's not be redundant and noisy.
Whiteboard: [Australis:P1]
Attachment #8341880 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.