1135095 - Telemetry event should be sent when enabling/disabling Tab sharing or window sharing

Status: RESOLVED FIXED

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

Comment 1

[Romain Testard [:RT]]

Shell, can we please get this on Trello as discussed in the stand-ups?

Comment 2

[Romain Testard [:RT]]

For info bug 1127574 deals with other similar Telemetry eventsthat are considered for Fx38 delivery.

Comment 3

[:shell escalante]

I'll set up triage next week for the backlog - but this wouldn't bump anything this release likely (only 3 weeks left).  There's a few metrics bugs we can triage for importance.  Took the FHR metrics bug for time spent in Firefox for this release (ended up being a bit trickier).

Comment 4

[Romain Testard [:RT]]

Having this is very desirable to understand the impact of the key feature we bring in Fx38.
I see this bug as the highest priority metrics bug at the moment - this is higher priority than bug 1129726

Comment 5

[Mike de Boer [:mikedeboer]]

Shell, since we bumped the prio here, can you adjust the rank field for me?

Comment 6

[:shell escalante]

Hi RT - I'm adding Gavin (will add you to his email with the team and their focuses for this iteration). 

Even being the highest priority Metric bug - are these bugs we would not ship Fx39 without? Is it a P1 based on how we're measuring things https://wiki.mozilla.org/Firefox/Iterative_Hello_Development#Product_Backlog?

How does metrics rank against shipping Tab Sharing or Context or starting the Link Clicker parity breakdown?

That being said - it's Mike's call if it's simplest to add now while he's touching Tab sharing code... but I wouldn't bump the Tab sharing feature bugs he is working on and delay Tab or Windows Sharing landing so it can be tested in Beta before release.

Comment 7

[:shell escalante]

Per Gavin's request - just updating bugs that carried over before they were 100% resolved to the next iteration.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add telemetry for window and tab sharing triggers

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8578608 [details] [diff] [review]
Patch v1: add telemetry for window and tab sharing triggers

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

Looks good. r=Standard8 with the comments addressed, also note that we need extra review from the data collection folks.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +188,5 @@
>        this.session.unpublish(this.screenshare);
>        this.screenshare.off("accessAllowed accessDenied");
>        this.screenshare.destroy();
>        delete this.screenshare;
> +      this._noteSharingState(this._windowId ? "browser" : "window", false);

Using this.screenshare.getStyle().videoSource might be nicer if it works - then we're definitely keying off the same value and not a by-product.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +216,5 @@
>        driver.startScreenShare(options);
>  
>        sinon.assert.calledOnce(sdk.initPublisher);
>        sinon.assert.calledWithMatch(sdk.initPublisher, fakeElement, options);
> +      sinon.assert.calledWithExactly(driver._noteSharingState, "browser", true);

Please split out to a separate it - e.g. "should log a telemetry action"

@@ +269,4 @@
>        driver.endScreenShare(new sharedActions.EndScreenShare());
>  
>        sinon.assert.calledOnce(session.unpublish);
> +      sinon.assert.calledWithExactly(driver._noteSharingState, "window", false);

Please split out to a separate it - e.g. "should log a telemetry action"

@@ +294,5 @@
> +
> +      driver.endScreenShare(new sharedActions.EndScreenShare());
> +
> +      sinon.assert.calledOnce(session.unpublish);
> +      sinon.assert.calledWithExactly(driver._noteSharingState, "browser", false);

Please split out to a separate it - e.g. "should log a telemetry action"

@@ +446,5 @@
>    });
>  
> +  describe("#_noteSharingState", function() {
> +    it("should record enabled sharing states for window", function() {
> +      console.log(driver._noteSharingState);

Debug left in?

@@ +448,5 @@
> +  describe("#_noteSharingState", function() {
> +    it("should record enabled sharing states for window", function() {
> +      console.log(driver._noteSharingState);
> +      driver._noteSharingState("window", true);
> +      sinon.assert.calledOnce(mozLoop.telemetryAddKeyedValue);

nit: I find it slightly nicer to separate the "action" and "expected results" by a blank line

::: toolkit/components/telemetry/Histograms.json
@@ +7290,5 @@
> +  "LOOP_SHARING_STATE_CHANGE": {
> +    "expires_in_version": "never",
> +    "kind": "count",
> +    "keyed": true,
> +    "releaseChannelCollection": "opt-out",

Is this really meant to be opt-out? Its unclear from the bug description, hence why I'm asking.

In any case, this will need approval from a data collection owner/peer as per https://wiki.mozilla.org/Firefox/Data_Collection

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8578608 [details] [diff] [review]
Patch v1: add telemetry for window and tab sharing triggers

Hi Vladan, are you ok with reviewing the histogram changes here? Please let me know if you need more information from me.

Comment 11

[Vladan Djeric (:vladan)]

Comment on attachment 8578608 [details] [diff] [review]
Patch v1: add telemetry for window and tab sharing triggers

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

::: toolkit/components/telemetry/Histograms.json
@@ +7290,5 @@
> +  "LOOP_SHARING_STATE_CHANGE": {
> +    "expires_in_version": "never",
> +    "kind": "count",
> +    "keyed": true,
> +    "releaseChannelCollection": "opt-out",

Mark is right, opt-out probes need privacy approval. Adding an opt-out probe is equivalent to adding an FHR probe. I added a warning about this to the Telemetry wiki (https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe)

- Let's give this probe an expiration date (6 months in the future akin to bug 1123660), we can review whether it needs to be a permanent probe before its expiration date
- Consider setting the alert_emails field to receive email notifications of flucations in the histogram's data, it can help catch regressions and it identifies the histogram's "owner"

Comment 12

[Vladan Djeric (:vladan)]

Comment on attachment 8578608 [details] [diff] [review]
Patch v1: add telemetry for window and tab sharing triggers

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

r+ with comments above

Comment 13

[Mike de Boer [:mikedeboer]]

Thanks, both!

Pushed to fx-team with comments addressed: https://hg.mozilla.org/integration/fx-team/rev/5c87c0017531

Mark, `getStyle()` unfortunately only retrieves the state of OT UI elements, which we don't use. The current videoSource is not persisted, unfortunately.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c87c0017531

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8578608 [details] [diff] [review]
Patch v1: add telemetry for window and tab sharing triggers

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello screenshare feature
[User impact if declined]: We won't get any usage metrics for the new screenshare feature without this patch.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: Aurora-patch

Unbitrotted patch with necessary Telemetry API parts ported over.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8583171 [details] [diff] [review]
Aurora-patch

Vladan, do I need to do more to make this work on Fx 38?

Reference: https://mail.mozilla.org/pipermail/firefox-dev/2015-March/002797.html

Comment 18

[Vladan Djeric (:vladan)]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Vladan, do I need to do more to make this work on Fx 38?

Unfortunately, yes. Unified Telemetry only exists in 39. To get the same opt-out on Release behavior, you would have to modify the old FHR

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8583171 [details] [diff] [review]
Aurora-patch

Per irc, we should either skip this for 38, or do a patch based on the older apis. If its not really quick, I'd suggest skipping for 38 given the amount of other work we've got.

Comment 20

[Mike de Boer [:mikedeboer]]

Vladan, the telemetry probe we're adding here is opt-in. So do I really need to add something to FHR as well? Can't I just port this to Fx 38 with the `releaseChannelCollection` property removed?

If not, what do I need to do to make the necessary changes to FHR? Is there a resource available somewhere?

Comment 21

[Vladan Djeric (:vladan)]

easy(In reply to Mike de Boer [:mikedeboer] from comment #20)
> Vladan, the telemetry probe we're adding here is opt-in. So do I really need
> to add something to FHR as well? Can't I just port this to Fx 38 with the
> `releaseChannelCollection` property removed?

Oh I see, the original patch was opt-out initially.
You don't need to make any additional changes. You can make this histogram opt-out on Nightly 39 if you like.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b636a032b24

Comment 23

[Romain Testard [:RT]]

Saptarshi, we can't see "LOOP_SHARING_STATE_CHANGE" on http://telemetry.mozilla.org/. Can you please help us understand where to look for these events?

Comment 24

[Vladan Djeric (:vladan)]

(In reply to Romain Testard [:RT] from comment #23)
> Saptarshi, we can't see "LOOP_SHARING_STATE_CHANGE" on
> http://telemetry.mozilla.org/. Can you please help us understand where to
> look for these events?

Our existing tools (like the Telemetry dashboard and histogram change-detector) don't support keyed histograms at the moment, but enum histograms are well supported.

If you want to track counts for a fixed number of keys (4 in this case), you can just use an "enum" histogram with each bucket representing a key. You can even add new values in the future by declaring the enum histogram with extra unused buckets. 

Keyed histograms are the better choice only when the keys are arbitrary strings which may not be known ahead of time, e.g. BLOCKED_ON_PLUGIN_INSTANCE_INIT_MS histogram with keys such as "Flash16.0.1.17" and "DivX3.4"

Comment 25

[Romain Testard [:RT]]

Thanks Vladan. I am not familiar with how enum histogram can be created - is there a wiki page where it's described (could not find one)?

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #24)
> (In reply to Romain Testard [:RT] from comment #23)
> > Saptarshi, we can't see "LOOP_SHARING_STATE_CHANGE" on
> > http://telemetry.mozilla.org/. Can you please help us understand where to
> > look for these events?

So, when we first added this stuff, my understanding was that the plan was to process this stuff using the Heka pipeline and display it elsewhere, not Telemetry.  Saptarshi, did I misunderstand?  If not, are we still waiting for custom code to be written to display it?
  
> Our existing tools (like the Telemetry dashboard and histogram
> change-detector) don't support keyed histograms at the moment, but enum
> histograms are well supported.

Ah, that wasn't evident from <https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe>, which is what mostly framed the original implementation choice.  Perhaps that's worth updating in the interim?

> If you want to track counts for a fixed number of keys (4 in this case), you
> can just use an "enum" histogram with each bucket representing a key. You
> can even add new values in the future by declaring the enum histogram with
> extra unused buckets. 
> 
> Keyed histograms are the better choice only when the keys are arbitrary
> strings which may not be known ahead of time, e.g.
> BLOCKED_ON_PLUGIN_INSTANCE_INIT_MS histogram with keys such as
> "Flash16.0.1.17" and "DivX3.4"

The reasoning for using keyed histograms (despite the architectural misfit that you point) is because enum histograms have some usability issues if you're actually trying to use them as enums rather than just numeric histograms, stemming from the fact that labels can only be integer-typed.  I'll file a separate bug on that shortly.  In some ideal world, it could be nice to get that fixed before switching over.

However, as Mike pointed out to me in IRC, it may well be more pragmatic to switch these to enums sooner in order to get more and useful data to PM for 38/39.

Comment 27

[Vladan Djeric (:vladan)]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #26)
> Ah, that wasn't evident from
> <https://developer.mozilla.org/en-US/docs/Mozilla/Performance/
> Adding_a_new_Telemetry_probe>, which is what mostly framed the original
> implementation choice.  Perhaps that's worth updating in the interim?

Good point, I updated the page

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Bug 1135095 filed describing the issues with enums only allowing integer labels.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #27)
> 
> Good point, I updated the page

Nice; thanks!

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Bug 1153471 filed describing the issues with enums only allowing integer labels.

Comment 31

["Saptarshi Guha[:joy]"]

(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #26)
> (In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #24)
> > (In reply to Romain Testard [:RT] from comment #23)
> > > Saptarshi, we can't see "LOOP_SHARING_STATE_CHANGE" on
> > > http://telemetry.mozilla.org/. Can you please help us understand where to
> > > look for these events?
> 
> So, when we first added this stuff, my understanding was that the plan was
> to process this stuff using the Heka pipeline and display it elsewhere, not
> Telemetry.  Saptarshi, did I misunderstand?  If not, are we still waiting
> for custom code to be written to display it?

I'm not clear with the details of that, but more importantly, I wouldn't be correct person to process Heka data. Sorry for any misunderstanding .

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

I spent some time chatting with Saptarshi today, and I now think my original understanding was wrong: 
the current theory, as I understand it, is that unified telemetry probe data (opt-in or opt-out) is still going to be (mostly?) handled and displayed via telemetry.mozilla.org.  Though, also as I understand it, that data could now more easily be handled in the Heka pipeline.  Feel free to correct anything I got wrong.