Make 'LOOP_SHARING_STATE_CHANGE' Telemetry event opt-out

RESOLVED FIXED in Firefox 39

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: RT, Assigned: mikedeboer)

Tracking

unspecified
mozilla40
Points:
1
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox38 unaffected, firefox39+ fixed, firefox40+ fixed)

Details

(Whiteboard: [metrics])

User Story

Currently we send telemetry events related to tab/window sharing as follows (per requirements on bug 1135095):
'LOOP_SHARING_STATE_CHANGE' keyed by WINDOW_ENABLED, WINDOW_DISABLED, BROWSER_ENABLED and BROWSER_DISABLED

In Fx38 with bug 1135095 we had to make this opt-in 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.

Attachments

(3 attachments, 1 obsolete attachment)

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

Can you flag someone from Privacy to review this change?
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Attachment #8590205 - Flags: review?(vdjeric)
Iteration: --- → 40.1 - 13 Apr
Points: --- → 1
Flags: qe-verify-
Flags: firefox-backlog+
User Story: (updated)
> 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
Attachment #8590205 - Flags: review?(vdjeric) → review+
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.
Attachment #8590205 - Attachment is obsolete: true
Attachment #8590762 - Flags: review?(vdjeric)
Attachment #8590762 - Flags: review?(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]:
-----------------------------------------------------------------

::: 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": ["firefox-dev@mozilla.org", "mdeboer@mozilla.com"],
>      "expires_in_version": "43",
> +    "kind": "enumerated",
> +    "n_values": 4,

ditto on reserving additional values
Attachment #8590762 - Flags: review?(vdjeric) → review+
Holding off on reviewing this until we hear back from Saptarshi re bug 1135095 comment 26.
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 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.
Attachment #8590762 - Flags: review?(dmose) → review+
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Whiteboard: [metrics]
https://hg.mozilla.org/mozilla-central/rev/1b917f420230
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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.
Attachment #8590762 - Flags: approval-mozilla-aurora?
You shouldn't change the types of existing histograms, Telemetry backend doesn't like it
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.
Attachment #8590762 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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?
Status: RESOLVED → REOPENED
Flags: needinfo?(vdjeric)
Resolution: FIXED → ---
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.
Flags: needinfo?(vdjeric)
Status: REOPENED → ASSIGNED
Attachment #8599227 - Flags: review?(vdjeric) → review+
https://hg.mozilla.org/mozilla-central/rev/83d73d15fcbc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Mike, do you want to request uplift for this new patch?
Flags: needinfo?(mdeboer)
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.
Flags: needinfo?(mdeboer)
Attachment #8602053 - Flags: review+
Attachment #8602053 - Flags: approval-mozilla-aurora?
(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 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.
Attachment #8602053 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.