Instrumented wrappers could break backward compatibility

[palkan]

Current RailsEventStore version: 0.36.0
Failing RailsEventStore version: 0.37.0+

I'm using custom mapper with additional API.
Tried to upgrade the gem version and found that everything is broken)

InstrumentedMapper only takes care of the basic mapper APIs assuming there is no additional methods. Which, I guess, true in most cases. Though it still looks too opinionated (and blocks me from upgrading 🙂).

I've been thinking about a ways to solve it:

1. make wrapper to be a delegate (e.g. via SimpleDelegator)
2. prepend instrumentation functionality (via mapper.prepend – involves singleton class creation, though not a big deal in this case)
3. let users use the exact mappers their provide to Client.new and add a separate InstrumentedClient.new with automatic wrapping (and add a module that could be included to custom mappers to seamlessly add instrumentation)

All the applied are also applied to instrumented dispatchers and repositories.

Would be glad to provide a PR when/if we decide on a way to solve this.

Thanks)

[mostlyobvious]

Hey, thanks for the report!

Let me better understand the issue:

1. InstrumentedMapper wraps any wrapper passed to RailsEventStore::Client
2. you're directly calling methods on mapper which are not the ones InstrumentedMapper responds to and InstrumentedMapper does not delegate calls to passed wrapper beyond the methods used by RES itself (event_to_serialized_record and serialized_record_to_event)

Is that correct?

[mostlyobvious]

3. let users use the exact mappers their provide to Client.new and add a separate InstrumentedClient.new with automatic wrapping (and add a module that could be included to custom mappers to seamlessly add instrumentation)

In fact RailsEventStore::Client is an opinionated wrapper on RubyEventStore::Client. Those two can be used almost interchangeably.

Almost, because RailsEventStore::Client adds one more method that is used by the middleware to collect request metadata.

module RailsEventStore
  class Client < RubyEventStore::Client
    attr_reader :request_metadata

    def initialize(repository: RailsEventStoreActiveRecord::EventRepository.new,
                   mapper: RubyEventStore::Mappers::Default.new,
                   subscriptions: RubyEventStore::PubSub::Subscriptions.new,
                   dispatcher: RubyEventStore::ComposedDispatcher.new(
                     RubyEventStore::ImmediateAsyncDispatcher.new(scheduler: ActiveJobScheduler.new),
                     RubyEventStore::PubSub::Dispatcher.new),
                   request_metadata: default_request_metadata)
      super(repository: RubyEventStore::InstrumentedRepository.new(repository, ActiveSupport::Notifications),
            mapper: RubyEventStore::Mappers::InstrumentedMapper.new(mapper, ActiveSupport::Notifications),
            subscriptions: subscriptions,
            dispatcher: RubyEventStore::InstrumentedDispatcher.new(dispatcher, ActiveSupport::Notifications)
            )
      @request_metadata = request_metadata
    end

    def with_request_metadata(env, &block)
      with_metadata(request_metadata.call(env)) do
        block.call
      end
    end

    private
    def default_request_metadata
      ->(env) do
        request = ActionDispatch::Request.new(env)
        {
          remote_ip:  request.remote_ip,
          request_id: request.uuid
        }
      end
    end
  end
end

As a short-term workaround I'd use MyRESClient < RubyEventStore::Client having with_request_metadata defined and changing mapper: to not include that instrumentation wrapper.

[mostlyobvious]

The direction we'd like to take in the future with mappers is more or less here: #400, which would be composing them from smaller bits:

CURRENT = Mapper.new(
  Default,
  SymbolizeMetadataKeys,
  Serializer.new,
)

JSON = Mapper.new(
  Default,
  SymbolizeMetadataKeys,
  Serializer.new(serializer: JSON),
)

I'd love to learn about your use case more :)

[palkan]

@pawelpacana

Is that correct?

Yep.

Almost, because RailsEventStore::Client adds one more method that is used by the middleware to collect request metadata.

Yeah, that was my first suggestion, but copy-pasting this single method doesn't seem to be a good idea(

[palkan]

I'd love to learn about your use case more :)

I'm using custom type -> class name mapping to not depend on Ruby classes names at all (so, I'm decoupling event types from classes).

So, I store this custom mapping within a mapper itself (thus I have a couple more method, e.g. register_event(type, class_name)).

The pros of not using class names:

• easier refactoring
• making it possible to "consume" events from other applications with their own event classes (which hasn't been implemented yet, but the idea is to re-broadcast some events using a remote broker).

[palkan]

The easiest solution for me would be storing the mapping somewhere else.
And it makes sense to somehow disallow users to extend mappers public API (at least by saying this in the docs).

Not sure the same would work for dispatchers and, especially, repositories (I think, having custom repository API could be useful).

[mostlyobvious]

I'm using custom type -> class name mapping to not depend on Ruby classes names at all (so, I'm decoupling event types from classes).

Btw. default mapper supports this already, provided it is given a map of type->class
https://railseventstore.org/docs/mapping_serialization/#available-mappers

      def serialized_record_to_event(record)
        event_type = events_class_remapping.fetch(record.event_type) { record.event_type }
        Object.const_get(event_type).new(
          event_id: record.event_id,
          metadata: TransformKeys.symbolize(serializer.load(record.metadata)),
          data:     serializer.load(record.data)
        )
      end

So, I store this custom mapping within a mapper itself (thus I have a couple more method, e.g. register_event(type, class_name)).

Which reminded me that the idea of registering events floated around #210 and #60.

The easiest solution for me would be storing the mapping somewhere else.

I guess it boils down to the fact the you already have an instance of RES client with its mapper instance and start registering this type -> class maps. Having a complete map before instantiating a mapper that uses such map could be one of the possibilities. Provided you don't really need to change that map at runtime.

[palkan]

Provided you don't really need to change that map at runtime.

One thing I forgot to mention: I do register events during the app initialization lazily, either when the event is first published or when a subscription is created.

[palkan]

Btw. default mapper supports this already, provided it is given a map of type->class

Yep, but I want to raise an explicit (and descriptive) exception instead of falling back to record.event_type.

def serialized_record_to_event(record)
  event_type = events_class_remapping.fetch(record.event_type) { record.event_type }
  # ... 
end

[mpraglowski]

Maybe the solution here could look like:

class AnyGivenMapper
  def initialize(event_builder: EventTypeToClass.new) # and some other dependencies as required
    @event_builder = event_builder
  end
  ...
  def serialized_record_to_event(record)
    event_builder(event_type).call(
      event_id: record.event_id,
      metadata: TransformKeys.symbolize(serializer.load(record.metadata)),
      data:     serializer.load(record.data)
    )
  end

event_builder should be an object that will provide another object implementing call method (lambda) that will build domain event with given attributes.

I will try to explore that idea in a PR :)

[mpraglowski]

Here start of the implementation #564
For now only for Default mapper but this should be embraced by all mappers.

[paneq]

@palkan could you show us the code? Where do you call the custom methods from? How do you access the mapper object to call those methods?

I assume it is possible to keep the reference to the inner mapper as a global constant and call these additional methods on it instead of on the wrapped, instrumented mapper. Any reason not to go this way?

Thank you for reporting the issue :)

[palkan]

Hi @paneq!
Check the code here: https://gist.github.com/palkan/644375279a037f1809d28a58b1abe390

I assume it is possible to keep the reference to the inner mapper as a global constant and call these additional methods on it instead of on the wrapped, instrumented mapper. Any reason not to go this way?

Yeah, it's possible. But as I already mentioned, Mapper is the simplest example.

What about dispatchers and repositories? Do you want to make their API un-extensible as well?

If so, it would be great to make it more clear for such developers as me that this entities should not have custom API methods.

[paneq]

@palkan Thank you, I will need to think about your answer :)

[swistak35]

@palkan I've made an instrumented repository to pass through any other custom methods, with the idea being that the instrumentation should be transparent to the repository being used.

I don't know whether you still need that, but feel free to check c23a973 :)