Closed SongoMen closed 1 year ago
Merging #64 (7b2ad08) into main (5b6d1bf) will increase coverage by
5.94%
. The diff coverage is51.76%
.:exclamation: Current head 7b2ad08 differs from pull request most recent head 0284620. Consider uploading reports for the commit 0284620 to get more accurate results
@@ Coverage Diff @@
## main #64 +/- ##
==========================================
+ Coverage 73.96% 79.90% +5.94%
==========================================
Files 212 52 -160
Lines 15447 1717 -13730
==========================================
- Hits 11425 1372 -10053
+ Misses 4022 345 -3677
Impacted Files | Coverage Δ | |
---|---|---|
backend/api/src/config.rs | 38.46% <11.11%> (-53.21%) |
:arrow_down: |
backend/api/src/api/v1/gql/chat.rs | 54.12% <54.12%> (ø) |
|
backend/api/src/api/v1/gql/mod.rs | 100.00% <100.00%> (ø) |
|
backend/api/src/api/v1/gql/models/message.rs | 100.00% <100.00%> (ø) |
|
backend/api/src/dataloader/chat_room.rs | 100.00% <100.00%> (ø) |
|
backend/api/src/global/mod.rs | 100.00% <100.00%> (+30.32%) |
:arrow_up: |
This will have to go in after #63
I think, initially we should focus on doing chat via the GQL subscription model. Where users can subscribe to a chat stream and GQL subscriptions will handle the emit events for us. We can add support for a raw websocket version as well as an SSE version later when we do a full Rest API. however initially we should just stick with a simple GQL subscription.
Secondly, this design makes use of tokio's broadcast channels. This is great for a single instance application but if we scale this app our horizontally, instances of the application need to be able to receive the chat messages which are sent to other nodes in the cluster.
The only realistic way to go about the 2nd point is to have an event emitting system.
Currently we use postgres for a database. Postgres does support a pubsub model and can be used as a fanout event emitter. Redis is also an option. Both have pros and cons.
For postgres, as a pubsub I am not entirely sure of the performance around that. I know it is very nice because we can have events (such as updates, inserts, deletes) trigger pubsub so a simple query INSERT INTO chat_messages (x, y, z) ($1, $2, $3)
will cause a pubsub emit to some custom defined endpoint based on the data inserted into the table. However I am not too sure about the relative performance of that.
Redis, redis is very very battle tested and their pubsub is currently what powers 7tv event sub and we have never had any issues with the scalability of that (when it comes to the actual event sub, not the long lived connections) The disadvantage to redis is we will have to emit an event (on insert) manually. Also redis is unreliable (it does not care if anyone is listening to the event) in this case its fine because chat messages can be dropped and its not very important.
Both are super strong cases. If postgres is performant enough I would opt to go for that. Perhaps as part of this ticket you could look at the relative performance of how postgres scales with subscriptions and triggers on inserts / updates / deletes ect.
@SongoMen Can you rebase with main?
Thanks for rerunning codecov
code cov reports that the chat subscription event is never hit, could we add a unit test for this subscription event?
As well as the get_redis_config on the AppConfig struct
Other than that well done on your UTs very good coverage everywhere else!!! 🚀
Proposed changes
This PR adds chat functionality to the backend and frontend, enabling users to send and receive messages in real-time. The backend now uses a Redis pool to publish messages to channels and a single Redis connection to retrieve messages and pass them on to the user's subscription.
When a user enters a chat, they will send a subscription request to the server responsible for handling all messages for that chat. The max message length is 500 and the amount of messages that will be visible on the chat is 300.
Types of changes
What types of changes does your code introduce to Scuffle? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Closes #39