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)

defect
Points:
5

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
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."
See Also: → 1137849
Summary: Add new "status" action to POST /rooms/{token} → Loop Client: Add new "status" action to POST /rooms/{token}
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
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
Whiteboard: [loop-metrics]
Attached file PR 312 (obsolete) —
Attachment #8575218 - Flags: review?(rhubscher)
Attachment #8575218 - Flags: review?(alexis+bugs)
Attachment #8575218 - Flags: review?(rhubscher) → review+
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: nobody → standard8
Iteration: --- → 39.2 - 23 Mar
Points: --- → 5
OS: Mac OS X → All
Hardware: x86 → All
Flags: qe-verify-
Flags: firefox-backlog+
This just does a little tidy up of event names.
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.
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)
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)
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)
Rank: 25 → 20
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 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 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-
(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).
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+
Updated for bitrot and review comments
Attachment #8583757 - Flags: review?(mdeboer)
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+
Depends on: 1146844
Whiteboard: [loop-metrics] → [loop-metrics][waiting on server 0.17.0 to be deployed]
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]
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Whiteboard: [loop-metrics][ready to land,waiting on server 0.17.0 to be deployed] → [loop-metrics]
Target Milestone: --- → mozilla40
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 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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: