feat: broadcast persistence - #2022
Conversation
CRAP Score Report |
6401d83 to
4927967
Compare
10f9d60 to
ea15322
Compare
ea15322 to
52c9004
Compare
32e6fb2 to
859d340
Compare
| end | ||
| end | ||
|
|
||
| defp result_to_single_struct( |
- check `:persistence` and store as `:broadcast` - add logger meta `tenant_id` - simplify `with` to have smaller diff - json passthrough using Jason.Fragment - fix docs
859d340 to
b2cd728
Compare
d782018 to
1b069d4
Compare
7844cde to
3f94e7a
Compare
| with {:ok, db_conn} <- Connect.lookup_or_start_connection(tenant.external_id) do | ||
| auth_params = %{auth_params | topic: topic} | ||
| Authorization.get_write_authorizations(db_conn, auth_params) | ||
| with {:ok, db_conn} <- Connect.lookup_or_start_connection(tenant.external_id), |
There was a problem hiding this comment.
should we just move this Connect clause out so you don't need to return db_conn here and keep this function just checking for the write authorizations?
There was a problem hiding this comment.
I'm not sure if you meant out of the with clauses or out of handle_private_message but on main handle_private_message already calls Connect.lookup_or_start_connection/1 indirectly and now it just moved to to be more explicit and reused by permissions_for_message and maybe_persist so I'm not sure there's something else we can change here. 🤔
There was a problem hiding this comment.
I've added a comment above to be more clear
| map() | binary(), | ||
| content_type() | ||
| ) :: :ok | {:error, term()} | {:error, atom(), String.t()} | ||
| def broadcast(_auth_params, %Tenant{suspend: true}, _topic, _event, _private, _payload, _content_type) do |
There was a problem hiding this comment.
So if I understand well this means that every API request will try to persist?
I think this should be an option you pass from the API request. And if that's the case let's do this just on the new API which uses SingleBroadcast and leave BatchBroadcast alone?
There was a problem hiding this comment.
Good point!
I think this should be an option you pass from the API request
Added a query param persist (similar to private) to enable persistence.
4cac34a to
664d408
Compare
| Authorization.get_write_authorizations(db_conn, auth_params) | ||
| defp maybe_persist(%BroadcastPolicies{persist: true}, db_conn, tenant, topic, event, payload) do | ||
| if FeatureFlags.broadcast_persistence_enabled?(tenant.external_id) do | ||
| Task.Supervisor.start_child(Realtime.TaskSupervisor, fn -> |
There was a problem hiding this comment.
Same behavior as ack=false.
It might useful to add a ack query param but not on this PR.
There was a problem hiding this comment.
Hmm yeah we might want to consider something like persist=ack or something like this? As ack here is only related to the case when something is persisted?
There was a problem hiding this comment.
What if we add the ack param (defaults to false) into the API as well?
As ack here is only related to the case when something is persisted?
Currently there's no ack right? So the only possible logic is to try to persist in a best-effort case, but having ack we could wait and respond with message id. Not sure is there's much value tho, need to discuss.
There was a problem hiding this comment.
It really depends because we are mixing things here I think?
API is always ACK (single broadcast at least) as it will return error if broadcasting failed for example if the payload size is above the limit. See the status codes the single broadcast API can return
On the websocket if you don't want ack it will not reply if it exceeded a payload size.
Now persisting is sort of another layer on top of this not the only thing
WDYT?
| example: false, | ||
| description: "Whether this is a private broadcast (requires RLS authorization). Defaults to false." | ||
| ] | ||
| # TODO: uncomment when broadcast persistence is public and ready |
There was a problem hiding this comment.
To not expose it yet.
| @@ -152,11 +155,12 @@ defmodule Realtime.Tenants.SingleBroadcast do | |||
|
|
|||
| defp handle_private_message(tenant, auth_params, topic, event, payload, content_type, rate_counter) do | |||
| case permissions_for_message(tenant, auth_params, topic) do | |||
There was a problem hiding this comment.
Calling {:ok, db_conn} <- Connect.lookup_or_start_connection(tenant.external_id) here, and then you don't need to get db_conn from "permissions_for_message" now that the connection is being used for two different things it makes less sense to have Connect being called from permissions_for_message
| defp convert_to_persistable_fields(%{"event" => event, "payload" => payload}), do: {:ok, event, payload} | ||
|
|
||
| defp convert_to_persistable_fields({event, :json, user_payload, _metadata}), | ||
| do: {:ok, event, Jason.Fragment.new(user_payload)} |
There was a problem hiding this comment.
Oh so Jason.Fragment will be properly stored as JSON without decoding/re-encoding by Ecto?
There was a problem hiding this comment.
Yes although it's a bit tricky to test that exact path, but still tested indirectly by all other tests.
| :ok | ||
| end | ||
|
|
||
| test "public broadcast is delivered but not stored", %{ |
There was a problem hiding this comment.
Should we make one of these tests send a binary payload?
| |> expect(:get_write_authorizations, 2, fn | ||
| _, %{topic: ^topic} -> %Policies{broadcast: %BroadcastPolicies{write: true}} | ||
| _, _ -> %Policies{broadcast: %BroadcastPolicies{write: false}} | ||
| _, %{topic: ^topic} -> {:ok, %Policies{broadcast: %BroadcastPolicies{write: true}}} |
There was a problem hiding this comment.
huh how was this test not breaking?
There was a problem hiding this comment.
Because the code is:
case Authorization.get_write_authorizations(db_conn, auth_params) do
{:ok, policies} -> policies
{:error, :not_found} -> nil
error -> error
endSo it just falls into the error clause returning the struct which is the exact same return when it pass {:ok, policies} (the first clause).
Since Authorization.get_write_authorizations/2 returns only {:ok, ...} | {:error, ...} (per spec) we should probably remove error -> error. Wdyt?
| end | ||
| end | ||
|
|
||
| describe "message persistence" do |
There was a problem hiding this comment.
There are no more changes on batch_broadcast 🤔 How are these tests passing?
There was a problem hiding this comment.
Because the only test now is test "the batch API never stores messages, even when authorized to persist" to make it explicit that batch broadcast doesn't persist messages.
There was a problem hiding this comment.
ooooh sorry I should've read it...
| assert :ok = SingleBroadcast.broadcast(auth_params, tenant, topic, "event", true, %{"a" => "b"}, :json, true) | ||
|
|
||
| assert {:ok, []} = Repo.all(db_conn, messages_for(topic), Message) |
There was a problem hiding this comment.
This test could be passing if the task supervisor has not inserted right? That's why eventually is used above, correct?
I wonder if we should check that there are no children under the task supervisor? 🤔
Or us a refute eventually with retries like down below?
There was a problem hiding this comment.
For now I'm avoiding adding too much code around tasks because that will be most likely temporary, we need something better to handle hard limits and pressure which is to be done in a following PR so I think it's okay for this version.
| boolean() | ||
| ) :: :ok | {:error, term()} | {:error, atom(), String.t()} | ||
| def broadcast(_auth_params, %Tenant{suspend: true}, _topic, _event, _private, _payload, _content_type) do | ||
| def broadcast(auth_params, tenant, topic, event, private, payload, content_type, persist \\ false) |
There was a problem hiding this comment.
I wonder if we should use opts here for private and persist? It gets harder to read boolean arguments like this. WDYT?
There was a problem hiding this comment.
Yep I prefer opts as well, changed it. 👍🏻
|
|
||
| assert_receive {:socket_push, _encoding, _data} | ||
| assert log =~ "UnableToPersistMessage" | ||
| assert {:ok, []} = Repo.all(db_conn, messages_for(topic), Message) |
There was a problem hiding this comment.
given the task stuff is it enough to look once for messages?
There was a problem hiding this comment.
socket_fixture in this module does ack_broadcast = true by default so the broadcast handler wait for that insert, but I also added ack_broadcast: true in the test to make it explicit.
| db_conn: db_conn, | ||
| tenant: tenant | ||
| } do | ||
| stub(GenCounter, :add, fn _ -> :ok end) |
There was a problem hiding this comment.
Because it shares the same setup and tenant with others tests that assert rate_counter.avg causing flaky tests.
There was a problem hiding this comment.
I see... We might want fix these flaky tests then. If you see them again let's see if we can fix it!
- pass opts with :private and :persist - fix msg "private channels" - test store and replay persisted binary messages - enable persistence per tenant in tests to simplify and make tests more reliable - pass an explicit ack:true to socket_fixture in tests to make it explicit
edgurgel
left a comment
There was a problem hiding this comment.
I've added some comments but other than those LGTM
Adapt the persistence probe to the lazy per-extension write authorization introduced in #2089: :persistence is now a valid extension requested on its own instead of an entry appended to the checked list.
|
🎉 This PR is included in version 2.132.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What kind of change does this PR introduce?
Introduce Broadcast Persistence allowing users to opt-in to
:persistenceto store broadcasted messages that can be replayed or manipulated inrealtime.messages.Closes REAL-956
Notes
trueawait persistence and return message id on responsefalsedo not wait persistence, just fire and forgetbroadcast_persistenceRef REAL-956
Testing
1. Enable feature flag
2. Create policies
3. Set token
claims = %{"role" => "authenticated", "sub" => "demo", "exp" => System.system_time(:second) + 100000000000} token = Joken.generate_and_sign!(%{}, claims, Joken.Signer.create("HS256", "dev"))4. Broadcast from WS
# broadcast {"topic":"realtime:persisted:demo","event":"phx_join","payload":{"config":{"private":true,"broadcast":{"self":true,"ack":true}}},"ref":"1"} {"topic":"realtime:persisted:demo","event":"broadcast","payload":{"type":"broadcast","event":"hello","payload":{"msg":1}},"ref":"2"}5. Broadcast from API
6. Broadcast from DB
7. Replay all messages
# replay {"topic":"realtime:persisted:demo","event":"phx_join","payload":{"config":{"private":true,"broadcast":{"replay":{"since":0,"limit":100}}}},"ref":"1"}