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

Add AS::Notifications instrumentation #73

Closed
wants to merge 350 commits into from
Closed

Conversation

mostlyobvious
Copy link
Member

Rails provides quite solid framework for logging/instrumentation. We
could use it to provide better insight into event handling. Popular
metric tools (Datadog, Librato) can already plug into that.
One could also imagine plugging log subscriber for debugging purpose.
http://api.rubyonrails.org/classes/ActiveSupport/Notifications.html
http://guides.rubyonrails.org/active_support_instrumentation.html#creating-custom-events

This commit adds new dispatcher, only for RailsEventStore and makes it a
default.

The way a new default was baked into EventBroker was driven primarily by
existing linters/shared_examples and mutant - likely to be refactored.
Good enough for now to start discussion about relevance of this change.

swistak35 and others added 30 commits April 30, 2015 22:34
* it didn't have tests
* wrong name
* bad place to set the time
* will be reintroduced when ready
…ause AR doesn't have to query before save."

This reverts commit aaa9b30.
@mostlyobvious
Copy link
Member Author

And it crashed mutant with spec/event_broker_spec.rb 🤔

Rails provides quite solid framework for logging/instrumentation. We
could use it to provide better insight into event handling. Popular
metric tools (Datadog, Librato) can already plug into that.
One could also imagine plugging log subscriber for debugging purpose.
http://api.rubyonrails.org/classes/ActiveSupport/Notifications.html
http://guides.rubyonrails.org/active_support_instrumentation.html#creating-custom-events

This commit adds new dispatcher, only for RailsEventStore and makes it a
default.

The way a new default was baked into EventBroker was driven primarily by
existing linters/shared_examples and mutant - likely to be refactored.
Good enough for now to start discussion about relevance of this change.
@mostlyobvious
Copy link
Member Author

Reworked and reduced surface of changes - just another dispatcher in this PR, with little help of just released dispatcher lint.

Changing default dispatcher could be another change, leaving intact now.

Discussion still welcome!

@mostlyobvious mostlyobvious changed the title Add AS::Notifications instrumentation to Dispatcher. Add AS::Notifications instrumentation Aug 21, 2017
@mostlyobvious mostlyobvious mentioned this pull request Aug 21, 2017
Still maintaining "describe" with class name so that mutant can match
specs to changed code.
Allow logging/monitoring calls to #publish_event via AS::Notifications.
There's no other dispatcher in RailsEventStore namespace actually to
name this one with specialized name.
@mostlyobvious
Copy link
Member Author

Note to myself: check if error in subscriber of AS::Notification can affect open transaction with publish_event

@@ -13,6 +13,11 @@ module RailsEventStore
expect(client.__send__("event_broker")).to be_a EventBroker
end

specify 'initialize proper dispatcher type' do
expect(EventBroker).to receive(:new).with(dispatcher: an_instance_of(Dispatcher))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks a bit useless. Is it needed because mutant complains otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly

@mlomnicki
Copy link
Member

it will be a super useful feature 👍
also the implementation looks good to me

Unfortunately AS::Notifications does not swallow errors raised in
subscribers and they're executed synchronously. An error in
instrumentation poses a risk to rollback domain changes and event
publication.
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