1245486 - Send Telemetry events for use of the "Pause" and "Restart" buttons

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Manuel Casas Barrado [:mancas]]

Add telemetry events when user clicks on "Pause"/"Restart" buttons

Comment 1

[GitHub Autolander Bot]

Attachment: [loop] mancas:bug1245486 > mozilla:master

Comment 2

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717066 [details] [review]
[loop] mancas:bug1245486 > mozilla:master

Mike, the code is working as expected but I'm getting an exception since the histogram does not exist. Could you tell me where I need to create the new one to be recognized?

Comment 3

[Manuel Casas Barrado [:mancas]]

Attachment: MC_Patch

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8717405 [details] [diff] [review]
MC_Patch

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

This change requires review from a data collection peer.

::: toolkit/components/telemetry/Histograms.json
@@ +8387,5 @@
>      "releaseChannelCollection": "opt-out",
>      "description": "Number of sessions where at least one chat message was exchanged"
>    },
> +  "LOOP_INFOBAR_ACTION_BUTTONS": {
> +    "alert_emails": ["[e-mail]", "[e-mail]"],

You might want to mbanner@moz there, instead of me.

Comment 5

[Benjamin Smedberg]

Comment on attachment 8717405 [details] [diff] [review]
MC_Patch

I'm a bit confused, because the bug is talking about two buttons (pause/restart), but this histogram is counting just one thing. Is that intended? The description here is very vague and needs to be clarified: are the pause/restart buttons the only buttons on the infobar, or are there other buttons and are they also counted?

Mike, will you be monitoring this value using the telemetry dashboards? As far as I know, those dashboards currently only show people who have opted into to extended telemetry (even on the release channel). If you need to look at the entire population, you will need to write your own queries.

In order for this to be opt-out, you need to have a statement of how this metric will provide user value. I suspect that you'll be using this to optimize other product decisions? If so can you provide some details about what those decisions are and how this data will influence it?

Comment 6

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717066 [details] [review]
[loop] mancas:bug1245486 > mozilla:master

Comments addressed. Take a look when you get a chance

Thank you

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> I'm a bit confused, because the bug is talking about two buttons
> (pause/restart), but this histogram is counting just one thing. Is that
> intended? The description here is very vague and needs to be clarified: are
> the pause/restart buttons the only buttons on the infobar, or are there
> other buttons and are they also counted?

There is a 'Disconnect' button, which is not counted. The Pause/ Restart buttons are one and the same button; a two-state toggle.

> Mike, will you be monitoring this value using the telemetry dashboards? As
> far as I know, those dashboards currently only show people who have opted
> into to extended telemetry (even on the release channel). If you need to
> look at the entire population, you will need to write your own queries.

I won't be monitoring this value, our PM will (:RT) and Ian (:ibicking). I *think* they're using the dashboards.

> In order for this to be opt-out, you need to have a statement of how this
> metric will provide user value. I suspect that you'll be using this to
> optimize other product decisions? If so can you provide some details about
> what those decisions are and how this data will influence it?

Of course. I think :RT simply omitted that from the user story - let's ask!

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8717066 [details] [review]
[loop] mancas:bug1245486 > mozilla:master

Just a few more things to address!

Comment 9

[Romain Testard [:RT]]

User value details now added to the user story field.
Regarding the way we'll be monitoring the values, we used to be able to review on https://telemetry.mozilla.org/ although the data does not seem to appear there anymore for opt-out data. I assume that writing our own requests assumes the use of a Spark instance, I created bug 1247268 to follow that up.

Comment 10

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717066 [details] [review]
[loop] mancas:bug1245486 > mozilla:master

I've addressed your comments. Please take a look at the PR

Thanks!

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8717066 [details] [review]
[loop] mancas:bug1245486 > mozilla:master

Thanks, looking good to me!

Comment 12

[Benjamin Smedberg]

How will this one counter here be able to satisfy the requirement "Share of Hello sessions where 'Restart sharing' gets used". If it's a simple counter, it seems that it would be hard to distinguish the case of multiple hello sessions, hit pause once in each, versus a single session where pause/restart was chosen.

Comment 13

[Romain Testard [:RT]]

Could we send just one instance of each event per Hello session when applicable ?

Comment 14

[Benjamin Smedberg]

That's fine with me; I don't know how much engineering work that is.

Comment 15

[Romain Testard [:RT]]

Manu, can you please have a look and let me know if we could send just one instance of each event per Hello session when applicable ?

Comment 16

[Ian Bicking (:ianbicking)]

Can we send this as two events, one "pause" and one "restart"?  Then 1 pause means they turned video off, 1 pause and 1 restart means they paused for a little while, etc.  Theoretically we can go by even/odd but that seems like it might be hard to analyze in Telemetry.

I'm not sure if we got the user value statement in: tracking these clicks allows us to analyze if the tab sharing feature of Hello is useful (e.g., if people turn it off and do not turn it back on, then it is not useful to that population).  Also we can see if the pause feature is used; if not we may redesign it or remove it.  If we make changes to the UI in the future this will provide a baseline for comparison.

Comment 17

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8717066 [details] [review]
[loop] mancas:bug1245486 > mozilla:master

Mike I've addressed comments from Ian and Romain. Now the events are send one, of each type, per session

Would you mind taking a look at the patch again?

Thanks!

Comment 18

[Ian Bicking (:ianbicking)]

To make sure the add-on doesn't raise an exception when installed on a Firefox without the Histograms.json (as will happen during QA), we should guard the Telemetry with a try/catch.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8717066 [details] [review]
[loop] mancas:bug1245486 > mozilla:master

Your filtering solution is nice & simple. Good work, thanks!

Comment 20

[Manuel Casas Barrado [:mancas]]

Landed in master: https://github.com/mozilla/loop/commit/488153c83028340b219766c46a72cc6dd41b51e6

We still need to land mc_patch in mozilla-central

Comment 21

[Benjamin Smedberg]

Comment on attachment 8717405 [details] [diff] [review]
MC_Patch

This patch doesn't seem to match the Hello change, which uses an enumerated histogram. Wrong patch perhaps?

Comment 22

[Manuel Casas Barrado [:mancas]]

Attachment: MC_Patch

Comment 23

[Benjamin Smedberg]

Comment on attachment 8720681 [details] [diff] [review]
MC_Patch

Does this build? I'm pretty sure you need an n_values: 2 for this to build properly, and I'm certain it won't show up on telemetry.mozilla.org properly without that.

data-review=me with that change

Comment 24

[Manuel Casas Barrado [:mancas]]

Attachment: 0001-Bug-1245486-Send-Telemetry-events-for-use-of-the-Pau.patch

Changes addressed so keeping f+

Comment 25

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/ac5ed9e7b4173585001b5691ea941d2afb95da4f
Bug 1245486 - Send Telemetry events for use of the "Pause" and "Restart" buttons. f=bsmedberg, r=mikedeboer

Comment 26

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/3cfeae80438b

Comment 27

[Manuel Casas Barrado [:mancas]]

Attachment: 0001-Bug-1245486-Send-Telemetry-events-for-use-of-the-Pau.patch

It compiles locally, please check if it works for you and land this and bug 1208416

Thank you!

Comment 28

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/970aa793c5e2778172cd8e527f9dc04848400d40
Bug 1245486 - Send Telemetry events for use of the Pause and Restart buttons. r=mikedeboer

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/970aa793c5e2

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8724643 [details] [diff] [review]
0001-Bug-1245486-Send-Telemetry-events-for-use-of-the-Pau.patch

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello, telemetry for use of pause & restart buttons
[User impact if declined]: We won't be able to measure the use of the pause/restart buttons on tab sharing until 47 - taking longer to get the results out.
[Describe test coverage new/current, TreeHerder]: Has been in m-c & aurora for quite a while.
[Risks and why]: Low, small change to json file.
[String/UUID change made/needed]: None

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724643 [details] [diff] [review]
0001-Bug-1245486-Send-Telemetry-events-for-use-of-the-Pau.patch

extra patch for Hello telemetry. Please uplift to beta.

Comment 32

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ca1243f29c3b4d7c9611d4dc040fbb65ddbf6c90