Add publish_status and remove_status methods to WebSocketServer #192
Conversation
| 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.
| } | ||
|
|
||
| /// Removes status messages by id from all clients. | ||
| pub fn remove_status(&self, status_ids: Vec<String>) { |
There was a problem hiding this comment.
Maybe make it a bit more generic?
| pub fn remove_status(&self, status_ids: Vec<String>) { | |
| pub fn remove_status<II, S>(&self, status_ids: II) | |
| where | |
| II: IntoIterator<Item = S>, | |
| S: Into<String>, | |
| { |
Or maybe that's overkill.
There was a problem hiding this comment.
I did that initially, but we end up needing a Vec<String> anyway, and maybe I'm wrong but I think IntoIterator<String>.collect::<Vec<String>>() will create a new Vec, so then it's just worse in the common case where you already had a Vec.
Better to get the user to do the collect into a Vec if they have an iterator, or just pass it in if they have a Vec.
| @@ -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
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.