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

perf(VehiclesForRouteChannel): Use vehicles from PubSub instead of GenServer #225

Merged
merged 7 commits into from
Oct 25, 2024

Conversation

KaylaBrady
Copy link
Collaborator

@KaylaBrady KaylaBrady commented Oct 24, 2024

Summary

Ticket: [Name](https://app.asana.com/0/1205732265579288/1208619681848095/f)

What is this PR for?

Addresses the bottleneck found in Load Testing when many users are joining vehicles channels at once.

Vehicles.PubSub is largely a copy of Predictions.PubSub - there is probably a good basis for extracting out common pieces, but I think it is worth saving that for a separate ticket.

This does switch VehicleChannel to use PubSub as well for consistency.

Testing:

  • Added & updated unit tests
  • Ran locally on phone pointing to dev-orange deployiment
  • Confirmed logs for new PubSub module in Splunk as expected

@@ -31,14 +31,7 @@ defmodule MobileAppBackendWeb.PredictionsForStopsV2Channel do
@spec handle_info({:new_predictions, any()}, Phoenix.Socket.t()) ::
{:noreply, Phoenix.Socket.t()}
def handle_info({:new_predictions, new_predictions_for_stop}, socket) do
{time_micros, _result} =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tiny log cleanup while copying over behavior of predictions channel

@KaylaBrady KaylaBrady marked this pull request as ready for review October 24, 2024 19:59
@KaylaBrady KaylaBrady requested a review from a team as a code owner October 24, 2024 19:59
@KaylaBrady KaylaBrady requested review from EmmaSimon and removed request for a team October 24, 2024 19:59
Copy link
Contributor

@EmmaSimon EmmaSimon left a comment

Choose a reason for hiding this comment

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

Did you bother running load testing again with these changes just to get a sense of the new upper threshold for connections? It's fine if not, I'm mostly just curious.

@KaylaBrady
Copy link
Collaborator Author

@EmmaSimon thanks for asking - definitely worth running again, and I hadn't yet! Added results to the Load testing doc - this got us to push past 3,300 users at a rate of 5 users / second. I think we'll still want to tackle static data caching to improve that further, or start with a higher base # of instances running.

@KaylaBrady KaylaBrady merged commit 8af1b57 into main Oct 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy to dev-orange Automatically deploy this PR to dev-orange
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants