Closed
Bug 1137813
Opened 9 years ago
Closed 9 years ago
Loop Client: Add new "status" action to POST /rooms/{token}
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox40 fixed)
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: abr, Assigned: standard8)
References
Details
(Whiteboard: [loop-metrics])
Attachments
(4 files, 6 obsolete files)
3.11 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
6.70 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
40.93 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
2.63 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
To allow for logging of room session states, the client should send status messages to the Loop server as described in https://wiki.mozilla.org/Loop/Architecture/Rooms#Updating_Session_State Generally, this should involve handling the six events indicated on that page (or, for those we handle, adding to the handling) in order to drive the state machine shown. Whenever one of the listed events occurs (even those events that do not trigger a state change), the client should send a POST message to "/rooms/{token}" with an action of "status."
Reporter | ||
Updated•9 years ago
|
Summary: Add new "status" action to POST /rooms/{token} → Loop Client: Add new "status" action to POST /rooms/{token}
Comment 1•9 years ago
|
||
Talking with abr, this bug should give us the visibility we need to get good data out of our logs. Since better metrics is a goal for this quarter, I made this is a P2 with a high rank. There's a matching server bug; I'll ping Alexis and Tarek to ask them to add it to their active bug queue.
Rank: 25
Priority: -- → P2
Assignee | ||
Comment 2•9 years ago
|
||
Adding dependency, as I don't think its worth working on this until we get the server side at least ready from the development perspective.
Depends on: 1137849
Reporter | ||
Updated•9 years ago
|
Whiteboard: [loop-metrics]
Comment 3•9 years ago
|
||
Attachment #8575218 -
Flags: review?(rhubscher)
Attachment #8575218 -
Flags: review?(alexis+bugs)
Updated•9 years ago
|
Attachment #8575218 -
Flags: review?(rhubscher) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8575218 [details] [review] PR 312 This PR was destined for bug 1137849 - so I've just moved it across there.
Attachment #8575218 -
Attachment is obsolete: true
Attachment #8575218 -
Flags: review?(alexis+bugs)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → standard8
Iteration: --- → 39.2 - 23 Mar
Points: --- → 5
OS: Mac OS X → All
Hardware: x86 → All
Updated•9 years ago
|
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Comment 5•9 years ago
|
||
This just does a little tidy up of event names.
Assignee | ||
Comment 6•9 years ago
|
||
This does the main metrics implementation. It currently needs tests, but I believe it works, if the server is fixed as requested in bug 1144851.
Assignee | ||
Comment 7•9 years ago
|
||
A little tidy up of event names before we do the main part.
Attachment #8579576 -
Attachment is obsolete: true
Attachment #8579577 -
Attachment is obsolete: true
Attachment #8581572 -
Flags: review?(mdeboer)
Assignee | ||
Comment 8•9 years ago
|
||
Some unit test tidy-up. This moves the dispatcher.dispatch stub to global and calls the action functions direct. This is an approach we've been switching to, and simplifies stubbing and call counting. This makes implementing the tests for the main part a bit easier.
Attachment #8581573 -
Flags: review?(mdeboer)
Assignee | ||
Comment 9•9 years ago
|
||
The main part of the patch with all the status logging and some tests. Since the server validation is going away, we can log what is seems more natural than the server was trying to force us. See bug 1144851 comment 0 for example logs this gives. I'm planning to hold off landing this until bug 1144851 and bug 1137849 are deployed to the server, but that shouldn't be too long (as in hopefully this week), so I think getting review now is the best thing, and then we can land as soon as the server deploys.
Attachment #8581575 -
Flags: review?(mdeboer)
Updated•9 years ago
|
Rank: 25 → 20
Comment 10•9 years ago
|
||
Comment on attachment 8581572 [details] [diff] [review] Part 1. Tidy up some event handler names in Loop's otSdkDriver. Review of attachment 8581572 [details] [diff] [review]: ----------------------------------------------------------------- Agreed, this rename and re-group of events looks better.
Attachment #8581572 -
Flags: review?(mdeboer) → review+
Comment 11•9 years ago
|
||
Comment on attachment 8581573 [details] [diff] [review] Part 2. Stub dispatcher.dispatch globally in otSdkDriver_test.js to simplify tests. Review of attachment 8581573 [details] [diff] [review]: ----------------------------------------------------------------- Nice :)
Attachment #8581573 -
Flags: review?(mdeboer) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8581575 [details] [diff] [review] Part 3. Add room connection status logging. Review of attachment 8581575 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good, but I'd like to give it another pass before r+'ing it. I hope you're ok with that. I recommend to not number the patches in your commit messages, because the previous two are quite unrelated to this one. ::: browser/components/loop/content/shared/js/actions.js @@ +447,5 @@ > + /** > + * Used to inform of the current sdk session, publisher and connection > + * status. > + */ > + SdkStatus: Action.define("sdkStatus", { I'd prefer to not pollute our code too much with 'SDK'... Can you rename this to 'ConnectionStatus'? ::: browser/components/loop/content/shared/js/activeRoomStore.js @@ +512,5 @@ > this.setStoreState({roomState: ROOM_STATES.SESSION_CONNECTED}); > }, > > /** > + * Handles a sdk status update, forwarding it to the server. nit: an SDK ::: browser/components/loop/content/shared/js/otSdkDriver.js @@ +387,4 @@ > this.dispatcher.dispatch(new sharedActions.RemotePeerConnected()); > }, > > + _calculateConnectedState: function() { With 'calculate' I expected to see some math down here, so I was _very_ disappointed to see none of that! ;-) Perhaps it's better to just rename this to `_getConnectionState` @@ +407,5 @@ > + * @param {String} clientType Used for connection created/destoryed. Indicates > + * if it is for the "peer" or the "local" client. > + */ > + _notifyMetricsEvent: function(eventName, clientType) { > + if (!eventName && !clientType) { Please change this to `if (!eventName) {`. You can't really check for optional arguments like this. @@ +416,5 @@ > + > + // We intentionally don't bounds check these, in case there's an error > + // somewhere, if there is, we'll see it in the server metrics and are more > + // likely to investigate. > + if (eventName) { Please remove this if-statement. You don't ever call this function without an eventName. If some consumer does, it should just bail early. @@ +455,5 @@ > + } > + > + if (state) { > + this._metrics.state = state; > + } to make this scope-wide record-keeping of the `state` useful, I'd like to suggest to make the state member an array, which we might later use for debugging the connection flow. so: `this._metrics.state.push(state);` and, if you alias `this._metrics` to `m` at the top of this function: `state: m.state[m.state.length - 1],` Or you remove the state record-keeping from the metrics object.
Attachment #8581575 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Mike de Boer [:mikedeboer] from comment #12) > @@ +455,5 @@ > > + if (state) { > > + this._metrics.state = state; > > + } > > to make this scope-wide record-keeping of the `state` useful, I'd like to > suggest to make the state member an array, which we might later use for > debugging the connection flow. > > so: `this._metrics.state.push(state);` > and, if you alias `this._metrics` to `m` at the top of this function: > `state: m.state[m.state.length - 1],` > > Or you remove the state record-keeping from the metrics object. Ah, I was originally designing the state handling expecting some events to need to remember the previous state. However, it looks like I've changed that now so that we always generate a state, so I can just drop the keeping of it (console logging should have enough historical data if we need it).
Assignee | ||
Comment 14•9 years ago
|
||
Updated for bitrot
Attachment #8581572 -
Attachment is obsolete: true
Attachment #8581573 -
Attachment is obsolete: true
Attachment #8581575 -
Attachment is obsolete: true
Attachment #8583755 -
Flags: review+
Assignee | ||
Comment 16•9 years ago
|
||
Updated for bitrot and review comments
Attachment #8583757 -
Flags: review?(mdeboer)
Comment 17•9 years ago
|
||
Comment on attachment 8583757 [details] [diff] [review] Part 3. Add room connection status logging. Review of attachment 8583757 [details] [diff] [review]: ----------------------------------------------------------------- \o/ Looks good! ::: browser/components/loop/LoopRooms.jsm @@ +441,5 @@ > + * > + * @param {String} roomToken The room token. > + * @param {String} sessionToken The session token for the session that has been > + * joined. > + * @param {sharedActions.SdkStatus} status The connection status. s/SdkStatus/ConnectionStatus/ nit: can you align the params nicely?
Attachment #8583757 -
Flags: review?(mdeboer) → review+
Assignee | ||
Updated•9 years ago
|
Depends on: 1146844
Whiteboard: [loop-metrics] → [loop-metrics][waiting on server 0.17.0 to be deployed]
Assignee | ||
Updated•9 years ago
|
Iteration: 39.2 - 23 Mar → 40.1 - 13 Apr
Whiteboard: [loop-metrics][waiting on server 0.17.0 to be deployed] → [loop-metrics][ready to land,waiting on server 0.17.0 to be deployed]
Assignee | ||
Updated•9 years ago
|
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/76f476bd8e80 https://hg.mozilla.org/integration/fx-team/rev/3a5accf683d0 https://hg.mozilla.org/integration/fx-team/rev/57e3d90396e1
Assignee | ||
Updated•9 years ago
|
Whiteboard: [loop-metrics][ready to land,waiting on server 0.17.0 to be deployed] → [loop-metrics]
Target Milestone: --- → mozilla40
Assignee | ||
Comment 19•9 years ago
|
||
A short follow-up. In testing loop-client just before releasing, I realised we're getting 401 errors back from the server as we're trying to send the "cleanup" event after the room has been left. We decided in bug 1144851 that this is acceptable for now, so this just disables sending of it.
Attachment #8598547 -
Flags: review?(mdeboer)
Comment 20•9 years ago
|
||
Comment on attachment 8598547 [details] [diff] [review] Part 4. Don't log the cleanup event as the server doesn't accept it after the room has been left. Review of attachment 8598547 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/loop/content/shared/js/otSdkDriver.js @@ +441,5 @@ > this._metrics.connections--; > if (clientType === "local") { > + // Don't log this, as the server doesn't accept it after > + // the room has been left. > + //state = "cleanup"; Alright, but I think the comment alone is enough to explain things; can you remove the code, instead of commenting it out?
Attachment #8598547 -
Flags: review?(mdeboer) → review+
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/76f476bd8e80 https://hg.mozilla.org/mozilla-central/rev/3a5accf683d0 https://hg.mozilla.org/mozilla-central/rev/57e3d90396e1 https://hg.mozilla.org/mozilla-central/rev/cdf65dcd35ea
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•