docs: [FC-0074] add ADR with event design suggestions #438
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by @openedx/hooks-extension-framework. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
mariajgrimaldi
changed the title
|
FYI @Squirrel18 @Alec4r |
mariajgrimaldi
changed the title
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| - Design events with a single responsibility in mind. Each event should represent a single action or fact that happened in the system. If an event contains multiple actions, consider splitting it into multiple events. For instance, if the course grade is updated to pass or fail, there should be two events: one for the pass action and another for the fail action. | ||
| - Manage the granularity of the event so it is not too coarse (generic with too much information) or too fine-grained (specific with too little information). When making a decision on the granularity of the event, start with the minimum required information for consumers to react to the event and add more information as needed with enough justification. If necessary, leverage API calls from the consumer side to retrieve additional information but always consider the trade-offs of adding dependencies with other services. |
There was a problem hiding this comment.
I agree with the idea of avoid generic events, however, we should also avoid split events like course_passed or course_failed when we could use just course_completed with an status.
could we include a practical example on an appropiate level of granularity?
There was a problem hiding this comment.
I'm interested in understanding why having a status is better. I think that in this case, it'd be easier to consume more granular events than leave the responsibility to the consumer to figure out whether the student passed or failed based on the status.
There was a problem hiding this comment.
I understand your perspective, and I think both approaches have valid use cases depending on the context, let me elaborate on why I suggested using course_completed with a status field and we can discuss further to find a balance
If the event represents a single conceptual action for example: completing a course, having one event like course_completed with a clear status passed, failed, etc. could simplify the producer's logic and reduce event proliferation and for consumers, interpreting the status field is relatively straightforward if it's well-documented and includes only a few well-defined values, however if different consumers need to handle passed and failed cases in significantly different ways, separate those events might reduce complexity for them but emitting separate events like course_passed and course_failed could also create challenges such as needing to ensure mutual exclusivty.
So in cases like this I think We could start with course_completed and a status field, ensuring it is well-documented and strictly validted and if in the future we observe a clear need for distinct event flows We could introduce the more granular events without breaking existing consumers.
There was a problem hiding this comment.
In the example we're using, I still think it'd be more straightforward to send smaller and more specific events. As I see it, course completion and grade passing would be two different critical facts happening in the system; therefore, they should be independent. This is more of a question of what consumers would want with a course completion event or a course passing status change.
In any case, these are the only suggestions that should be evaluated for each case. We currently have an event called COURSE_PASSING_STATUS_UPDATED which uses a similar flow control status field called is_passing. In that case, given that the status is directly related to the event, using a status field is acceptable. What do you think? Should we maybe use a rule of thumb indicating that an event can be split into more events, given that they could communicate more facts of the system?
There was a problem hiding this comment.
I added some examples illustrating the granularity I'm referring to: https://github.com/openedx/openedx-events/pull/438/files#diff-bdc081c0ccc9885672f08f8b2c853ca7ce8a068db2cb2497d11e04b19013e19aR88-R112
There was a problem hiding this comment.
@Alec4r I worry that your example is predicated on understanding the needs of the consumers. I don't think that's always possible, to predict how various consumers will need to consume events. I agree with @mariajgrimaldi that consuming an event that doesn't require me to also check the status field is conceptually simpler, even if it means I need to handle consuming a few discrete events.
| ~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| - When designing an event, consider the consumers that will be using it. What information do they need to react to the event? What data is necessary for them to process the event? | ||
| - Design events carefully from the start to minimize breaking changes for consumers, although it is not always possible to avoid breaking changes. |
There was a problem hiding this comment.
will it be necessary to version events to handle event changes, or what is the plan for handling event changes?
There was a problem hiding this comment.
Events are versioned by definition, see this ADR for more info. As for the evolution of the events schema, this other ADR describes what the behavior is supposed to be: https://github.com/openedx/openedx-events/blob/main/docs/decisions/0006-event-schema-serialization-and-evolution.rst#decision, although according to this comment the ADR might be outdated -- I'll be working on updating it. The reality is that we haven't needed to change an event definition in any way that's breaking.
There was a problem hiding this comment.
@robrap: Can you help us figure out what needs to change from the ADR-0006? I could do it, but I need more context to do it effectively.
There was a problem hiding this comment.
Hello @mariajgrimaldi. I was off for the last two weeks, so just getting back to things.
It seems like you are pretty up to date. I'll summarize what I think you are already saying.
- As noted in the comment you linked to, the existence of optional fields probably changes the 0006 recommendation. The first sentence of the deferred alternatives in the ADR reads: "We could add the ability to add optional fields to an event...". And we did add this ability.
- Most importantly, no events have actually gone through any process of evolution, so all of this is just best-guesses about what we'd like to do and how we think it would work. It was deemed too much effort to proceed any further without an actual need.
Does this help? Let me know if you have any specific questions.
There was a problem hiding this comment.
Yes, it helps! Thank you. So I'll go ahead and create a new ADR considering the full transitive evolution strategy, so at least it is documented somewhere, noting that we haven't gone through any process of evolution just yet.
I'll update the ADR in the coming weeks as a different effort. Thank you!
There was a problem hiding this comment.
The event design suggestions are pretty easy to understand, I think the main goal of this document is to provide clear information on how an event should be design and this document accomplishes that goal, even for someone who has no experience in this topic.
Co-authored-by: Sarina Canelake <[email protected]>
There was a problem hiding this comment.
Thanks @mariajgrimaldi. Looks good. I don't have any blocking comments. Just a variety of thoughts related to the docs in general.
This doc made me wonder about how-to vs adr when it comes to these docs. I don't think there is any perfect answer. I think the most important thing is helping people find this information when they need it.
Related:
- We don't have adr vs how-to for OEPs, so there, we sort of use the OEP type of "Architecture" vs "Best Practice", and even there it gets fuzzy which one to use at times.
- We used OEPs for cross-open-edx decisions for events before this repo existed. For example: https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0041-arch-async-server-event-messaging.html. I don't know if people would know that event related decisions from before a specific date are listed as OEPs, and after a certain date are dropped as ADRs in this repo. This is something you should consider as part of the FC. You could drop notes in the OEPs stating where all future event-related ADRs live, or you could migrate + note, etc.
| @@ -0,0 +1,127 @@ | |||
| 16. Event Design Practices | |||
There was a problem hiding this comment.
Nit: => Event Design Best Practices? Were you intentionally avoiding calling these "best practices"?
There was a problem hiding this comment.
I did, yes. This is mainly because these are opinionated suggestions that not all developers might agree with, although they aren't that controversial. So, I'm okay with calling them good practices, considering that we all agree with them.
| Responsibility and Granularity | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| - Design events with a single responsibility in mind. Each event should represent a single action or fact that happened in the system. If an event contains multiple actions, consider splitting it into multiple events. For instance, if the course grade is updated to pass or fail, there should be two events: one for the pass action and another for the fail action. |
There was a problem hiding this comment.
I know this ADR is about the event design itself, but somewhere you may want to consider adding a note like the following:
Events that are split across multiple actions is an exceptional case where the same event queue/topic should be used to help maintain order across these events.
There was a problem hiding this comment.
This note's pretty useful! Thanks. I'll make sure to add it right away.
I like the idea of adding a brief update to the OEP to direct people to look in this repo for ADRs. |
|
Thank you both, @sarina @robrap! I've created this PR to address your suggestions about updating the OEPs: openedx/open-edx-proposals#662 |
Description
This PR adds an ADR with suggestions on how to design a new event based on the Building Event-Driven Microservices 3rd chapter about Communication and Data Contracts and Martin Fowler's Event Driven article. I'm proposing that these practices be considered when implementing new events, but they should NOT be considered standards that all events should strictly follow. Also, I'm not saying either that all the existing events in the library comply (or should) with these practices.
This document will be used as a reference in the How to Create a New Event guide. I'm currently validating these items while I write the guide, making sure these suggestions are clear enough to follow. You can review it here: #439
Testing instructions
Review here: https://docsopenedxorg--438.org.readthedocs.build/projects/openedx-events/en/438/decisions/0016-event-design-practices.html
Deadline
None
Checklists
Check off if complete or not applicable:
Merge Checklist:
Post Merge:
finished.