1171962 - In-conversation chat metrics

Status: VERIFIED FIXED

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

Comment 1

[Mike de Boer [:mikedeboer]]

Adam mentioned yesterday that it's prudent to add reporting of data channel connection establishment success & failure and message sending success/ failure.
However, pinging telemetry for each and every message sent might be a bit too much, even though data submissions are batched and scheduled.

Comment 2

[Romain Testard [:RT]]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Adam mentioned yesterday that it's prudent to add reporting of data channel
> connection establishment success & failure and message sending success/
> failure.
> However, pinging telemetry for each and every message sent might be a bit
> too much, even though data submissions are batched and scheduled.

How about an approach where we would just report the number of message delivery failures and message delivery successes per session at the end of the session?

Comment 3

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Adam mentioned yesterday that it's prudent to add reporting of data channel
> connection establishment success & failure and message sending success/
> failure.

Adam, I'm confused - originally you mentioned that we didn't need this level of protocol checking? (i.e. additional message delivery tracking).

Am I misunderstanding something here?

> However, pinging telemetry for each and every message sent might be a bit
> too much, even though data submissions are batched and scheduled.

Yeah, that feels too much. Session based would be fine. I think we should plan for it, but probably in a separate bug, and just keep this to the chat metrics maybe.

Comment 4

[Adam Roach [:abr]]

(In reply to Mark Banner (:standard8) (afk until 20th July) from comment #3)
> (In reply to Mike de Boer [:mikedeboer] from comment #1)
> > Adam mentioned yesterday that it's prudent to add reporting of data channel
> > connection establishment success & failure and message sending success/
> > failure.
> 
> Adam, I'm confused - originally you mentioned that we didn't need this level
> of protocol checking? (i.e. additional message delivery tracking).

I think for right now, if we can verify that the datachannel is being set up correctly, it's not a really high priority to make sure that each message makes it through. The information we get about media packet loss rates should be good enough to tell us that the end-to-end path is working, since media and datachannel follow the same path.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: introduce telemetry histogram and hooks to count IM sessions

Comment 6

[Vladan Djeric (:vladan)]

Comment on attachment 8635307 [details] [diff] [review]
Patch v1: introduce telemetry histogram and hooks to count IM sessions

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

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> However, pinging telemetry for each and every message sent might be a bit
> too much, even though data submissions are batched and scheduled.

This perfectly fine as far as Telemetry is concerned. We have gfx refresh-driver code accumulating a histogram sample every 16ms. Also, the sending of telemetry pings is not in any way related to the number of measurements logged. The only issue with reporting IM counts is privacy.

::: browser/components/loop/test/mochitest/browser_mozLoop_telemetry.js
@@ +187,5 @@
> +  let snapshot;
> +  for (let i = 1; i < 4; ++i) {
> +    gMozLoopAPI.telemetryAddValue(histogramId, 1);
> +    snapshot = histogram.snapshot();
> +    Assert.strictEqual(snapshot.counts[0], i);

don't forget to remove this test code before the histogram expires or this test will be perma-failing in Nightly 45

::: toolkit/components/telemetry/Histograms.json
@@ +7801,5 @@
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "expires_in_version": "45",
> +    "kind": "count",
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Number of sessions where at least one chat message was exchanged"

I'm guessing there's another histogram that records how many Loop convos of all types were created during the Firefox browser session?

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8635307 [details] [diff] [review]
Patch v1: introduce telemetry histogram and hooks to count IM sessions

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

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +14,4 @@
>    var FAILURE_DETAILS = loop.shared.utils.FAILURE_DETAILS;
>    var SCREEN_SHARE_STATES = loop.shared.utils.SCREEN_SHARE_STATES;
>  
> +

Nit: extra blank link here.

@@ +577,5 @@
> +      var actions = ["receivedTextChatMessage", "sendTextChatMessage"];
> +      // Unregister first, just to be sure.
> +      this.dispatcher.unregister(this, actions);
> +      this.dispatcher.register(this, actions);
> +

Could this count only once across multiple sessions in a room if the desktop browser doesn't quit?

I _think_ that would be the wrong thing to content, since this number is supposed to be comparable to sessions.  If that's true, you could consider doing this stuff in the "dataChannelsAvailable" action handler.

We probably want automated tests to nail down the behavior in both the single and multiple session cases, whichover one is the right one to count.

@@ +960,5 @@
> +    /**
> +     * Handles chat messages received and/ or about to send. If this is the first
> +     * chat message for the current session, register a count with telemetry.
> +     * It will unhook the listeners when the telemetry criteria have been
> +     * fulfilled to make sure we remain lean.

It would be good to be explicit in this comment) that this counts on the desktop side but not the standalone side.

::: browser/components/loop/content/shared/js/utils.js
@@ +110,5 @@
> +    CONTEXT: "chat-context",
> +    TEXT: "chat-text",
> +    ROOM_NAME: "room-name"
> +  };
> +

Please removing the original definition in textChatStore.js and point existing references to this one so we avoid the situation where one table is updated and the other isn't.

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +1569,5 @@
> +      expect(dispatcher._eventData.receivedTextChatMessage.length).eql(1);
> +      expect(dispatcher._eventData.sendTextChatMessage.length).eql(1);
> +      expect(store.getStoreState().chatMessageExchanged).eql(false);
> +      sinon.assert.notCalled(fakeMozLoop.telemetryAddValue);
> +    }

I like the modularization here!

::: browser/components/loop/test/shared/dispatcher_test.js
@@ +44,5 @@
> +
> +      dispatcher.register(object, ["getWindowData"]);
> +      dispatcher.unregister(object, ["getWindowData"]);
> +
> +      expect(dispatcher._eventData.hasOwnProperty("getWindowData")).eql(false);

I get that this is prevailing style in this file, and this is also doesn't seem terribly high priority, so you don't need to change this now if you don't want.  

But if (at least) these new tests worked using only the public APIs (e.g. by dispatching actual events) rather than looking at implementation details, the tests would be more useful/robust for code refactoring.  If you don't want to do this now, how about leaving an XXX comment behind suggesting this for the future?

@@ +60,5 @@
> +
> +      dispatcher.unregister(object2, ["getWindowData"]);
> +      expect(dispatcher._eventData.hasOwnProperty("getWindowData")).eql(false);
> +    });
> +  })

Nit: missing final semicolon.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: introduce telemetry histogram and hooks to count IM sessions

Carrying over r=vladan,dmose.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

As far as I can tell, this got missed:

> We probably want automated tests to nail down the behavior in both the single and multiple session cases,
> whichover one is the right one to count.

http://logs.glob.uno/?c=mozilla%23loop#c43144 went over this somewhat in Loop, and the case I'm talking about should be counted as multiple sessions.

So please write a test to ensure that the code counts twice in the multiple session case.  I _think_ this should be doable as a unit test.  Thanks!

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Erik, is my comment 9 (and the log I link to there) still sensible, given your session definition update?  If not, can you clarify what _is_ correct?  (Maybe just the Sessions wiki page)?

Comment 11

[Erik Rose [:erik][:erikrose]]

Dan, I've updated https://wiki.mozilla.org/Loop/Session. The Session Progress algo (which I think is our most accurate log-based one atm) does indeed count 2 sessions in the following scenario:

A joins
B joins
A leaves
A rejoins

Let me know if that doesn't answer your question.

Comment 12

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/5e518c0314a6896b08bd5e299e07ba06cfa5c786
Bug 1171962: introduce telemetry histogram that counts the amount of sessions that exchanged one or more chat messages. r=vladan,dmose

Comment 13

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

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

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8637927 [details] [diff] [review]
Patch v2: introduce telemetry histogram and hooks to count IM sessions

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello chat feature
[User impact if declined]: Since this feature is already signed off and in Fx 41, we'd like to get a minimal amount of measurement to see how successful the feature is.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor, one telemetry histogram added.
[String/UUID change made/needed]: n/a.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8637927 [details] [diff] [review]
Patch v2: introduce telemetry histogram and hooks to count IM sessions

Sure, anything which can help you making better calls.

Comment 16

[Carsten Book [:Tomcat]]

Hi, this cause conflicts like warning: conflicts during merge.
merging browser/components/loop/content/shared/js/activeRoomStore.js incomplete! (edit conflicts, then use 'hg resolve --mark')
merging browser/components/loop/content/shared/js/utils.js
merging browser/components/loop/test/shared/activeRoomStore_test.js
merging browser/components/loop/test/shared/otSdkDriver_test.js
merging browser/components/loop/ui/ui-showcase.js
merging browser/components/loop/ui/ui-showcase.jsx
merging toolkit/components/telemetry/Histograms.json

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Branch patch: aurora-only.

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Branch patch: beta-only.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7bbe47be7493

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6dcd21137163

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6dcd21137163

Comment 22

[Bogdan Maris, Desktop QA]

We verified that each new chat session, will result in a count registered in Histrograms under LOOP_ROOM_SESSION_WITHCHAT. We tested using Firefox 44 beta 2 across platforms (Windows 7 64-bit, Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 64-bit).