Introduce Gitlab::EventStore as simple pub-sub system
Problem
With GitLab monolith becoming larger and more domains being defined we have a problem of entangling these domains with each others due to temporal coupling.
An emblematic example is PostReceive
worker where a lot happens across multiple domains. If a new behavior requires to react to a new commit being pushed then we add code somewhere in PostReceive
or its sub-components. (Git::ProcessRefChangesService
, etc.). There are of course may of these examples in our codebase.
This type of architecture causes:
- Violation of the Single Responsibility Principle.
- Increase risk in adding code to a code base you are not familiar with, simply because your domain needs to do something at that particular moment. There may be nuances you don't know about which may introduce bugs or performance degradation.
- Domain boundaries are violated. Within a specific namespace (e.g.
Git::
) we suddenly see classes from other domains chiming in (Ci::
,MergeRequests::
, etc.).
What does this MR do?
This MR introduces an event-driven pattern to allow us decoupling domains by implementing a simple pub-sub system. This system leverages our existing Sidekiq workers and the observability we have today, so no new technology or message queuing systems.
This essentially leaves the existing workers as is to perform async work but inverts the dependency where:
- before: a worker for domain
A
was scheduled from inside the domainB
- after: domain
A
subscribes to an eventX
from domainB
using its workerA::SomeWorker
. WhenB
publishes an eventX
, it's consumed by its subscribers, includingA::SomeWorker
.
Next steps
- Log publishing of domain events and dispatching to subscribers, in order to support better debugging in Kibana.
-
Enable the feature flag
ci_publish_pipeline_events
- Ensure that
correlation_id
from subscribers call be traced back to the caller - add provenance of events - Deprecate
UpdateHeadPipelineForMergeRequestWorker
in favour of inlining the code - Extract shared examples to test that events are published and received and have it as standard way to ensure end-to-end message passing.
- Static analyzer to ensure
perform
method is not overridden when includingGitlab::EventStore::Subscriber
module and thathandle_event
method is defined. - Static analyzer to ensure we don't call workers from other namespaces but publish events instead.
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team