Implement an internal events system

[dacook]

What we should change and why (this is tech debt)

I think an events system would make the system more robust and scalable. It brings multiple benefits, and be overall simpler than implementing some of those benefits ad-hoc in various parts of the system.

What is an events system? Whenever something happens that can trigger another part of the system, or external notifications, we can implement a Publish/Subscribe layer. I've not done much research at all yet, but this article (and the mentioned gem) look like a good way to go: https://blog.carbonfive.com/evented-rails-decoupling-complex-domains-in-rails-with-domain-events/

Context

While working on #9616 to introduce webhooks, I discovered a few ways that an internal events system could improve our system (see https://github.com/openfoodfoundation/openfoodnetwork/pull/9687#issuecomment-1432594596). I realised I was implementing some parts of this, but in domain-specific code, which was going to make the codebase bigger and harder to follow, so I opted not to do that, and wait until we can implement something like this.

Benefits / Requirements:

• Decouple logic
  • for example, consider one method calling two unrelated modules. If the first one fails, the second would not execute.
  • asynchronous: being able to run published events as scheduled jobs means the calling code doesn't get interrupted
• event deduplication (eg order_cycle.opened should only happen once, according to a key like "#{order_cycle.id}-#{order_cycle.opened_at}"). Although i'm not sure Wisper supports this.
• Debugging: decoupling could make it harder to debug issues, so we need to ensure good help is in place
  • what if you subscribe to an event and the code doesn't run like you expected?
  • What the subscribing code runs and fails, and you can't see what triggered it? One way to help could be to store the calling scope in the event metadata, and in the event of an exception it could be added to the bugsnag context

Impact and timeline

Some parts of the code that might be changed/enhanced:

1. OrderCycleClosingJob - Right now it's got a single purpose, but in the future we may need to trigger more than one result.
   • Can be decoupled from OrderCycleNotificationJob and run it async
   • Instead of recording completion (and preventing re-running) with processed_at, we could potentially deduplicate the event instead.
2. Soon-to-be-added OrderCycleOpenedJob will be similar
3. SubscriptionPlacementJob could be decoupled from OrderManagement::Subscriptions::Summarizer
4. Maybe webhooks could be more closely integrated with this, making it easy to subscribe to any internal events.

I'm not sure how long it would take to implement. I would start first where it's needed the most (perhaps webhooks).

[rioug]

Rails offer some sort of pub/sub system with Active Support Instrumentation , it's maybe something we can leverage as it supports adding custom events

[abdellani]

This looks interesting :)

if I can help anywhere let me know.

[lin-d-hop]

I'm late to the party, but this is a great proposal @dacook. I wonder how we can tag this onto some feature set.... :thinking:

[isidzukuri]

I will play the role of the opposition in this discussion :smile:

events (pub/sub) != decoupling. Reasons described in "Benefits / Requirements" are leading to hiding coupling behind events, but its still there. It would hide dependencies on other modules and will become harder to trace contracts between classes. Also complexity will grow because of new layer. Bunch of new classes added for maintenance. Writing tests for applications with events is a lot harder. Asynchronousity given by events leads to pain in debugging and understanding of applications. Its simpler to work with ordered stack of calls which you can read with your own eyes in one file, one by one.

for example, consider one method calling two unrelated modules

Its question of cohesion. Maybe its better to reorganize classes/modules? If they calling each other so maybe they are actually related.

for example, consider one method calling two unrelated modules. If the first one fails, the second would not execute.

it can be solved with rescue. If so than person who reads the code would know that some function is expected to fail and its ok, and how its handled

asynchronous: being able to run published events as scheduled jobs means the calling code doesn't get interrupted

isn't simple line of code to initialize background job the solution for the problem?

it harder to debug issues

Right! Harder! Way harder!

Occam's_razor #KISS #YAGNI