Add performance stats API #445
Conversation
| @@ -30,10 +30,15 @@ import { Sync } from './sync'; | |||
| * Class to handle operational messages with a client. | |||
| */ | |||
| export class Execution extends Protocol { | |||
| public networkStats: NetworkStats; | |||
There was a problem hiding this comment.
This should go on the Connection class, as a peer of ConnectionQuality. #Closed
|
|
||
| /** @private */ | ||
| public beforeSend(message: Message, promise?: ExportedPromise): Message { | ||
| const data = new Buffer(JSON.stringify(message, (key, value) => { |
There was a problem hiding this comment.
Benefit to putting NetworkStats on Connection: You won't have to double-serialize the message to get the size. #Closed
There was a problem hiding this comment.
Actually that's not true. The Pipe connection class deals with objects and not serialized data, so it would still have to manually serialize. #Closed
There was a problem hiding this comment.
oh you're right. that is more efficient, but annoying in this instance.
In reply to: 325397097 [](ancestors = 325397097)
| import filterEmpty from '../utils/filterEmpty'; | ||
| import validateJsonFieldName from '../utils/validateJsonFieldName'; | ||
|
|
||
| export interface NetworkStatsFrame { |
There was a problem hiding this comment.
interface [](start = 7, length = 9)
For consistency, prefer type over interface. Reserve interface for types that will be implemented by a class. #Closed
| .filter(client => client.id !== clientId && client.isJoined()) | ||
| .forEach(client => client.setAuthoritative(false)); | ||
| const oldAuthority = this.authoritativeClient; | ||
| const newAuthority = this._clientSet[clientId]; |
There was a problem hiding this comment.
I was hoping this could be handled on pipe.remote. Getting the stats from the authoritative peer is a lot of new code. #Closed
There was a problem hiding this comment.
Nothing gets serialized or deserialized anywhere but in a WebSocket connection. If we did it in the pipe it would add overhead.
In reply to: 325909690 [](ancestors = 325909690)
There was a problem hiding this comment.
Agreed this is better approach than adding overhead in in pipe.remote.
In reply to: 325918122 [](ancestors = 325918122,325909690)
| const toOldAuthority = oldAuthority && oldAuthority.conn instanceof EventedConnection | ||
| ? oldAuthority.conn : null; | ||
| if (oldAuthority) { | ||
| oldAuthority.setAuthoritative(false); |
There was a problem hiding this comment.
You changed the behavior here. We used to send .setAuthoritative(false) to all other clients. You might be introducing a bug here (or exposing one). Safer to keep it the way it was. #Closed
| }; | ||
|
|
||
| /** A collection of network statistics from a certain point in time. */ | ||
| export interface NetworkStatsReport { |
There was a problem hiding this comment.
interface [](start = 7, length = 9)
This should be a type, not an interface. #Closed
| * A snapshot of various stats useful for debugging and benchmarking. None of these correlate exactly with | ||
| * a particular client's experience, framerate, memory usage, etc., but should be generally indicative. | ||
| */ | ||
| export interface PerformanceStats { |
There was a problem hiding this comment.
interface [](start = 7, length = 9)
Should be a type not an interface #Closed
| * that clients are wasting CPU cycles serializing and deserializing messages. | ||
| */ | ||
| networkMessageCount: [number, number, number]; | ||
| } |
There was a problem hiding this comment.
Needs a semicolon #Closed
Implements #374