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

RESOLVED FIXED in Firefox 38

Status

Hello (Loop)
Client
P2
normal
Rank:
25
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: RT, Assigned: mikedeboer)

Tracking

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

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

(Whiteboard: [metrics])

User Story

[User story] As a product manager I want to understand how sharing gets used in the product so that I can make the right product decisions.

Send a Telemetry event in the following scenarios:
* Tab sharing enabled
* Tab sharing disabled
* Window sharing enabled
* Window sharing disabled

Attachments

(2 attachments)

Comment hidden (empty)
(Reporter)

Updated

4 years ago
Blocks: 1099241
User Story: (updated)
(Reporter)

Comment 1

4 years ago
Shell, can we please get this on Trello as discussed in the stand-ups?
Flags: needinfo?(sescalante)
(Reporter)

Comment 2

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

Comment 3

3 years ago
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).
Rank: 34
Flags: qe-verify-
Flags: needinfo?(sescalante)
Flags: in-testsuite?
Flags: firefox-backlog+
Priority: -- → P3
Whiteboard: [metrics]
(Reporter)

Comment 4

3 years ago
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
(Assignee)

Comment 5

3 years ago
Shell, since we bumped the prio here, can you adjust the rank field for me?
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Points: --- → 3
Flags: needinfo?(sescalante)
Priority: P3 → P1

Comment 6

3 years ago
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.
Rank: 34 → 25
Flags: needinfo?(sescalante)
Flags: needinfo?(rtestard)
Flags: needinfo?(gavin.sharp)
Priority: P1 → P2

Comment 7

3 years ago
Per Gavin's request - just updating bugs that carried over before they were 100% resolved to the next iteration.
Iteration: 38.3 - 23 Feb → 39.2 - 23 Mar
(Assignee)

Updated

3 years ago
Flags: needinfo?(rtestard)
Flags: needinfo?(gavin.sharp)
(Assignee)

Comment 8

3 years ago
Created attachment 8578608 [details] [diff] [review]
Patch v1: add telemetry for window and tab sharing triggers
Attachment #8578608 - Flags: review?(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
Attachment #8578608 - Flags: review?(standard8) → review+
(Assignee)

Comment 10

3 years ago
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.
Attachment #8578608 - Flags: review?(vdjeric)
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 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
Attachment #8578608 - Flags: review?(vdjeric) → review+
(Assignee)

Comment 13

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/5c87c0017531
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 15

3 years ago
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.
Attachment #8578608 - Flags: approval-mozilla-aurora?
Attachment #8578608 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 16

3 years ago
Created attachment 8583171 [details] [diff] [review]
Aurora-patch

Unbitrotted patch with necessary Telemetry API parts ported over.
Attachment #8583171 - Flags: review?(standard8)
(Assignee)

Comment 17

3 years ago
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
Attachment #8583171 - Flags: feedback?(vdjeric)
(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
Attachment #8583171 - Flags: feedback?(vdjeric)
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.
Attachment #8583171 - Flags: review?(standard8)
(Assignee)

Comment 20

3 years ago
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?
(Assignee)

Updated

3 years ago
Flags: needinfo?(vdjeric)
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.
Flags: needinfo?(vdjeric)
(Reporter)

Updated

3 years ago
Blocks: 1148381
(Reporter)

Comment 23

3 years ago
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?
Flags: needinfo?(sguha)
(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"
(Reporter)

Comment 25

3 years ago
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)?
(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.
(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 hidden (obsolete)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #27)
> 
> Good point, I updated the page

Nice; thanks!
Bug 1153471 filed describing the issues with enums only allowing integer labels.
(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 .
Flags: needinfo?(sguha)
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.
(Assignee)

Updated

3 years ago
Depends on: 1166646
You need to log in before you can comment on or make changes to this bug.