-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-17562] Initial POC of Distributed Events #5323
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5323 +/- ##
==========================================
- Coverage 44.32% 44.23% -0.09%
==========================================
Files 1483 1487 +4
Lines 68416 68572 +156
Branches 6173 6179 +6
==========================================
+ Hits 30325 30335 +10
- Misses 36782 36928 +146
Partials 1309 1309 ☔ View full report in Codecov by Sentry. |
New Issues (7)Checkmarx found the following issues in this Pull Request
Fixed Issues (17)Great job! The following issues were fixed in this Pull Request
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happened across this PR so maybe this feedback is a little early, if so sorry about that.
var message = JsonSerializer.Serialize(eventMessage); | ||
using var httpClient = new HttpClient(); | ||
var content = new StringContent(message, Encoding.UTF8, "application/json"); | ||
var response = await httpClient.PostAsync(_httpPostUrl, content); | ||
response.EnsureSuccessStatusCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be creating short lived HttpClient
's. We should instead created a named HttpClient
in DI and retrieve it with IHttpClientFactory
that gets injected here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be fixed now, but let me know if I need to refine it further. 👍
src/Core/AdminConsole/Services/Implementations/RabbitMqEventListenerBase.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventWriteService.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventWriteService.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventHttpPostListener.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things and some questions.
src/Core/AdminConsole/Services/Implementations/RabbitMqEventHttpPostListener.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventRepositoryListener.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventHttpPostListener.cs
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventListenerBase.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventListenerBase.cs
Outdated
Show resolved
Hide resolved
|
||
await channel.ExchangeDeclareAsync(exchange: _exchangeName, type: ExchangeType.Fanout); | ||
|
||
var body = JsonSerializer.SerializeToUtf8Bytes(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is JSON our best option for serialization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, at least to start. Were you thinking about something like Protobuf?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps ... just don't know best practices with Rabbit.
If you're running this to test locally, note that in refactoring all of the global settings, you'll need to adopt the new secrets and replublish them. The sample secrets are included in the description above (I've updated it to match the new format). After adding those, you'll need to run:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me as a first pass and we'll iterate on some additional topics as discussed.
|
||
await channel.ExchangeDeclareAsync(exchange: _exchangeName, type: ExchangeType.Fanout); | ||
|
||
var body = JsonSerializer.SerializeToUtf8Bytes(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps ... just don't know best practices with Rabbit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more thoughts.
src/Core/AdminConsole/Services/Implementations/RabbitMqEventHttpPostListener.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventRepositoryListener.cs
Outdated
Show resolved
Hide resolved
src/Core/AdminConsole/Services/Implementations/RabbitMqEventWriteService.cs
Show resolved
Hide resolved
Co-authored-by: Justin Baur <[email protected]>
src/Core/AdminConsole/Services/Implementations/RabbitMqEventRepositoryListener.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my perspective.
@withinfocus Is this POC a step in the direction of decoupling services and moving toward an event based architecture? Or is there a different end goal for this?
Not necessarily, yet -- just allowing our events to be ingested in more places. It's definitely an implicit enabler of that though, for the eventing angle at least. We could use this same backbone for other interprocess communication. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-17562
📔 Objective
This PR is an initial POC of a new way of handling events. The end goal will be to add an AMQP messaging for all platforms that will broadcast and fan-out events in addition to our normal handling of events. This allows multiple integrations to be built in the future (e.g a Slack message when X event occurs, etc). The Cloud version will ultimately be built on Azure Service Bus, but for this initial POC we wanted to just build something for local / self-hosted.
This PR adds a new opt-in flow for Events. Specifically:
IEventWriteService
implementation that writes events to the RabbitMQ exchangeThe following is copied from my PR in the contributing-docs repo. This is added to the Events page to document the new RabbitMQ additions and how to configure and start the service.
Distributed Events (optional)
There is a new system which will add support for distributing events via an AMQP nessaging system.
In the future this will enable new integrations by allowing for a means to subscribe to
events via messaging stream.
As an initial proof of concept, there is an optional RabbitMQ implementation that refactors the way
events are handled when running locally or self-hosted. Instead of writing directly to the
Events
tablevia the
EventsRepository
, it will broadcast each event via a RabbitMQ exchange. A newRabbitMqEventRepositoryListener
then subscribes to the RabbitMQ exchange and writes to theEvents
table via theEventsRepository
. The end result is the same (events are stored in thedatabase), but this allows for other integrations to subscribe.
To illustrate this, there is also a
RabbitMqEventHttpPostListener
which subscribes to the RabbitMQevents exchange and
POST
s each event to a configurable URL. This is meant to bea simple, concrete example of how multiple integrations are enabled by moving to
distributed events.
To enable the new RabbitMQ-based event stream, take the following steps:
Running the RabbitMQ container
Verify that you've set a RabbitMQ username and password in the
.env
file (see.env.example
for an example)
Use Docker Compose to run the container with your current settings:
This will run the RabbitMQ container with your username and password on localhost with the standard ports
To verify this is running, open
http://localhost:15672
in a browser and login with the username and password in your.env
file.Configuring the server to use RabbitMQ for events
Add the following to your
secrets.json
file, changing the defaults to match your.env
file:(optional) To use the
RabbitMqEventHttpPostListener
, specify a queue name and destinationURL that will receive the POST inside of the RabbitMq object:
receive these requests and let you inspect them.
Re-run the powershell script to add these secrets to each Bitwarden project:
Start (or restart) all of your projects to pick up the new settings
With these changes in place, you should see the database events written as before, but you'll also see in the RabbitMQ management interface that the messages are flowing through the configured exchange.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes