ADR-0007: Pilot activity bounded context - #35402
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request adds ADR-0007, documenting the pilot phase for extracting an Activity bounded context as part of Fleet's modular monolith transition, and updates the ADR index to include references to ADRs 0006 and 0007. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/Contributing/adr/0007-pilot-activity-bounded-context.md (1)
106-120: Add language identifier to fenced code block.The folder structure code block is missing a language specifier. Add
diffor leave blank if structure-only, but best practice is to specify the language for syntax highlighting.-``` +``` server/ └── activity/(Note: If representing a filesystem structure, you can also use
diffortextas the language identifier.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/Contributing/adr/0007-pilot-activity-bounded-context.md(1 hunks)docs/Contributing/adr/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/Contributing/adr/0007-pilot-activity-bounded-context.md
[style] ~52-~52: Consider replacing this word to strengthen your wording.
Context: ...s. This is a generic architectural term and does not refer to Go modules (the d...
(AND_THAT)
[style] ~332-~332: Consider using a different adjective in this context to strengthen your wording.
Context: ...lities extraction. Vulnerabilities is a good candidate for the next bounded context ...
(GOOD_ALTERNATIVE)
🪛 markdownlint-cli2 (0.18.1)
docs/Contributing/adr/0007-pilot-activity-bounded-context.md
106-106: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
docs/Contributing/adr/README.md (1)
46-47: ADR index entries follow existing conventions.Both new ADR entries are formatted correctly and consistent with existing list items. The entries are properly linked to their corresponding ADR files.
docs/Contributing/adr/0007-pilot-activity-bounded-context.md (11)
11-29: Context section is thorough and well-reasoned.Excellent use of industry examples (GitLab, Kubernetes) with concrete evidence. The distinction between this pilot (audit portion only) and the complete activity system is clear, and the trade-offs are transparently acknowledged. The learnings from ADR-0001 are well-integrated.
31-46: Design guidelines are clear and actionable.The bounded context design principles (database transactions, schema, communication) are well-articulated with explicit checkmarks and warnings. The constraint against cross-context transactions is particularly important and well-justified.
48-62: Glossary removes ambiguity and aids clarity.Defining "Modularization," "Package," "Bounded context," "Cross-cutting concern," "Vertical slice," "Ubiquitous language," and "Table prefix" upfront is excellent. This is especially helpful given the technical nature of the document and potential confusion with Go module terminology.
102-139: Folder structure rationale is sound and enables boundary enforcement.The three-level hierarchy (root
activity/,activity/service/,activity/mysql/) is well-motivated. The separation clearly enables architectural checks to prevent unauthorized imports of internal packages (service/andmysql/private to the context), which directly supports the bounded context goals.
140-163: Database naming strategy is pragmatic and extensible.The choice of table prefixes (
activity_past,activity_upcoming, etc.) with descriptive suffixes is clearer than alternatives and anticipates future expansion. Aligns well with GitLab's established pattern and industry standards.
165-176: Architecture enforcement plan is concrete and leverages existing patterns.Referencing
server/archtest(used by Android MDM) and proposingarch_test.goin the Activity context keeps tooling consistent and lowers adoption friction. The specific import restrictions are clear and enforceable.
182-217: Communication pattern clearly establishes Activity as a write-only cross-cutting concern.The Mermaid diagram and dependency description effectively communicate that other contexts write through
activity.NewActivity()without reading back. This asymmetric dependency is well-justified as an event-sourcing approach for audit trails.
219-295: Consequences section provides balanced analysis with explicit trade-offs.The benefits, drawbacks, and impact sections are realistic. Particularly strong is the transparent acknowledgment in the drawbacks (lines 247–253) that audit-only scope may be too small to justify a separate bounded context—this demonstrates clear-eyed thinking about the pilot's limitations and accepted risks.
299-352: Alternatives considered are thorough and well-reasoned.All three alternatives are presented with detailed pros, cons, and explicit reasoning for rejection:
- Alt 1 (status quo): Acknowledges short-term benefits but correctly identifies long-term risk.
- Alt 2 (larger context like vulnerabilities): Recognizes higher business value but explains why smaller pilot is more prudent.
- Alt 3 (shared datastore): Clearly articulates how it would undermine bounded context isolation and references GitLab's success with full-stack ownership.
This demonstrates architectural maturity.
367-377: Success criteria are specific and time-bounded.Seven measurable success criteria with a 2-week evaluation window provide clear checkpoint for decision-making (continue, refine, or pivot). The criteria cover both technical aspects (boundaries, migration, performance) and organizational factors (developer experience, documentation).
1-10: ADR metadata is complete and follows template.Status, Date, and heading are properly formatted. The "Proposed" status is appropriate for a new ADR under review.
There was a problem hiding this comment.
@getvictor thanks again for putting this together! +1 on starting with activity module. Is there a place where you can expand a bit on how this will integrate into our current Fleet service? It looks like @jahzielv's code is merged to main which does this by creating a new activity module in serve.go and injecting it into the Android service; is this the general pattern you're envisioning? We've talked about ways to streamline/automate this sort of dependency injection in the future, especially as the dependency graph grows, but I'm fine starting with a manual approach and iterating.
There was a problem hiding this comment.
Yeah there are some DI libraries for Go out there, but I think we're better off rolling any DI tooling on our own. Those libs tend to either
- Be hard to integrate into an existing codebase, or
- Hide a ton of the implementation details
If we write our own, we can iterate, integrate it slowly, and only build what we need when we need it.
There was a problem hiding this comment.
@sgress454 Yes, the activity module will have an interface that will be injected into the current Fleet service (and any other services that need it). We're not explicitly trying to improve DI in this ADR. But there is value in having a standard/cleaner DI approach that would make our integration tests easier.
|
|
||
| **Database transactions:** | ||
| - ✅ **Exclusive write ownership**: Each bounded context owns writing to its tables exclusively (no other context writes to them) | ||
| - ✅ **Reads allowed across contexts**: Other contexts can read via joins for queries, but this indicates coupling |
There was a problem hiding this comment.
this indicates coupling - I think we should avoid coupling between modules, even for read. If a module needs data from another context, it should ideally get it through that module’s public abstraction.
There was a problem hiding this comment.
Yes, normal reads should go through the bounded context interface. However, sometimes we have transactions that join other tables, so we're not explicitly preventing DB reads that join tables from other bounded contexts.
There was a problem hiding this comment.
Yeah I think for now, we have to live the world of "one big pot".
I think we should avoid those cross-boundaries JOINS if possible, but it's also going to be a while until we get to a place where they are not necessary.
mostlikelee
left a comment
mostlikelee
left a comment
There was a problem hiding this comment.
I think it's important to discuss how this approach will affect us organizationally. With each team owning multiple modules, we will undoubtedly need to collaborate more cross-team as a given feature will likely cross multiple modules. Further utilizing working groups may address this.
|
Some important context: we need this change because the nature of what we're building has changed. We're no longer building a read-only osquery wrapper: we're building a distributed system that manages very complex state and enables very complex actions to be taken across hundreds of thousands of devices. To build that system, we need an updated approach to our architecture. This change is critical because it will allow us to better serve customers and build the best device management product out there. |
I actually think the opposite. With bounded contexts, a bug fix or a story should actually be touching few (ideally 1) bounded context on average. If we have most of our PRs touching multiple bounded contexts, then this is a red flag. |
@getvictor I see what you're saying! I agree with Tim as well though: organizationally we have to agree to build the product with DDD and bounded contexts in order to maximize the impact of this decision. One tactical organizational step we can take: tightly defining each team's responsibilities. With a very detailed (always subject to change ofc!) list of domains for each team, we can then identify the red flags you call out. Right now it'd be very hard to do that, because organizationally we spread work of all kinds across all the teams. |
|
|
||
| ### Industry examples of modular monolith architecture | ||
|
|
||
| **GitLab** (Ruby on Rails, 2.2M+ LOC): Successfully transitioned from monolith to modular architecture. Their [modular monolith documentation](https://handbook.gitlab.com/handbook/engineering/architecture/design-documents/modular_monolith) outlines key motivations: avoiding microservices complexity, maintaining code approachability, and enabling better feature boundaries through DDD. As [GitLab CEO stated](https://about.gitlab.com/blog/why-were-sticking-with-ruby-on-rails/#monorails): "A well-structured, well-architected, highly modular program that runs as a single process." |
There was a problem hiding this comment.
Great structure, and GitLab's example of how to transition successfully and transparently.
|
@getvictor Thank you, as always, for the thoughtful ADR. I appreciate you continuing to drive this initiative forward and make it a reality. Great resource you found in GitLab's docs! Also very applicable to where we're at. @sgress454 @mostlikelee @jahzielv Thanks for your feedback. The ADR process yields better results when we consider it from many angles. I think what some comments are getting at is discussed in GitLab's docs:
Is that accurate? I think it's salient because if all teams are working on all features, they'll all be working in all domains, which would create a lot of coordination overhead. This separation is much more effective when specific teams own specific domains. Our goal going into 2026 is for that to be the case, which started with defining areas of expertise. We plan to extend that such that we're better planning our roadmap to the capacities of the individual teams. If more capacity is needed, we'll grow the product group's capacity that is responsible for that domain. If we want to benefit from those results, we need to invest in getting there incrementally and iteratively. Great call out from @jahzielv:
Since we already have the areas of expertise defined, we could expand it with more detail. A sheet would probably be the best place to work that out. As we modularize the service layer, we could introduce ownership of each service, with the goal of directing all future work in that service to that group. Of course getting there will be sticky and require more cross-team communication, but that's an investment worth making. Anyway just wanted to check in on this. I still need to review the ADR in depth, and I'd like to read through GitLab's docs as well. Planning to provide full review before EOD Wednesday. |
@lukeheath this was the biggest takeaway from that doc for me! I think it's 100% accurate and relates to where we are today. Our architectural decisions ultimately have to align with and support our workflows as a team and the product we want to build. |
lukeheath
left a comment
lukeheath
left a comment
There was a problem hiding this comment.
Some minor tweaks, but I agree with this ADR in theory. However, I'm unclear what kind of changes are necessary at the organization level to implement this change. For example:
- Who owns the new Activity bounded context?
- Does only that group touch that context, or can other product groups touch it if it's reviewed by the group that owns it?
- How do we decide and document which bounded contexts are owned by which groups? The answers to 1 and 2 above will inform this decision.
I'm not sure how much of that information needs to be captured in the ADR vs. elsewhere, but I'd like to know what changes are necessary to make this a reality before it's merged. To me, it looks like this:
- File, draft, and estimate an eng-initited story to make this change.
- Update the handbook to document established bounded contexts and who owns it. Are any process changes needed? If one group owns the context, how do we make sure any stories touching it get routed to them? Do any activity changes need to be spec'd as their own story?
- We update CODEOWNERS so any changes to an owned bounded context require review from the established DRIs for that. This could also serve as the document for who owns which bounded contexts.
Once we've clarified what operational changes are needed I'm supportive of approving and merging this ADR. I just want to make sure we know what it will take to get traction across the org.
Also, because @lucasmrod is back online tomorrow, I'd like to get his review and thoughts before merging. In order for this to be successful long-term, we'll need buy-in from the engineers working in the affected code so everyone is building towards the same goal. To that end, I want to make sure we're considering it from all angles and addressing any concerns.
|
|
||
| **Database schema:** | ||
| - ✅ **Single shared database**: All contexts share one MySQL database for operational simplicity (no separate databases per context) | ||
| - ⚠️ **Cross-context joins allowed but scrutinized**: Joins across bounded context tables are permitted for read operations (queries, reports, dashboards), but indicate coupling and should receive extra scrutiny during code review |
There was a problem hiding this comment.
How can we make sure this happens, and all engineers know? Update PR template?
There was a problem hiding this comment.
The simplest approach is to add it to PR template.
A more sophisticated approach would be to run a check in CI that looks for DB table names mentioned in the wrong bounded context. We could then, for example, maintain a list of allowed cross-context joins and require that an entry be added to that list if a new JOIN (or table mention) was added.
Co-authored-by: Luke Heath <luke@fleetdm.com>
I recommend the team that does this work to own the new Activity bounded context. (dogfooding)
Yes, per our values, anyone can touch the new bounded context. Ownership will be determined via handbook and/or CODEOWNERS. Yes, changes should be reviewed by the team that owns it.
Yes. This is our normal process.
Yes, handbooking the ownership is a good idea. In general, once engineers/product/managers are familiar with the bounded contexts that we have, it should be obvious (hopefully) to everyone which bounded context(s) the story belongs in. Just like it is fairly obvious which of today's product groups the story belongs to. Activity changes should NOT need their own story. The plan for this ADR is for the other contexts to own the activity details (what activities they source). The activity context should take an interface and support common functionality on all activities. That said, if we're adding brand new functionality/fields to activities, then it should be a new story or subtask.
Yes, this makes sense.
|
MagnusHJensen
left a comment
MagnusHJensen
left a comment
There was a problem hiding this comment.
Overall a very nice read, hoping this gets approved and excited to learn more.
|
|
||
| Following GitLab's bounded context pattern, each context owns its full stack including data access: | ||
|
|
||
| ```text |
There was a problem hiding this comment.
Just wanted to understand where would the Module/Bounded context service interface live?
Inside the bounded context package or at a shared top-level? I don't think it's super clear from the read.
There was a problem hiding this comment.
In the Go community, it is considered idiomatic to define the interface with the consumer. This means:
- consumers (other bounded contexts) only declare the methods they need
- different consumers can define different interfaces around a common concrete type based on their needs
I'd like to try following this pattern since it fits best with a modular architecture. Although in this case, there will be methods that everyone will want, so it makes sense to have those methods in a shared interface at the top level.
There was a problem hiding this comment.
If the pilot goes well, we'll create a handbook page for the modular monolith that documents accepted conventions similar to GitLab's (though not as exhaustive with our resources.)
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
LGTM. It is a low-risk pilot and worth doing it before taking the bounded context idea to the next level with bigger components/features (e.g. vulnerabilities).
Just a few thoughts:
- How about calling it what it is:
auditoractivity_audit(instead ofactivity_past). Other contexts cannot import server/activity/service or server/activity/mysql (private implementation: Usually the way to signal "private" in Go is to useinternaldirectories (server/activity/internal/*).- Database table prefix seems like a simple and clear idea for these bounded contexts.
- Re: https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/audit-logs.md. Currently the developer needs to add new activities to
ActivityDetailsList. We could take the chance to improve this moving forward (or to keep things simple, just put all these types into a separateautomatic_docpackage or so).
Although
@lucasmrod Great idea to use internal. I think we need more architectural boundaries/checks like this. The only issue is that it creates a need for a wiring layer at the activity bounded context, since serve.go cannot directly instantiate the datastore. But perhaps that's better. https://github.com/fleetdm/fleet/pull/36454/files#diff-d3f5d0102791ab40f30af4cc0144f07acf81a4c7343d0a01cc467a1a538ec0cd
Yes, makes sense. I think this is a bit outside the scope, but we can try to make this improvement. |
|
@lukeheath, assuming this ADR is approved, I have the first (of several) implementation PR up for review: #36454 |
@lukeheath one tactical organizational change I'd suggest would be to have a synchronous eng + product training on domain driven design. I think it'd be really helpful for us all to be on the same page on this and have a ubiquitous language around this approach to product development. |
|
Your modules approach is great for organizing service-layer code, and I think we should keep doing that. ADR-0007 is about something different: it's about proving we can extract a true bounded context with enforced boundaries. The immediate value isn't in the activity feature itself. It's in establishing the pattern and discovering what coupling we need to break. For example, ADR-0001 (Android service) started down this path but didn't fully decouple generic logic from domain logic. This ADR is about doing that separation properly so we have a template for larger extractions. Once we've done one bounded context end-to-end, we'll have a clearer path for extracting higher-value contexts. Identity (User, Session) is a candidate for the next extraction since it's a cross-cutting concern like activities, which makes it a natural fit for separation. Adding/separating modules inside a bounded context is complementary work. It's cleaning up the service layer, which makes future bounded context extraction easier. |
|
Thanks all, great updates. Will review ASAP this week. Looks like we have a good plan for implementing it. |
| **Database transactions:** | ||
| - ✅ **Exclusive write ownership**: Each bounded context owns writing to its tables exclusively (no other context writes to them) | ||
| - ✅ **Reads allowed across contexts**: Other contexts can read via joins for queries, but this indicates coupling | ||
| - ❌ **DB transactions DO NOT cross bounded context boundaries**: Each transaction must be scoped to a single context's tables to avoid distributed transaction complexity |
There was a problem hiding this comment.
Do we know what this would look like for other parts of the software? It feels like it would require radically changing our DB schema to allow for more potential inconsistency.
There was a problem hiding this comment.
Upcoming activities would be a good thought exercise given the expectations that code and the queue have around transactions that span the upcoming activities table + a number of other tables(software installs, script executions, VPP and possibly soon MDM commands)
There was a problem hiding this comment.
I agree that upcoming activities is a good thought exercise and I haven't thought about it in detail. It may be the most complex situation we have. The 2 potential bounded contexts on my radar as easier follow up ADRs are identity (Users) and vulnerabilities.
There was a problem hiding this comment.
it would require radically changing our DB schema
Let's not do this part until we hit Fleet 5.
| - ⚠️ **Cross-context joins allowed but scrutinized**: Joins across bounded context tables are permitted for read operations (queries, reports, dashboards), but indicate coupling and should receive extra scrutiny during code review | ||
|
|
||
| **Bounded context communication:** | ||
| - ✅ **Public service APIs only**: Bounded contexts expose public service layer methods (e.g., `ListActivities()`); internal implementation details and datastore methods remain private to the context |
There was a problem hiding this comment.
Do we need to have Product review this? Since they ultimately design our APIs we at least need them to be aware of this as it may need to inform certain API design decisions
There was a problem hiding this comment.
Currently, our handlers (controllers) are closely tied to the service. So, we are tying the API to our service calls, often a 1-for-1 match. With bounded contexts, some API calls may need to orchestrate calls to multiple services.
I was thinking that perhaps that orchestration logic should be in its own controller layer (above service). Just an idea now. I haven't tried it.
If that's the case, the API would be somewhat decoupled from service layer methods.
There was a problem hiding this comment.
Yep, any API changes (or changes to any interface) would need to go through product drafting. We'll be approaching implementation as an engineering-initiated story that's drafted and estimated, so we can hash out more specifics in that issue before it's implemented.
|
If we do find that DDD is a helpful tool for us and this POC is successful, what are next steps to incorporate DDD into our product development/drafting flow? |
If the pilot is successful, we'll likely engage a DDD coach/consultant. Someone with an outsider's perspective that can help educate us on DDD thinking, but also help us find our way through the codebase's hall of mirrors. @getvictor Really appreciate you being a driving force for this. The need for modularization is something we've all recognized for awhile, but it takes someone to invest energy into turning that sentiment into action. I'm approving this ADR as a pilot with the following agreement:
If the pilot is successful, we'll formulate another ADR to define record our decision to adopt it throughout the codebase, and define an implementation approach. |
lukeheath
left a comment
lukeheath
left a comment
There was a problem hiding this comment.
@getvictor Let's work on fleshing out the eng-initiated story on Monday 🙌
8 participants
See new ADR doc in this PR
Summary by CodeRabbit