Add publish_status and remove_status methods to WebSocketServer #192
Conversation
rust/foxglove/src/websocket.rs
Outdated
| self.data_plane_tx.try_send(message).unwrap_or_else(|err| { | ||
| tracing::warn!("Failed to send status to client {}: {err}", self.addr) | ||
| }); | ||
| self.send_data_lossy(message, MAX_SEND_RETRIES); |
There was a problem hiding this comment.
I wonder whether these should be sent over the control plane channel instead, so that we don't drop them. I presume status messages are relatively low-frequency and high-importance.
There was a problem hiding this comment.
Yeah, I think so given our discussion we had on slack about dropping messages and using status messages over control plane to notify that.
How about a mix? control_plane for warning and above, data plane for info?
There was a problem hiding this comment.
That seems pretty reasonable, sure.
| @@ -168,6 +168,22 @@ impl WebSocketServerHandle { | |||
| .await; | |||
| } | |||
|
|
|||
| /// Publishes a status message to all clients. | |||
There was a problem hiding this comment.
Maybe add links to the ws-protocol spec for these.
| level: crate::websocket::StatusLevel, | ||
| message: impl Into<String>, | ||
| id: Option<impl Into<String>>, |
There was a problem hiding this comment.
Since there's an optional argument, we might consider having the client construct the Status, and perhaps with a builder (imagine, e.g., Status::error("oh no").id("x")).
There was a problem hiding this comment.
Seems slightly better, yes
…to change it to disconnect slow client
| @@ -175,6 +174,18 @@ impl WebSocketServerHandle { | |||
| .await; | |||
| } | |||
|
|
|||
| /// Publishes a status message to all clients. | |||
| /// https://github.com/foxglove/ws-protocol/blob/main/docs/spec.md#status | |||
| pub fn publish_status(&self, status: Status) { | |||
There was a problem hiding this comment.
Need to add these methods on the blocking handle too.
Co-authored-by: Greg Smith <gasmith@foxglove.dev>
Co-authored-by: Greg Smith <gasmith@foxglove.dev>
Relatively simple PR to add support for publishing status messages to all connected clients, and removing them by list of ids.
WS Spec
Current Python implementation
One thing I did here is treat status messages the same as log messages, in that when the queue is full it will drop older messages from the data plane queue.