Detect and flag missed notifications from RabbitMQ #4317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
echoes-hq
bot
added
the
echoes/initiative: scale
services/background-worker/src/Wire/BackendDeadUserNotificationWatcher.hs
Outdated
Show resolved
Hide resolved
services/background-worker/src/Wire/BackendDeadUserNotificationWatcher.hs
Show resolved
Hide resolved
services/background-worker/src/Wire/BackendDeadUserNotificationWatcher.hs
Outdated
Show resolved
Hide resolved
zebot
added
the
ok-to-test
elland
force-pushed
the
dead-notifications
branch
2 times, most recently
from
9eed61b to
05f0c1b
Compare
elland
force-pushed
the
dead-notifications
branch
10 times, most recently
from
3a1b28d to
2f0d7e3
Compare
elland
force-pushed
the
dead-notifications
branch
from
2f0d7e3 to
cfda2da
Compare
elland
force-pushed
the
dead-notifications
branch
from
d060f45 to
5c9b3db
Compare
elland
force-pushed
the
dead-notifications
branch
from
aca5dfe to
23d552c
Compare
elland
force-pushed
the
dead-notifications
branch
from
ca03c82 to
73ea202
Compare
…s' into dead-notifications
elland
commented
Nov 11, 2024
| ( Log.msg (Log.val "Could not find x-last-death-queue in headers") | ||
| . Log.field "error_configuring_dead_letter_exchange" (show headers) | ||
| ) | ||
| error "Could not find x-last-death-queue in headers" |
There was a problem hiding this comment.
We probably want to throw instead, so this can be handled by our middlewares.
| Log.err env.logger $ | ||
| Log.msg (Log.val "Could not parse msgHeaders into uid/cid for dead letter exchange message") | ||
| . Log.field "error_parsing_message" (show dat) | ||
| error "Could not parse msgHeaders into uid/cid for dead letter exchange message" |
services/background-worker/src/Wire/BackendDeadUserNotificationWatcher.hs
Show resolved
Hide resolved
mdimjasevic
approved these changes
Nov 12, 2024
|
Let's add the changelog to the parent. |
elland
merged commit 61bacf2
into
WPB-10308-use-rabbti-mq-classic-queues-for-notifications
10 checks passed
mdimjasevic
added a commit
that referenced
this pull request
Nov 12, 2024
mdimjasevic
added a commit
that referenced
this pull request
Nov 12, 2024
* create rabbitmq exchange * set up DLX * wip * create user queues on client add/update * make it internal server error * make galley compile * make rabbitmq mandatory * fix integration tests * set correct DLX headers * wip * Add a WIP test to consume notifs via `GET /events` * cannon: Roughly implement subscribing notifs from RabbitMQ * gundeck: Start implementing push to rabbitmq * integration: Fix assertion to assert on a real event * integration: Use correct vHost in cannon * cannon: Ensure exchange exists and publish event correctly on WS * NotificationSubsystem: Align names for queues and extract them as top level bindings * gundeck: Push events to RabbitMQ for compatible clients * integration: Assert that acked events don't come back * cannon: Forward client acks to rabbitmq * WIP: Get rid of channel as an explicit param for NotificationSubsystem actions * Get galley to compile. * Fix some easy TODOs. * Use these library better. * resolve rebase conflict better. * fix ghc errors. * Extract rabbitmq channel lookup into helper. * Move setupConsumableNotificationsClient from subsystems to gundeck. * Revert "Get galley to compile." This reverts commit 8a1840d. * wire-subsystems: Fix compile errors in tests * gundeck: doesn't depend on wire-subsystems (yet?) * brig: Remove unnecessary import * cannon: Don't create the queue for clients, expect it to already be there * cannon: close ws connection when something goes wrong * Funky! * Undo funkiness: Create Wire.API.WebSocket with types for comms on the new websocket * WIP: cannon: try to use the new types from wire-api The case split is not the nicest, perhaps we can solve it with one these things: - Bring back the funkiness for couple of commits ago - Use separate types for server to client and client to server messages * wire-api: Cabal file * gen nix stuff * ormolu * Is this a good way of representing websocket messages? * Resolve TODO. amqp doesn't offer a bulk push operation. instead it makes individual pushes performant enough. https://www.rabbitmq.com/docs/publishers * Refactor test, extend coverage. * Suggestions for better module name; removed "websocket" reference from some names. * Roundtrip tests for Message*To*. * Source comments. * debug failing test [WIP] * refactor tests a bit, fix ack, fix typos * fix test, handle connection closed, format * ping pong test * Maintain stable connection to rabbitmq from cannon. [WIP] * Fix typo. * Tune tests. * Remove ping-pong stuff. * Revert "Maintain stable connection to rabbitmq from cannon. [WIP]" This reverts commit 861ee10. problems with this approach: - there is a maximum number of chans / conn. - this is all very complicated and should be done separately. * Test multiple acks and out of order acks * cannon: Refactor code a little and more logging * integration: Deal with events websocket more gracefully, ensure all acks are sent * cannon: Easier to understand cleanup code * Add TODO for tomorrow * cannon: Ensure invalid messages don't accumulate * small re-org of code * Avoid using unsafeRange * Reduce top level functions * integration: Test that old and new clients can co-exist * gundeck: Optimize number of calls to brig * gundeck: Try to not kill brig * Fix typo * gundeck: Remove pairing comment * wire-api: Rename Wire.API.WebSocket -> Wire.API.Event.WebSocketProtocol * gundeck: Don't configure dead-lettering while declaring queues This should be done via Policies: https://www.rabbitmq.com/docs/parameters#policies When done with Policies, we can change our mind about how to deal with dead-lettering later because queues cannot be redeclared with with different headers. * More TODOs * Use direct exchange for user notifications Topic exchange is not very useful for our usecase * integration: Test that users only get notifs meant for them * integration/Notifications: Allow waiting for notifs without a client * Deflake newly written tests * cannon: Don't declare the exchange, its not needed * integration: Make cannon logLevel Warn again * integrations: Throw error if websocket responds with invalid JSON * Add ticket number to FUTUREWORK * Add notification Id to rabbitmq notifs This can be used by clients to detect duplicate deliveries. * Makefile/clean-rabbit: Also cleanup exchanges * gundeck: Get preexisting unit tests to pass * Set correct expiration on events pushed to RabbitMQ * fix a typo in the changelog * Fix a release note * Add changelogs for public and internal API changes * Move around routing key helper functions * Remove commented out code * Detect and flag missed notifications from RabbitMQ (#4317) * Update a change log to reflect changes from PR #4317 --------- Co-authored-by: Akshay Mankar <[email protected]> Co-authored-by: Matthias Fischmann <[email protected]> Co-authored-by: Marko Dimjašević <[email protected]> Co-authored-by: Igor Ranieri <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://wearezeta.atlassian.net/browse/WPB-11167
Checklist
changelog.d