1127574 - Rooms should have Telemetry feedback for generate/copy/email data

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

When we wrote rooms, we didn't include the telemetry data that Calls had.

For calls we had:

- LOOP_CLIENT_CALL_URL_REQUESTS_SUCCESS
- LOOP_CLIENT_CALL_URL_SHARED (emailed or copied)

What do we want for ROOMS? I'm currently thinking

- LOOP_ROOMS_CREATED
- LOOP_ROOMS_URL_SHARED

Created is obviously an increment, not the total per user - though we could potentially add that as well.

Thoughts?

Comment 1

[Romain Testard [:RT]]

We already have "created rooms" from the server data although I am not sure if we can correlate server data with telemetry data well so perhaps the created rooms data help this correlation?

Also is this the kind of data that could be added to FHR for simpler correlation with the other server data?

I can think of these other types of events we'll want to track - does this make sense to address this all under a single bug?
- Tab sharing initiated/stopped
- Window sharing initiated/stopped
- Face mute/unmute
- Audio mute/unmute
- Copy URL from panel
- Copy URL from conversation window
- Share URL from conversation window
- Rename room
- Delete room
- Change availability status
- Import contacts

Comment 2

[Adam Roach [:abr]]

(In reply to Romain Testard [:RT] from comment #1)
> We already have "created rooms" from the server data although I am not sure
> if we can correlate server data with telemetry data well so perhaps the
> created rooms data help this correlation?


The issue that I see here is that server information will be complete, while telemetry data will be sampled. In particular, this means that figuring out how many times each link is shared, on average, will be impossible.

> Also is this the kind of data that could be added to FHR for simpler
> correlation with the other server data?

I'm hesitant to use FHR for most of these things, simply for the privacy implications. I think we need to carefully consider what questions we want answered, and whether we can answer them with something less intrusive than an opt-out solution. I suspect we can.

> I can think of these other types of events we'll want to track - does this
> make sense to address this all under a single bug?
> - Tab sharing initiated/stopped
> - Window sharing initiated/stopped
> - Face mute/unmute
> - Audio mute/unmute
> - Copy URL from panel
> - Copy URL from conversation window
> - Share URL from conversation window
> - Rename room
> - Delete room
> - Change availability status
> - Import contacts

I think these, along with the "create room", are reasonable candidates for telemetry, although I don't think they're the subject of *this* bug as it was envisioned.

For this bug, I think we want counts for "create room", "delete room", "copy from panel", "email from panel," "copy from conversation window", "email from conversation window", and "email from error panel after a direct call failed".

I would suggest putting the other proposed metrics into a different bug or bugs.

Comment 3

[Romain Testard [:RT]]

OK fair points.
I created bug 1129726 to deal with other room usage data.

Comment 4

[Romain Testard [:RT]]

Shell, I am marking this as blocking Fx38 so we can start tracking this work as part of the Hello bugs.

Comment 5

[:shell escalante]

Based on prioritized work - this bug will likely not get done in next 2 iterations... dropping rank/priority

Comment 6

[:shell escalante]

discussed with Mark and RT about raising these to uplift them with other telemetry.  smaller changes and getting us info starting in 39 for user behavior that we'll use to make decisions.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: add telemetry hooks copy and email of room and direct call URLs

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: add telemetry hooks copy and email of room and direct call URLs

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: add telemetry hooks for Hello rooms creation and deletion actions

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8612264 [details] [diff] [review]
Patch 1.1: add telemetry hooks copy and email of room and direct call URLs

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

Looks good, r=Standard8, as I mention at the bottom, this needs opt-out approval/review from one of the telemetry peers.

::: browser/components/loop/modules/MozLoopService.jsm
@@ +43,5 @@
>  };
>  
> +/**
> + * Values that we segment sharing a room URL action telemetry probes into.
> + * 

nit: Eslint says trailing space

::: browser/components/loop/test/desktop-local/conversationViews_test.js
@@ +89,5 @@
>        stopAlerting: sinon.stub(),
>        userProfile: {
>          email: "[e-mail]"
> +      },
> +      telemetryAddValue: function() {}

Adding this implies incorrect stubbing and the function isn't called directly from conversationViews.jsx - the stub of "composeCallUrlEmail" in "should compose an email once the email link is received" needs to be moved up to beforeEach, so that "should close the conversation window once the email link is received" has it stubbed as well.

Hence, please move the stub and remove this addition.

::: browser/components/loop/test/desktop-local/roomStore_test.js
@@ +93,5 @@
>            open: function() {},
>            rename: function() {},
>            on: sandbox.stub()
> +        },
> +        telemetryAddValue: sandbox.stub()

nit: I generally prefer to use sinon.stub() for things like this where we're recreating the object each time, as we don't need the sandbox functionality to restore the stub before/after.

::: toolkit/components/telemetry/Histograms.json
@@ +7448,5 @@
>      "n_values": 8,
>      "releaseChannelCollection": "opt-out",
>      "description": "Number of times the sharing feature has been enabled and disabled (0=WINDOW_ENABLED, 1=WINDOW_DISABLED, 2=BROWSER_ENABLED, 3=BROWSER_DISABLED)"
>    },
> +  "LOOP_SHARING_ROOM_URL": {

This needs opt-out review from one of the Telemetry peers.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8612264 [details] [diff] [review]
Patch 1.1: add telemetry hooks copy and email of room and direct call URLs

Vladan, can you review the new histograms I added and their opt-out state?

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8612265 [details] [diff] [review]
Patch 2: add telemetry hooks for Hello rooms creation and deletion actions

...and the same for the one I added here?

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8612265 [details] [diff] [review]
Patch 2: add telemetry hooks for Hello rooms creation and deletion actions

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

Looks good r+ subject to the opt-out review.

Comment 14

[Vladan Djeric (:vladan)]

Comment on attachment 8612264 [details] [diff] [review]
Patch 1.1: add telemetry hooks copy and email of room and direct call URLs

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

Which question is this seeking to answer? Are you just interested in 1) how commonly URLs are shared, and 2) how often each of the sharing methods is used?

::: toolkit/components/telemetry/Histograms.json
@@ +7450,5 @@
>      "description": "Number of times the sharing feature has been enabled and disabled (0=WINDOW_ENABLED, 1=WINDOW_DISABLED, 2=BROWSER_ENABLED, 3=BROWSER_DISABLED)"
>    },
> +  "LOOP_SHARING_ROOM_URL": {
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "expires_in_version": "50",

do we really need a year's worth of metrics on this?

Comment 15

[Vladan Djeric (:vladan)]

Comment on attachment 8612265 [details] [diff] [review]
Patch 2: add telemetry hooks for Hello rooms creation and deletion actions

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

::: toolkit/components/telemetry/Histograms.json
@@ +7462,5 @@
> +    "expires_in_version": "50",
> +    "kind": "enumerated",
> +    "n_values": 8,
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Number of times a room action is performed (0=CREATE_SUCCESS, 1=CREATE_FAIL, 2=DELETE_SUCCESS, 3=DELETE_FAIL)"

do you really want room creation & room deletion in the same histogram? are you interested in comparing, e.g. how common it is fail to create a room vs fail to delete a room?

i'm guessing you'll be looking at these histograms' data in the histogram dash, right?

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #14)
> do we really need a year's worth of metrics on this?

I guess not really, I just copy & pasted that number TBH. I can lower it down to ~6mo.

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #15)
> do you really want room creation & room deletion in the same histogram? are
> you interested in comparing, e.g. how common it is fail to create a room vs
> fail to delete a room?

No, I don't think we'll be correlating the two actions like that. I simply grouped them together semantically, so that our PM can look at one graph for all room actions.

> i'm guessing you'll be looking at these histograms' data in the histogram
> dash, right?

Yes, that's the idea. Well, I won't be looking at the data probably, but RT/ Chad et al. will.

Do you have a suggestion perhaps that would make for a better histogram structure?

Comment 18

[Vladan Djeric (:vladan)]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Do you have a suggestion perhaps that would make for a better histogram
> structure?

- I would store measurements that are not directly related in different histograms, e.g. one histogram for room creation success vs failure, another histogram for room deletion vs failure. This also makes it easier to write automated monitoring tools, so you don't get meaningless alerts for example if the ratio of total # of creation failures vs total # of deletion failure changes in the histogram
- I was asking if you guys will be using the histogram dash to determine how commonly URLs are shared because we're in the process of designing new Telemetry dashboards right now and we're trying to figure out if need to be able to get total event counts from dashboards, e.g. to determine that 150,000 room links were shared in Firefox 39

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #18)
> (In reply to Mike de Boer [:mikedeboer] from comment #17)
> > Do you have a suggestion perhaps that would make for a better histogram
> > structure?
> 
> - I would store measurements that are not directly related in different
> histograms, e.g. one histogram for room creation success vs failure, another
> histogram for room deletion vs failure. This also makes it easier to write
> automated monitoring tools, so you don't get meaningless alerts for example
> if the ratio of total # of creation failures vs total # of deletion failure
> changes in the histogram

Alright, that's a good suggestion, thanks! :)

> - I was asking if you guys will be using the histogram dash to determine how
> commonly URLs are shared because we're in the process of designing new
> Telemetry dashboards right now and we're trying to figure out if need to be
> able to get total event counts from dashboards, e.g. to determine that
> 150,000 room links were shared in Firefox 39

I think :RT would be quite interested to see that information in a glance on a dashboard! It might be _very_ useful to contact him to talk through a couple of ideas to match theory with practice.

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: add telemetry hooks copy and email of room and direct call URLs

Carrying over r=Standard8.

Comment 21

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: add telemetry hooks for Hello rooms creation and deletion actions

Carrying over r=Standard8.

Comment 22

[Vladan Djeric (:vladan)]

Comment on attachment 8615250 [details] [diff] [review]
Patch 2.1: add telemetry hooks for Hello rooms creation and deletion actions

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

Postponing review until we figure out approach over email

Comment 23

[Vladan Djeric (:vladan)]

Comment on attachment 8615250 [details] [diff] [review]
Patch 2.1: add telemetry hooks for Hello rooms creation and deletion actions

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

Postponing review until we figure out approach over email #2

Comment 24

[Vladan Djeric (:vladan)]

Comment on attachment 8615249 [details] [diff] [review]
Patch 1.2: add telemetry hooks copy and email of room and direct call URLs

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

Postponing review until we figure out approach over email #2 :)

Comment 25

[Vladan Djeric (:vladan)]

I'll take a look tomorrow, I'm on PTO today

Comment 26

[Vladan Djeric (:vladan)]

Comment on attachment 8615249 [details] [diff] [review]
Patch 1.2: add telemetry hooks copy and email of room and direct call URLs

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

r+ for the histogram

Comment 27

[Vladan Djeric (:vladan)]

Comment on attachment 8615250 [details] [diff] [review]
Patch 2.1: add telemetry hooks for Hello rooms creation and deletion actions

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

r+ for the histogram

as previously discussed, you'll have to do custom analyses (vs looking at dashes) to interpret these

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2a48b9407b97
https://hg.mozilla.org/integration/fx-team/rev/6d0e5f55fd21

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/2a48b9407b97
https://hg.mozilla.org/mozilla-central/rev/6d0e5f55fd21

Comment 30

[Mike de Boer [:mikedeboer]]

Comment on attachment 8615249 [details] [diff] [review]
Patch 1.2: add telemetry hooks copy and email of room and direct call URLs

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello, no bug.
[User impact if declined]: Hello rooms have been introduced a long time ago, but we weren't measuring their usage from the client. The loop-server does cover some of the metrics, but not as complete as implemented here.
For our product owner it's important to _know_ during this phase of the product.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 31

[Mike de Boer [:mikedeboer]]

Comment on attachment 8615250 [details] [diff] [review]
Patch 2.1: add telemetry hooks for Hello rooms creation and deletion actions

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello, no bug.
[User impact if declined]: Hello rooms have been introduced a long time ago, but we weren't measuring their usage from the client. The loop-server does cover some of the metrics, but not as complete as implemented here.
For our product owner it's important to _know_ during this phase of the product.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 32

[Kate Glazko]

Comment on attachment 8615249 [details] [diff] [review]
Patch 1.2: add telemetry hooks copy and email of room and direct call URLs

Approving for Aurora uplift because minor risks, has been on m-c for three days with no known issues.

Comment 33

[Kate Glazko]

Comment on attachment 8615250 [details] [diff] [review]
Patch 2.1: add telemetry hooks for Hello rooms creation and deletion actions

Approving for Aurora uplift because minor risks, has been on m-c with no known issues.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf8f2ea74db0
https://hg.mozilla.org/releases/mozilla-aurora/rev/58fd619fde28