Don't raise when not having an event handler for all possible events

[sogamoso]

If we expect consumers using streamy to consume all types of events we're forcing them to do stuff like this https://github.com/cookpad/global-feeds/blob/604ed0bb94a9c5a46c22acf0557a3643126fe40f/app/consumers/application_consumer.rb#L10.

We should allow consumers to choose which topics they subscribe to without having to rescue Streamy::EventHandlerNotFoundError. What do you think?

[lewispb]

@sebasoga I disagree, a consumer should explicitly decide how to handle each type of event. Otherwise, they may miss events and never realise. If there was a typo in one of the event handlers we'd never know until we figured it didn't work. Right now raising an error means Kafka will pause consuming and resume when the error is resolved with a code deploy. That code deploy can decide to ignore the error or continue.

[sogamoso]

a consumer should explicitly decide how to handle each type of event.

I agree that every consumer should decide how to explicitly decide to handle events. The part I think it's a bad idea is forcing them to do it by raising an exception if they don't. This promotes a bad practice of using exceptions to control the flow of execution avoid creating empty handlers and that catch all approach might end up hiding different reasons why the error was raised.

I think a better approach would be to allow the consumer to explicitly state which type of events it cares about (or the opposite), by allowing something like this:

Streamy::Config.consume_events(:foo, :bar, :baz) # This means that any event not in the list will be ignored

If there was a typo in one of the event handlers we'd never know until we figured it didn't work.

We could still do this by raising the same error (Streamy::EventHandlerNotFoundError) when the consumer doesn't have a handler for every event they explicitly to consume. But not raise it because there's not a handler for every event available.

[lewispb]

Want to whiteboard this on Monday so we can see why raising an exception works really well for this?

Interesting discussion here

"I just encountered a serious error, and right now getting out of this control flow to prevent data corruption or other damage is more important than trying to continue onward."

Please read https://github.com/karafka/karafka/wiki/Error-handling-and-back-off-policy for details of why Karafka needs us to raise or rescue exceptions.

[sogamoso]

Sounds good! 👍

[sogamoso]

Had a very productive chat with @lewispb today about this. Here are some of the things we agreed about the current approach:

• 👍 Raising an exception for unknown messages types helps us catch bugs
• 👎 Raising an exception for irrelevant messages types forces us to create dummy (empty) handlers that just add noise

Following that line of thought, we believe that having a list of events the consumer knows about but decides to ignore would be a good solution. In other words, we should:

• Explicitly set a list of (type of) events we want to ignore
• Raise an exception only when:
  • Processing an event for which we don't have a handler, and
  • The event is not included in the list of ignored events

This is roughly how that could look like:

Streamy

# lib/streamy/message_processor.rb
def event_handler
  handler_class_name.safe_constantize || ignored_type_handler || raise(handler_not_found_error)
end

private

  def ignored_type_handler. # Returns a "dummy handler" similar to using the null object pattern
    IgnoredTypeHandler if Config.ignored_event_types.includes?(message[:type])
  end

Consumer app

# config/initializers/streamy.rb
Streamy::Config.ignored_event_types[:topic_name] = %i(type_1 type_2)

# app/consumers/application_consumer.rb
def consume
  params_batch.each do |message|
    perform_action_with_newrelic_trace(category: :task, name: message["type"]) do
      Rails.logger.debug("Handling #{message['type']}")
      Streamy::MessageProcessor.new(message).run
    end
  end
end

/cc @dannyd4315