1152733 - Make 'LOOP_SHARING_STATE_CHANGE' Telemetry event opt-out

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect)

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: make the LOOP_SHARING_STATE_CHANGE histogram opt-out

Vladan, is this all that's needed to make it opt-out?

Can you flag someone from Privacy to review this change?

Comment 2

[Vladan Djeric (:vladan)]

> Vladan, is this all that's needed to make it opt-out?

Yes, this will make the histogram opt-out. Note that only Firefox versions 39 and higher support opt-out histograms

> Can you flag someone from Privacy to review this change?

- I can review the change, I'm one of the Data Collection peers https://wiki.mozilla.org/Firefox/Data_Collection
- And I already reviewed making this histogram opt-out in bug 1135095 three weeks ago

See my latest comment in bug 1135095 about declaring the histogram differently

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: convert all Loop keyed histograms to be enumerated types and opt-out

Due to Vladan's 'nudge nudge, wink wink' toward using enumerated histograms instead of keyed counters, I felt the need to review our telemetry code.

Turns out we don't need keyed histograms at all. What I did here is move both telemetry probes over to be enumerated, so that we can take advantage of the flat data format and more solid reporting feature in the dashboard for RT and others.

/ht vladan.

Comment 4

[Vladan Djeric (:vladan)]

Comment on attachment 8590762 [details] [diff] [review]
Patch v2: convert all Loop keyed histograms to be enumerated types and opt-out

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

::: toolkit/components/telemetry/Histograms.json
@@ +7483,5 @@
>    },
>    "LOOP_TWO_WAY_MEDIA_CONN_LENGTH": {
>      "expires_in_version": "43",
> +    "kind": "enumerated",
> +    "n_values": 4,

allocate more than 4 values, otherwise you'll have to declare a new histogram if you decide you need additional buckets in the future

@@ +7488,2 @@
>      "releaseChannelCollection": "opt-out",
> +    "description": "Connection length for bi-directionally connected media (0=SHORTER_THAN_10S, 1=BETWEEN_10S_AND_30S, 2=BETWEEN_30S_AND_5M, 3=MORE_THAN_5M)"

don't forget to add the alert-emails field

@@ +7491,5 @@
>    "LOOP_SHARING_STATE_CHANGE": {
>      "alert_emails": ["[e-mail]", "[e-mail]"],
>      "expires_in_version": "43",
> +    "kind": "enumerated",
> +    "n_values": 4,

ditto on reserving additional values

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Holding off on reviewing this until we hear back from Saptarshi re bug 1135095 comment 26.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

I spent some time chatting with Saptarshi and Vladan about this today, and I'm still a bit on the fence.  Mike, can you and I (and RT, if he wants) chat about this on Vidyo after tomorrow morning's meeting?  I think that should be enough to get sufficient clarity here.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8590762 [details] [diff] [review]
Patch v2: convert all Loop keyed histograms to be enumerated types and opt-out

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

r=dmose with vladan's comments addressed.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1b917f420230

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1b917f420230

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590762 [details] [diff] [review]
Patch v2: convert all Loop keyed histograms to be enumerated types and opt-out

Approval Request Comment
[Feature/regressing bug #]: bug 1135095
[User impact if declined]: we had to make Loop telemetry probes opt-in on Fx38 although this prevents us from comparing the Telemetry data collected with our server side traffic. The request is to make 'LOOP_SHARING_STATE_CHANGE' opt-out to allow us measuring the use of the feature when compared to overall usage.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 11

[Vladan Djeric (:vladan)]

You shouldn't change the types of existing histograms, Telemetry backend doesn't like it

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8590762 [details] [diff] [review]
Patch v2: convert all Loop keyed histograms to be enumerated types and opt-out

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #11)
> You shouldn't change the types of existing histograms, Telemetry backend
> doesn't like it

From speaking with Vladan I understand that it is more than "shouldn't" and rather that we need a new patch that defines a new histogram. I'm denying uplift. Based on what I wrote above, I suggest that the patch be backed out in favour or relanding with a new histogram definition.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Re-reading the last paragraph in comment 12 more closely, it sounds like we need to do something before 40 uplifts.  Since the patch hasn't yet been backed out, and presumably Nightly 40 has now for a little while been generated differently-typed data under the same name it's not obvious to me exactly what the right thing to do here is.  Vladan, will simply landing another patch that renames it now be sufficient?

[Tracking Requested - why for this release]:

My understanding from reading comment 12 is that something bad will happen if we don't make some change before 40 ships, but it's not entirely clear what that failure mode is.  Vladan, can you fill in the details on that as well?

Comment 14

[Vladan Djeric (:vladan)]

An explanation by Mark Reid:

> "I believe the problem with modifying histogram definitions is due to the aggregation code - it could have trouble aggregating for a channel+version by submission date if the bucket count or type changed.  Since the aggregation code does not handle keyed histograms (which is what the histogram was before, right?), it should be effectively the same 

So yes, you can just land a patch now that renames it. It's probably not even strictly necessary with the current backend implementation, but we should still do it regardless.
With respect to uplifts, it doesn't matter which histogram name you uplift, since Aurora hasn't seen this histogram yet. The new name is better, just for consistency.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Ad. patch: rename histograms

Comment 16

[Mike de Boer [:mikedeboer]]

Thanks!

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/83d73d15fcbc

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83d73d15fcbc

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Mike, do you want to request uplift for this new patch?

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Branch patch: folded patch for m-a

Carrying over r=vladan,dmose.

Approval Request Comment
[Feature/regressing bug #]: bug 1135095
[User impact if declined]: we had to make Loop telemetry probes opt-in on Fx38 although this prevents us from comparing the Telemetry data collected with our server side traffic. The request is to make 'LOOP_SHARING_STATE_CHANGE' opt-out to allow us measuring the use of the feature when compared to overall usage.
This patch also renames the respective histograms by suffixing their names with '_1' as to not cause trouble for the telemetry system when this lands on Aurora.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 20

[Mike de Boer [:mikedeboer]]

(In reply to Liz Henry (:lizzard) from comment #18)
> Mike, do you want to request uplift for this new patch?

Liz, thanks for the ping!!

Comment 21

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8602053 [details] [diff] [review]
Branch patch: folded patch for m-a

Approved for uplift to aurora since the base for this change has had several days on m-c.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e84ae588f726