Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: chat room #64

Merged
merged 1 commit into from
Jun 14, 2023
Merged

feat: chat room #64

merged 1 commit into from
Jun 14, 2023

Conversation

SongoMen
Copy link
Member

@SongoMen SongoMen commented Feb 27, 2023

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 apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Checklist

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.

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Closes #39

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #64 (7b2ad08) into main (5b6d1bf) will increase coverage by 5.94%.
The diff coverage is 51.76%.

❗ 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%) ⬇️
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%) ⬆️

... and 212 files with indirect coverage changes

@TroyKomodo
Copy link
Member

This will have to go in after #63

@SongoMen SongoMen force-pushed the songomen/chat branch 2 times, most recently from 9169dc1 to d049326 Compare March 3, 2023 23:25
@TroyKomodo
Copy link
Member

TroyKomodo commented Mar 4, 2023

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 SongoMen force-pushed the songomen/chat branch 2 times, most recently from ecfd053 to ab864c9 Compare March 6, 2023 10:47
backend/api/src/main.rs Outdated Show resolved Hide resolved
@SongoMen SongoMen marked this pull request as ready for review March 8, 2023 23:29
@SongoMen SongoMen requested review from a team as code owners March 8, 2023 23:29
backend/api/Cargo.toml Outdated Show resolved Hide resolved
@SongoMen SongoMen temporarily deployed to lint-test-build March 28, 2023 19:51 — with GitHub Actions Inactive
@SongoMen SongoMen temporarily deployed to lint-test-build March 28, 2023 23:04 — with GitHub Actions Inactive
@SongoMen SongoMen temporarily deployed to lint-test-build March 29, 2023 13:52 — with GitHub Actions Inactive
Copy link
Member

@TroyKomodo TroyKomodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good start!

backend/api/Cargo.toml Outdated Show resolved Hide resolved
backend/api/src/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
backend/api/src/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
backend/api/src/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
backend/api/src/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
backend/api/src/tests/api/v1/gql/chat.rs Show resolved Hide resolved
backend/api/src/tests/api/v1/gql/chat.rs Show resolved Hide resolved
backend/api/src/tests/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
backend/api/src/tests/api/v1/gql/chat.rs Show resolved Hide resolved
backend/api/src/tests/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
@SongoMen SongoMen marked this pull request as draft March 30, 2023 14:24
@SongoMen SongoMen temporarily deployed to lint-test-build March 30, 2023 14:27 — with GitHub Actions Inactive
@TroyKomodo
Copy link
Member

TroyKomodo commented Mar 30, 2023

Thanks for rerunning codecov
image
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
image

Other than that well done on your UTs very good coverage everywhere else!!! 🚀

backend/api/src/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
backend/api/src/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
backend/api/src/api/v1/gql/chat.rs Outdated Show resolved Hide resolved
@TroyKomodo TroyKomodo temporarily deployed to lint-test-build June 11, 2023 18:35 — with GitHub Actions Inactive
@TroyKomodo TroyKomodo temporarily deployed to lint-test-build June 11, 2023 18:53 — with GitHub Actions Inactive
@TroyKomodo TroyKomodo temporarily deployed to lint-test-build June 11, 2023 21:12 — with GitHub Actions Inactive
@TroyKomodo TroyKomodo temporarily deployed to lint-test-build June 12, 2023 16:25 — with GitHub Actions Inactive
@TroyKomodo TroyKomodo temporarily deployed to lint-test-build June 12, 2023 18:10 — with GitHub Actions Inactive
@furSUDO furSUDO marked this pull request as ready for review June 12, 2023 18:18
@TroyKomodo TroyKomodo requested a review from treuks June 13, 2023 13:15
Added chat functionality to the backend and frontend, enabling users to
send and receive messages in real-time.
@TroyKomodo TroyKomodo merged commit 0284620 into ScuffleTV:main Jun 14, 2023
3 of 4 checks passed
@TroyKomodo TroyKomodo temporarily deployed to lint-test-build July 14, 2023 00:27 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chat Room
4 participants