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

Allow to override config #519

Merged
merged 8 commits into from
Sep 15, 2016
Merged

Allow to override config #519

merged 8 commits into from
Sep 15, 2016

Conversation

jondeandres
Copy link
Contributor

@jondeandres jondeandres commented Sep 6, 2016

Many users ask how to override the configuration for a specific block of
code. This PR adds that functionality through the method
Rollbar.with_config, that delegates to the per-thread
Rollbar::Notifier instance.

The argument passed to #with_config is a Hash having key names equal
to the allowed configuration options and the new value to use for the
given block of code.

Example:

Rollar.configure do |config|
  config.environment = 'production'
end

Rollbar.with_config(environment: 'staging') do
  begin
    # do something that will crash
  rescue => e
    Rollbar.error(e)
  end
end

In the example from above, the reported error will use staging as
environment instead of production.

Jon de Andres added 5 commits September 5, 2016 16:09
In order to make the gem code viable for the "temporaly override
notifier configuration" feature, we needed to do few big changes in the
behavior.

Before this commit, Rollbar.configuration is an object that doesn't
mutate (at first), and its values are propagated to the notifier,
Rollbar.notifier.

This presents some problems if we want to override the config per
notifier at certain moment, since we are using Rollbar.configuration at
some places, so, it's a global value that is not part of the notifier.

With this commit, Rollbar.configuration points to
Rollbar.notifier.configuration, so we don't have a global configuration
without owner and we always use the current thread notifier's
configuration.

Other changes:

- Rollbar.reset_notifier! doesn't clear the notifier but clears the
  scope object for the notifier. The notifier is the owner of the
  configuration, so we cannot drop it. But we ensure that the scope is
  fresh when using this method. This is the current behavior that the
  middleware expect for this method
- Rollbar.configure, Rollbar.reconfigure and Rollbar.preconfigure
  delegate to Rollbar.notifier.

At the end, we ensure that Rollbar.configuration is *always* the
configuration for the notifier being used by the current thread. Having
this scenario, we are not able to create new temporaly notifiers with
new configuration and return to the previous one when a block code is executed
Many users ask how to override the configuration for a specific block of
code. This PR adds that functionality through the method
`Rollbar.with_config`, that delegates to the per-thread
Rollbar::Notifier instance.

The argument passed to `#with_config` is a `Hash` having key names equal
to the allowed configuration options and the new value to use for the
given block of code.

Example:

```ruby
Rollar.configure do |config|
  config.environment = 'production'
end

Rollbar.with_config(environment: 'staging') do
  begin
    # do something that will crash
  rescue => e
    Rollbar.error(e)
  end
end
```

In the example from above, the reported error will use `staging` as
environment instead of `production`.
Now .scoped() receives two optional arguments, the scope overrides and
the config overrides. So now we create a new instance inside the given
code block

Rollbar.scoped! will also allow passing configuration overrides, but be
careful with them, since it mutates the configuration.
new_notifier = subject.scope(new_scope, new_config)

expect(new_notifier).not_to be(subject)
expect(new_notifier.configuration.environment).to be_eql('foo')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also make sure that the parent notifier's configuration has not been changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, done.

@brianr
Copy link
Member

brianr commented Sep 14, 2016

Looks great 👍

@jondeandres jondeandres merged commit 8fef1ca into master Sep 15, 2016
@jondeandres jondeandres deleted the override-config branch September 15, 2016 14:16
jondeandres pushed a commit that referenced this pull request Sep 15, 2016
Since Rollbar.configuration points to the current thread's configuration
and Rollbar.notifier was just instantiating Rollbar::Notifier without
parent, the new threads' notifier didn't inherit the configuration

In order to maintain the correct behavior we add the concept of
`parent_notifier`, which is the first one instantiated when calling any
of: `Rollbar.configure`, `Rollbar.preconfigure`, `Rollbar.reconfigure`
or `Rollbar.unconfigure`.

This keeps the previous behavior before merging
#519, since those methods
worked on the global configuration `Rollbar.configuration`.
jondeandres pushed a commit that referenced this pull request Sep 15, 2016
Since Rollbar.configuration points to the current thread's configuration
and Rollbar.notifier was just instantiating Rollbar::Notifier without
parent, the new threads' notifier didn't inherit the configuration

In order to maintain the correct behavior we add the concept of
`parent_notifier`, which is the first one instantiated when calling any
of: `Rollbar.configure`, `Rollbar.preconfigure`, `Rollbar.reconfigure`
or `Rollbar.unconfigure`.

This keeps the previous behavior before merging
#519, since those methods
worked on the global configuration `Rollbar.configuration`.
jondeandres pushed a commit that referenced this pull request Sep 15, 2016
Since Rollbar.configuration points to the current thread's configuration
and Rollbar.notifier was just instantiating Rollbar::Notifier without
parent, the new threads' notifier didn't inherit the configuration

In order to maintain the correct behavior we add the concept of
`parent_notifier`, which is the first one instantiated when calling any
of: `Rollbar.configure`, `Rollbar.preconfigure`, `Rollbar.reconfigure`
or `Rollbar.unconfigure`.

This keeps the previous behavior before merging
#519, since those methods
worked on the global configuration `Rollbar.configuration`.
jondeandres pushed a commit that referenced this pull request Sep 15, 2016
Features:
- Allow to override config. See [#519](#519).
- Send code and context frame data. See [#523](#523).
- Send GET, POST and raw body in their correct place. See [#522](#522).
- Increase max payload from 128kb to 512kb. See [#521](#521).
- Add resque-rollbar functionality to the gem. See [#516](#516).
- Send custom.orig_host and custom.orig_uuid on too large payloads. See [#518](#518).
- Add Content-Length and Content-Type headers to the reports. See [#513](#513).

Bug fixes:
- SecureHeaders fixes. See [#478](#478).
- Include validations plugin in activerecord base. See [#503](#503).
- Require tempfile and use ::Tempfile. See [#514](#514).
- Extract correct client IP from X-Forwarded-For header. See [#515](#515).
- Delayed job fix on job serialization. See [#512](#512).

Others:
- Fix tests on rails40 and ruby 1.8.7. See [#485](#485).
- Move log methods to public section. See [#498](#498).
- Change rails50.gemfile to use Rails 5.0.0. See [#495](#495).
- Update CHANGELOG.md to fix incorrect links. See [#502](#502).
- Improve Rake support to avoid conflicts with other services. See [#517](#517).
- Make Codeclimate happier with Rollbar::Middlware::Js. See [#520](#520).
@jondeandres jondeandres mentioned this pull request Sep 15, 2016
jondeandres pushed a commit that referenced this pull request Sep 15, 2016
[skip ci]

Features:
- Allow to override config. See [#519](#519).
- Send code and context frame data. See [#523](#523).
- Send GET, POST and raw body in their correct place. See [#522](#522).
- Increase max payload from 128kb to 512kb. See [#521](#521).
- Add resque-rollbar functionality to the gem. See [#516](#516).
- Send custom.orig_host and custom.orig_uuid on too large payloads. See [#518](#518).
- Add Content-Length and Content-Type headers to the reports. See [#513](#513).

Bug fixes:
- SecureHeaders fixes. See [#478](#478).
- Include validations plugin in activerecord base. See [#503](#503).
- Require tempfile and use ::Tempfile. See [#514](#514).
- Extract correct client IP from X-Forwarded-For header. See [#515](#515).
- Delayed job fix on job serialization. See [#512](#512).

Others:
- Fix tests on rails40 and ruby 1.8.7. See [#485](#485).
- Move log methods to public section. See [#498](#498).
- Change rails50.gemfile to use Rails 5.0.0. See [#495](#495).
- Update CHANGELOG.md to fix incorrect links. See [#502](#502).
- Improve Rake support to avoid conflicts with other services. See [#517](#517).
- Make Codeclimate happier with Rollbar::Middlware::Js. See [#520](#520).
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.

3 participants