Closed Bug 1276006 Opened 4 years ago Closed 4 years ago

Add telemetry to count how many users use container tabs

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: ethan, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [userContextId][domsecurity-active][uplift49-])

Attachments

(1 file, 2 obsolete files)

If users ever invoke a container tab, count them as container users.
If they don't, don't count them.
Priority: -- → P2
Component: Security → DOM: Security
Whiteboard: [userContextId]
Whiteboard: [userContextId] → [userContextId][domsecurity-backlog]
Attached patch telemetry.patch (obsolete) — Splinter Review
Mike, are you familiar with telemetry?
What I would like to have from this patch is:

1. Do people use containers?
2. If yes, which one, and how many containers per user.
Assignee: nobody → amarchesini
Attachment #8766484 - Flags: review?(mconley)
Priority: P2 → P1
Whiteboard: [userContextId][domsecurity-backlog] → [userContextId][domsecurity-active]
Status: NEW → ASSIGNED
Comment on attachment 8766484 [details] [diff] [review]
telemetry.patch

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

r-'ing because I think we're double-counting in a common case, but otherwise this looks okay (just waiting to hear back in case there's a good reason for counting on browser element creation).

You'll also need a data review from a data privacy peer - my goto reviewer is usually bsmedberg. Once he signs off, you'll want to add data-review=bsmedberg to the commit message.

::: browser/base/content/tabbrowser.xml
@@ +1780,5 @@
>              b.setAttribute("tooltip", this.getAttribute("contenttooltip"));
>  
>              if (userContextId) {
>                b.setAttribute("usercontextid", userContextId);
> +              ContextualIdentityService.telemetry(userContextId);

Are we sure we need this one?

I think this is going to cause us to double-count usage if the user adds a tab which ends up creating a browser. We might only need the addTab one, unless you've got a reason for adding this one here?
Attachment #8766484 - Flags: review?(mconley) → review-
In addition to the number of tabs open in each context by the aggregate user base, will this probe also tell us what percentage of users are using containers?
Attached patch telemetry.patch (obsolete) — Splinter Review
Attachment #8766484 - Attachment is obsolete: true
Attachment #8767703 - Flags: review?(mconley)
Attachment #8767703 - Flags: review?(benjamin)
Comment on attachment 8767703 [details] [diff] [review]
telemetry.patch

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

Looks like this'll work as advertised.
Attachment #8767703 - Flags: review?(mconley) → review+
Comment on attachment 8767703 [details] [diff] [review]
telemetry.patch

Why is this a keyed histogram? In general keys are expensive and should be avoided unless they are necessary. If a key is necessary, the histogram description should be very specific about what key values may contain.

Also, "Tracking the use of Containers" is pretty vague. Please indicate exactly what you are counting, such as "Counter is incremented every time a user launches a tab in a container" or "Counter is incremented the first time e user uses each container in each session". or maybe "any container". In any case, it should be possible to tell just be reading the description what is recorded, without having to look through the implementation.
Attachment #8767703 - Flags: review?(benjamin) → review-
Attached patch telemetry.patchSplinter Review
Attachment #8767703 - Attachment is obsolete: true
Attachment #8768541 - Flags: review?(benjamin)
Comment on attachment 8768541 [details] [diff] [review]
telemetry.patch

I'm still not convinced about "never", but I'll periodically ping you about this since it's clear that this will be valuable over more than 6 months. data-review=me (I did not review the code, just the histogram definitions).
Attachment #8768541 - Flags: review?(benjamin) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a36f96cf7d6
Add telemetry to count how many users use container tabs, r=mconley, data-review=bsmedberg
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21dd2e281e4c
Add telemetry to count how many users use container tabs - part 2, r=me
https://hg.mozilla.org/mozilla-central/rev/3a36f96cf7d6
https://hg.mozilla.org/mozilla-central/rev/21dd2e281e4c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Whiteboard: [userContextId][domsecurity-active] → [userContextId][domsecurity-active][uplift49-]
You need to log in before you can comment on or make changes to this bug.