In-conversation chat metrics

VERIFIED FIXED in Firefox 42

Status

defect
P2
normal
Rank:
20
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: RT, Assigned: mikedeboer)

Tracking

unspecified
mozilla44
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed, firefox44 verified)

Details

(Whiteboard: [chat][metrics])

User Story

As a product manager, I want to understand what share of Hello sessions use chat.

Acceptance criteria:
- If at least 1 IM gets exchanged during a conversation session, report a Hello session with IM
- The number of IM Hello sessions can be compared to the overall number of Hello sessions (opt-out Telemetry events)

Attachments

(3 attachments, 1 obsolete attachment)

No description provided.
Blocks: 1108893
User Story: (updated)
Rank: 20
Flags: firefox-backlog+
Priority: -- → P2
Whiteboard: [chat][metrics]
Assignee: nobody → mdeboer
Iteration: --- → 42.1 - Jul 13
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.
(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?
(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.
Flags: needinfo?(adam)
(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.
Flags: needinfo?(adam)
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
Points: --- → 5
Attachment #8635307 - Flags: review?(vdjeric)
Attachment #8635307 - Flags: review?(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]:
-----------------------------------------------------------------

(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": ["firefox-dev@mozilla.org", "mdeboer@mozilla.com"],
> +    "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?
Attachment #8635307 - Flags: review?(vdjeric) → review+
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.
Attachment #8635307 - Flags: review?(dmose) → review+
Status: NEW → ASSIGNED
Carrying over r=vladan,dmose.
Attachment #8635307 - Attachment is obsolete: true
Attachment #8637927 - Flags: review+
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!
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
Iteration: 42.3 - Aug 10 → 43.1 - Aug 24
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)?
Flags: needinfo?(erik)
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.
Flags: needinfo?(erik)
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
https://hg.mozilla.org/mozilla-central/rev/5e518c0314a6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
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.
Attachment #8637927 - Flags: approval-mozilla-beta?
Attachment #8637927 - Flags: approval-mozilla-aurora?
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.
Attachment #8637927 - Flags: approval-mozilla-beta?
Attachment #8637927 - Flags: approval-mozilla-beta+
Attachment #8637927 - Flags: approval-mozilla-aurora?
Attachment #8637927 - Flags: approval-mozilla-aurora+
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
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.