Skip to content

Commit

Permalink
Remove the ability to configure multiple notifiers
Browse files Browse the repository at this point in the history
This feature starts getting in the way of future development of the library:

* We have to maintain a complex API to support different configurations for many
  notifiers (we have 3 now vs 1 when we were releasing this functionality). We
  want a simple API

* It's hard to pass the config object around. If a component that sits at the
  bottom layer of the library architecture suddenly needs to read some config
  value, we have to pass it through all the layers, from the top to the
  bottom. This was a compromise since the very beginning but back then it was
  easy to maintain it (although it always bugged me). We now rapidly add new
  features and this approach does not scale well - too much clutter

* Classes know more than they really need. As a result, testing becomes harder
  since we need to inject the config (sometimes with "correct" values).

An alternative way would be exposing the config through
`Airbrake.notifiers[:default][:notice].config.config_value`. This only adds to
the clutter since the API becomes more complex and less malleable.

On the other hand, this feature is likely not used by 90% of our customers. We
have a solid Rails workflow that never involves more than 1 notifier. If people
need another notifier, they can easily instantiate it directly through
`Airbrake::NoticeNotifier.new`.
  • Loading branch information
kyrylo committed Feb 21, 2019
1 parent 17d6ced commit cdd2fb4
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 105 deletions.
65 changes: 3 additions & 62 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,7 @@ Key features
* Awesome performance (check out our benchmarks)<sup>[[link](#running-benchmarks)]</sup>
* Asynchronous exception reporting<sup>[[link](#asynchronous-airbrake-options)]</sup>
* Promise support<sup>[[link](#promise)]</sup>
* Flexible configuration options (configure as many Airbrake notifers in one
application as you want)<sup>[[link](#configuration)]</sup>
* Flexible configuration options<sup>[[link](#configuration)]</sup>
* Support for proxying<sup>[[link](#proxy)]</sup>
* Support for environments<sup>[[link](#environment)]</sup>
* Filters support (filter out sensitive or unwanted data that shouldn't be sent)<sup>[[link](#airbrakeadd_filter)]</sup>
Expand Down Expand Up @@ -70,10 +69,8 @@ Invoke the following command from your terminal:
gem install airbrake-ruby
```

Examples
--------

### Basic example
Example
-------

This is the minimal example that you can use to test Airbrake Ruby with your
project.
Expand Down Expand Up @@ -111,65 +108,9 @@ puts "\nAnother ZeroDivisionError was sent to Airbrake, but this time synchronou
"See it at #{response['url']}"
```

### Creating a named notifier

A named notifier can co-exist with the default notifier. You can have as many
notifiers configured differently as you want.

```ruby
require 'airbrake-ruby'

# Configure first notifier for Project A.
Airbrake.configure(:project_a) do |c|
c.project_id = 105138
c.project_key = 'fd04e13d806a90f96614ad8e529b2822'
end

# Configure second notifier for Project B.
Airbrake.configure(:project_b) do |c|
c.project_id = 123
c.project_key = '321'
end

params = { time: Time.now }

# Send an exception to Project A.
Airbrake[:project_a].notify('Oops!', params)

# Send an exception to Project B.
Airbrake[:project_b].notify('Oops!', params)

# Wait for the notifiers to finish their work and make them inactive.
%i(project_a project_b).each { |notifier_name| Airbrake[notifier_name].close }
```

Configuration
-------------

Before using the library and its notifiers, you must configure them. In most
cases, it is sufficient to configure only one, default, notifier.

```ruby
Airbrake.configure do |c|
c.project_id = 105138
c.project_key = 'fd04e13d806a90f96614ad8e529b2822'
end
```

Many notifiers can co-exist at the same time. To configure a new notifier,
simply provide an argument for the `configure` method.

```ruby
Airbrake.configure(:my_notifier) do |c|
c.project_id = 105138
c.project_key = 'fd04e13d806a90f96614ad8e529b2822'
end
```

You cannot reconfigure already configured notifiers.

### Config options

#### project_id & project_key

You **must** set both `project_id` & `project_key`.
Expand Down
30 changes: 7 additions & 23 deletions lib/airbrake-ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -197,24 +197,14 @@ def notifiers
}
end

# Configures a new +notifier+ with the given name. If the name is not given,
# configures the default notifier.
# Configures the Airbrake notifier.
#
# @example Configuring the default notifier
# Airbrake.configure do |c|
# c.project_id = 113743
# c.project_key = 'fd04e13d806a90f96614ad8e529b2822'
# end
#
# @example Configuring a named notifier
# # Configure a new Airbrake instance and
# # assign +:my_other_project+ as its name.
# Airbrake.configure(:my_other_project) do |c|
# c.project_id = 224854
# c.project_key = '91ac5e4a37496026c6837f63276ed2b6'
# end
#
# @param [Symbol] notifier_name the name to be associated with the notifier
# @yield [config] The configuration object
# @yieldparam config [Airbrake::Config]
# @return [void]
Expand All @@ -224,24 +214,18 @@ def notifiers
# is missing (or both)
# @note There's no way to reconfigure a notifier
# @note There's no way to read config values outside of this library
def configure(notifier_name = :default)
def configure
yield config = Airbrake::Config.new

if @notice_notifiers.key?(notifier_name)
raise Airbrake::Error,
"the '#{notifier_name}' notifier was already configured"
if @notice_notifiers.key?(:default)
raise Airbrake::Error, 'Airbrake was already configured'
end

raise Airbrake::Error, config.validation_error_message unless config.valid?

# TODO: Kludge to avoid
# https://github.com/airbrake/airbrake-ruby/issues/406
# Stop passing perf_notifier to NoticeNotifier as soon as possible.
perf_notifier = PerformanceNotifier.new(config)
@performance_notifiers[notifier_name] = perf_notifier
@notice_notifiers[notifier_name] = NoticeNotifier.new(config, perf_notifier)

@deploy_notifiers[notifier_name] = DeployNotifier.new(config)
@performance_notifiers[:default] = PerformanceNotifier.new(config)
@notice_notifiers[:default] = NoticeNotifier.new(config)
@deploy_notifiers[:default] = DeployNotifier.new(config)
end

# @return [Boolean] true if the notifier was configured, false otherwise
Expand Down
3 changes: 1 addition & 2 deletions lib/airbrake-ruby/notice_notifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,12 @@ class NoticeNotifier
# notice_notifier = Airbrake::NoticeNotifier.new(config)
#
# @param [Airbrake::Config] config
def initialize(config, perf_notifier = nil)
def initialize(config)
@config = config
@context = {}
@filter_chain = FilterChain.new
@async_sender = AsyncSender.new(config)
@sync_sender = SyncSender.new(config)
@perf_notifier = perf_notifier

add_default_filters
end
Expand Down
20 changes: 2 additions & 18 deletions spec/airbrake_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,28 +36,12 @@
end.to yield_with_args(Airbrake::Config)
end

context "when invoked with a notice notifier name" do
it "sets notice notifier name to the provided name" do
described_class.configure(:test) { |c| c.merge(config_params) }
expect(described_class[:test]).to be_an(Airbrake::NoticeNotifier)
end
end

context "when invoked without a notifier name" do
it "defaults to the :default notifier name" do
described_class.configure { |c| c.merge(config_params) }
expect(described_class[:default]).to be_an(Airbrake::NoticeNotifier)
end
end

context "when invoked twice with the same notifier name" do
context "when invoked twice" do
it "raises Airbrake::Error" do
described_class.configure { |c| c.merge(config_params) }
expect do
described_class.configure { |c| c.merge(config_params) }
end.to raise_error(
Airbrake::Error, "the 'default' notifier was already configured"
)
end.to raise_error(Airbrake::Error, 'Airbrake was already configured')
end
end

Expand Down

0 comments on commit cdd2fb4

Please sign in to comment.