-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AggregateRoot's repository instrumentation #591
Conversation
Instruments `load` & `store` methods. Tracks aggregate class, stream and for storing also current aggregate version & number of events stored.
instrumentation.instrument("store.repository.aggregate_root", | ||
aggregate_class: aggregate.class, | ||
aggregate_version: aggregate.version, | ||
stored_events: aggregate.unpublished_events.size, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should pass just aggregate.unpublished_events
here. That would be in symmetry with append_to_stream.repository.rails_event_store
.
aggregate_class: aggregate.class, | ||
aggregate_version: aggregate.version, | ||
stored_events: aggregate.unpublished_events.size, | ||
stream_name: stream_name) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/stream_name/stream, again following append_to_stream.repository.rails_event_store
def store(aggregate, stream_name) | ||
instrumentation.instrument("store.repository.aggregate_root", | ||
aggregate_class: aggregate.class, | ||
aggregate_version: aggregate.version, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/aggregate_version/version/
|
||
def store(aggregate, stream_name) | ||
instrumentation.instrument("store.repository.aggregate_root", | ||
aggregate_class: aggregate.class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think passing the instance, labeled as aggregate would be fine (thus s/aggregate_class/aggregate/)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instrumentation is useful in logging context, being able to track particular instance is helping.
|
||
describe "#load" do | ||
specify "wraps around original implementation" do | ||
some_repository = spy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for spy to check if it still follows an API that is mocked? I'm always wary of such tests. I briefly remember https://github.com/psyho/bogus implementing this check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
expect(notification_calls).to eq([{ | ||
aggregate_class: Order, | ||
aggregate_version: -1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting question, are we more interested in version before or after storing events? Also what happens in context of instrumentation if we cannot store events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually repository gets a new instance of aggregate, so on load method call version does not matter. The version in store instrumentation is an aggregate version before store.
Instruments
load
&store
methods. Tracks aggregate class, stream andfor storing also current aggregate version & number of events stored.