1276006 - Add telemetry to count how many users use container tabs

Status: RESOLVED FIXED

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

Description

[Ethan Tseng [:ethan]]

If users ever invoke a container tab, count them as container users.
If they don't, don't count them.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: telemetry.patch

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 3

[Tanvi Vyas[:tanvi]]

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?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: telemetry.patch

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8767703 [details] [diff] [review]
telemetry.patch

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

Looks like this'll work as advertised.

Comment 6

[Benjamin Smedberg]

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.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: telemetry.patch

Comment 8

[Benjamin Smedberg]

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).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a36f96cf7d6
Add telemetry to count how many users use container tabs, r=mconley, data-review=bsmedberg

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21dd2e281e4c
Add telemetry to count how many users use container tabs - part 2, r=me

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3a36f96cf7d6
https://hg.mozilla.org/mozilla-central/rev/21dd2e281e4c