Add instrumentation for repositories #423

swistak35 · 2018-08-08T19:01:07Z

Issue: #244

mostlyobvious · 2018-08-08T19:47:08Z

Interesting, I thought that Client would be more appropriate than Repository. It might be a bit too low-level. And definitely not all methods there deserve instrumenting.

mostlyobvious · 2018-08-08T19:47:21Z

rails_event_store/lib/rails_event_store/instrumented_repository.rb

+      end
+    end
+
+    def has_event?(event_id)


From one standpoint, as long as this is repository interface, then it deserves instrumenting. On the other hand what's the value of instrumenting this particular one?

@pawelpacana check my PR comment, I have big doubt about that one too.

swistak35 · 2018-08-08T19:50:10Z

My take on instrumentation, learning on what was in #90. I've started with Robert's idea on instrumented repository, because I wanted something easily deliverable. I believe that higher-level instrumentation still can be interesting in cases different than benchmarking (logging maybe?), but that's some start.

Also, I was not sure about instrumenting all public methods of repository, i.e. has_event?(...) - I'm not sure how useful is that instrumentation, except some hardcore benchmarking :P But I guess that in the end, instrumentation is to allow others to play with their ideas how to use it, not only some predefined usecases.

What do you guys think?

mostlyobvious · 2018-08-08T19:52:15Z

rails_event_store/spec/instrumented_repository_spec.rb

+    let(:test_link_events_to_stream) { false }
+    let(:test_binary) { false }
+
+    it_behaves_like :event_repository, InstrumentedRepository


Issue: #244

mostlyobvious · 2018-08-08T20:07:07Z

rails_event_store/lib/rails_event_store/client.rb

@@ -8,7 +8,7 @@ def initialize(repository: RailsEventStoreActiveRecord::EventRepository.new,
                   dispatcher: ActiveJobDispatcher.new,
                   request_metadata: default_request_metadata,
                   page_size: PAGE_SIZE)
-      super(repository: repository,
+      super(repository: InstrumentedRepository.new(repository),


mostlyobvious · 2018-08-08T20:11:42Z

Regarding which are worth instrumenting -- from maintainer perspective it is better to add less and reconsider adding more in the future than removing published instruments.

swistak35 · 2018-08-08T20:13:12Z

@pawelpacana Which one would you leave? I tend to all except has_event? and last_stream_event.

mostlyobvious · 2018-08-08T20:16:23Z

@pawelpacana Which one would you leave? I tend to all except has_event? and last_stream_event.

Good call, lets drop those two for now.

These considered not necessary right now, maybe they will be added later. Issue: #244

paneq · 2018-08-09T05:33:40Z

rails_event_store/spec/instrumented_repository_spec.rb

+
+    let(:test_race_conditions_any)   { false }
+    let(:test_race_conditions_auto)  { false }
+    let(:test_expected_version_auto) { false }


why false ?

paneq · 2018-08-09T05:33:52Z

rails_event_store/spec/instrumented_repository_spec.rb

+    let(:test_race_conditions_any)   { false }
+    let(:test_race_conditions_auto)  { false }
+    let(:test_expected_version_auto) { false }
+    let(:test_link_events_to_stream) { false }


why false here as well ?

Dunno about these, I've initially skipped these values because they've required some additional methods to be defined. I will look at it.

It doesn't make sense probably to test race conditions but it does make sense to verify that linking events still works correctly, don't you think?

paneq · 2018-08-09T05:36:31Z

rails_event_store/lib/rails_event_store/instrumented_repository.rb

@@ -0,0 +1,48 @@
+module RailsEventStore
+  class InstrumentedRepository


I like this decorator :)

paneq · 2018-08-09T05:37:36Z

rails_event_store/spec/client_spec.rb

@@ -43,5 +43,18 @@ module RailsEventStore
      expect(published.first.metadata[:request_id]).to eq('dummy_id')
      expect(published.first.metadata[:timestamp]).to be_a Time
    end
+
+    specify 'wraps repository into instrumentation' do


paneq · 2018-08-09T05:46:58Z

The tests are super verbose...

      specify "instruments" do
        some_repository = double
        allow(some_repository).to receive(:append_to_stream)
        instrumented_repository = InstrumentedRepository.new(some_repository)
        notification_calls = subscribe_to("append_to_stream.repository.rails_event_store")
        event1 = Object.new
         instrumented_repository.append_to_stream([event1], "SomeStream", "c456845d-2b86-49c1-bdef-89e57b5d86b1")
         expect(notification_calls).to eq([
          { events: [event1], stream: "SomeStream" }
        ])
      end
    end

Allow me to offer some alternative:

let(:instrumented_repository) { InstrumentedRepository.new(InMemoryRepository.new) }
let(:event) { RubyEventStore::Event.new }
let!(:notification_calls) { subscribe_to(/repository.rails_event_store/) }

      specify "instruments append_to_stream" do
         instrumented_repository.append_to_stream([event], "SomeStream", "c456845d-2b86-49c1-bdef-89e57b5d86b1")
         expect(notification_calls).to eq([
          { events: [event], stream: "SomeStream" }
        ])
      end
    end

I believe these 3 lets can be easily reused between many specs. Definitely no need to use mocks in every spec when you can just decorate InMemoryRepo and have it working with no pain.

The only downside is that we don't verify exact notification name but only that it belongs to the namespace repository.rails_event_store. But even if you don't go with that the reduced verbosity of not using mocks could be worth dozens lines of tests less.

paneq · 2018-08-09T05:53:07Z

Interesting, I thought that Client would be more appropriate than Repository. It might be a bit too low-level. And definitely not all methods there deserve instrumenting.

Good point. I felt the same. But on the other hand... It should be relatively easy to write similar decorators for Broker and Mapper and then one should be able to pin-point exact performance problems to lower-level components and maybe monitoring Client would be lower prio. It does not do that much on itself (but it does a few things).

paneq · 2018-08-09T05:56:52Z

rails_event_store/spec/instrumented_repository_spec.rb

+      end
+    end
+
+    def subscribe_to(name)


As we never unsubscribe, doesn't it lead to memory leaks in all RailsEventStore tests?

Wouldn't these arrays continue receiving payloads for all tests (in this file and other files) which call methods on RailsEventStore::Clients. It might be better to verify with temporary subscriptions:

callback = lambda {|*args| ... } ActiveSupport::Notifications.subscribed(callback, /rails_event_store/) do ... end

let(:received) { [] } around do |spec| callback = lambda {|*args| received << args } ActiveSupport::Notifications.subscribed(callback, /rails_event_store/) do spec.call end end

or

let(:received) { [] } around do |spec| callback = lambda {|*args| received << args } ActiveSupport::Notifications.subscribed(callback, "#{spec.metadata[:subscription_name]}.repository.rails_event_store") do spec.call end end specify "instruments append_to_stream", subscription_name: "append_to_stream" do # ... end

swistak35 · 2018-08-09T06:58:40Z

@paneq mesa like use InMemoryRepository instead of mocks, but mesa no like too many lets :)

swistak35 · 2018-08-09T07:01:49Z

then one should be able to pin-point exact performance problems to lower-level components

As I've said earlier, I still think higher-level instrumentation can be valuable. As I understand it, instrumentation is not only about performance problems, but also about hooking in the right moment because of some particular usecase we can not think about. Where are you, AOP ;)

paneq · 2018-08-09T07:09:13Z

As I've said earlier, I still think higher-level instrumentation can be valuable.

That's unquestionable. It's rather a matter of priorities. Do we start with low-level instrumentation or higher-level.

As I understand it, instrumentation is not only about performance problems, but also about hooking in the right moment because of some particular usecase we can not think about. Where are you, AOP ;)

Interesting. I don't see it much that way because that's only instrumentation and not AOP :) You can't affect the code from within the subscribed code.

Issue: #423 (comment)

Issue: #244

Now, when it doesn't directly on ActiveSupport::Notifications, it can be in RubyEventStore. ActiveSupport::Notifications still is present in the tests, because it serves us as API standard. Issue: #244

paneq · 2018-08-09T08:54:52Z

@swistak35 any pland for documentation and RDoc?

swistak35 · 2018-08-09T08:56:17Z

@paneq yeah, I haven't started it because I expected that the implementation may change rapidly, I'd prefer to make documentation in separate PR after merge

paneq · 2018-08-09T09:10:13Z

good for me :)

mostlyobvious · 2018-08-09T16:42:42Z

Let's deal with brew cask issue on master.

swistak35 added 8 commits August 8, 2018 21:00

Add instrumentation for repositories on #append_to_stream

e68cbf7

Issue: #244

Add instrumentation to #link_to_stream in repositories

c467e9a

Issue: #244

Add instrumentation to #delete_stream in repositories

60555c3

Add instrumentation for #has_event? in repositories

62e741a

Issue: #244

Add instrumentation to #last_stream_event in repositories

3f190c3

Issue: #244

Add instrumentation for #read_event in repositories

77b803c

Issue: #244

Add instrumentation to #read in repositories

d26323e

Issue: #244

Ensure that instrumented repository still behaves like repository

6f54bea

Issue: #244

swistak35 requested review from mostlyobvious and paneq August 8, 2018 19:44

mostlyobvious reviewed Aug 8, 2018

View reviewed changes

Instrument a repository by default

51d9dda

Issue: #244

mostlyobvious reviewed Aug 8, 2018

View reviewed changes

Remove some instrumentations

1f7fc92

These considered not necessary right now, maybe they will be added later. Issue: #244

paneq reviewed Aug 9, 2018

View reviewed changes

swistak35 added 4 commits August 9, 2018 09:14

Enable additional linter tests

a870a38

Use temporary subscriptions to avoid memory leaks

d5ccda8

Issue: #423 (comment)

Make ActiveSupport::Notifications a dependency

111efbf

Issue: #244

Move instrumentation to RubyEventStore

1b0ecbe

Now, when it doesn't directly on ActiveSupport::Notifications, it can be in RubyEventStore. ActiveSupport::Notifications still is present in the tests, because it serves us as API standard. Issue: #244

mostlyobvious merged commit 50676f4 into master Aug 9, 2018

mostlyobvious deleted the rl/as-instrumentation branch August 9, 2018 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add instrumentation for repositories #423

Add instrumentation for repositories #423

swistak35 commented Aug 8, 2018

mostlyobvious commented Aug 8, 2018

mostlyobvious Aug 8, 2018

mostlyobvious Aug 8, 2018

swistak35 Aug 8, 2018

swistak35 commented Aug 8, 2018

mostlyobvious Aug 8, 2018

mostlyobvious Aug 8, 2018

mostlyobvious commented Aug 8, 2018

swistak35 commented Aug 8, 2018

mostlyobvious commented Aug 8, 2018

paneq Aug 9, 2018

paneq Aug 9, 2018

swistak35 Aug 9, 2018

paneq Aug 9, 2018

paneq Aug 9, 2018

paneq Aug 9, 2018

paneq commented Aug 9, 2018

paneq commented Aug 9, 2018

paneq Aug 9, 2018

paneq Aug 9, 2018

swistak35 commented Aug 9, 2018

swistak35 commented Aug 9, 2018

paneq commented Aug 9, 2018

paneq commented Aug 9, 2018

swistak35 commented Aug 9, 2018

paneq commented Aug 9, 2018

mostlyobvious commented Aug 9, 2018

		@@ -0,0 +1,48 @@
		module RailsEventStore
		class InstrumentedRepository

Add instrumentation for repositories #423

Add instrumentation for repositories #423

Conversation

swistak35 commented Aug 8, 2018

mostlyobvious commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swistak35 commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mostlyobvious commented Aug 8, 2018

swistak35 commented Aug 8, 2018

mostlyobvious commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paneq commented Aug 9, 2018

paneq commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swistak35 commented Aug 9, 2018

swistak35 commented Aug 9, 2018

paneq commented Aug 9, 2018

paneq commented Aug 9, 2018

swistak35 commented Aug 9, 2018

paneq commented Aug 9, 2018

mostlyobvious commented Aug 9, 2018