Skip to content
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

Merged
merged 4 commits into from Jun 7, 2019

Conversation

mpraglowski
Copy link
Member

Instruments load & store methods. Tracks aggregate class, stream and
for storing also current aggregate version & number of events stored.

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,
Copy link
Member

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
Copy link
Member

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,
Copy link
Member

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,
Copy link
Member

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/)

Copy link
Member

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
Copy link
Member

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.

Copy link
Member

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,
Copy link
Member

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?

Copy link
Member Author

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.

@mpraglowski mpraglowski merged commit 3ea6a15 into master Jun 7, 2019
@mpraglowski mpraglowski deleted the aggregate-root-repository-instrumentation branch June 7, 2019 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants