Closed
Bug 1171962
Opened 8 years ago
Closed 8 years ago
In-conversation chat metrics
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox42 fixed, firefox43 fixed, firefox44 verified)
People
(Reporter: RT, Assigned: mikedeboer)
References
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 files, 1 obsolete file)
29.51 KB,
patch
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
31.30 KB,
patch
|
Details | Diff | Splinter Review | |
31.25 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Updated•8 years ago
|
Rank: 20
Flags: firefox-backlog+
Priority: -- → P2
Whiteboard: [chat][metrics]
Updated•8 years ago
|
Assignee: nobody → mdeboer
Iteration: --- → 42.1 - Jul 13
Assignee | ||
Comment 1•8 years ago
|
||
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.
Reporter | ||
Comment 2•8 years ago
|
||
(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•8 years ago
|
||
(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)
Comment 4•8 years ago
|
||
(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)
Updated•8 years ago
|
Iteration: 42.1 - Jul 13 → 42.2 - Jul 27
Assignee | ||
Updated•8 years ago
|
Points: --- → 5
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8635307 -
Flags: review?(vdjeric)
Attachment #8635307 -
Flags: review?(dmose)
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•8 years ago
|
||
Carrying over r=vladan,dmose.
Attachment #8635307 -
Attachment is obsolete: true
Attachment #8637927 -
Flags: review+
Comment 9•8 years ago
|
||
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!
Updated•8 years ago
|
Iteration: 42.2 - Jul 27 → 42.3 - Aug 10
Updated•8 years ago
|
Iteration: 42.3 - Aug 10 → 43.1 - Aug 24
Comment 10•8 years ago
|
||
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)?
Updated•8 years ago
|
Flags: needinfo?(erik)
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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: 8 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 14•8 years ago
|
||
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?
Updated•8 years ago
|
status-firefox42:
--- → affected
status-firefox43:
--- → affected
Comment 15•8 years ago
|
||
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+
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
Flags: needinfo?(mdeboer)
Assignee | ||
Comment 18•8 years ago
|
||
Updated•8 years ago
|
Flags: qe-verify+
Comment 22•8 years ago
|
||
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).
You need to log in
before you can comment on or make changes to this bug.
Description
•