Skip to content

Commit

Permalink
Implement the "query_stats" option and disable it
Browse files Browse the repository at this point in the history
Likely fixes:

* airbrake/airbrake#994 (Memory usage)
* airbrake/airbrake#990 (CPU and Average Response
  increased after version 7)

The SQL query collection feature is still in alpha and we display it only for
certain accounts. Therefore, the vast majority of our customers cannot even see
what airbrake-ruby collects. Since the feature is still in alpha, it is
currently buggy (in a sense that it consumes too much memory & CPU power).

I profiled a Rails app on a certain route with `memory_profiler` for Rack Mini
Profiler and I can see a significant increase of memory usage.

=== performance_stats = false

```
Total allocated: 12434583 bytes (145711 objects)
Total retained:  508023 bytes (3346 objects)

allocated memory by gem

       264  airbrake/lib
```

=== performance_stats = true && query_stats = false

```
Total allocated: 12461685 bytes (146002 objects)
Total retained:  508431 bytes (3353 objects)

allocated memory by gem

      3816  airbrake/lib
```

=== performance_stats = true && query_stats = true

```
Total allocated: 186535107 bytes (1447037 objects)
Total retained:  35810972 bytes (349882 objects)

allocated memory by gem

    709720  airbrake/lib
```

According to these reports memory usage ramped up by 700% compared to the route
stats feature. The route stats feature itself is already quite expensive and the
query collection feature is just too much.
  • Loading branch information
kyrylo committed Aug 5, 2019
1 parent d41f4b1 commit 056ee63
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 25 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,13 @@ Airbrake Ruby Changelog

### master

* Added the `query_stats` option that configures SQL performance
monitoring. If `performance_stats` is `false`, setting this to `true` won't
have effect because `performance_stats` has higher precedence. It's also
disabled by default, and it's currently in alpha (works only for some
accounts). Enabling is not recommended for now.
([#491](https://github.com/airbrake/airbrake-ruby/pull/491))

### [v4.5.1][v4.5.1] (July 29, 2019)

* Improved performance of `PerformanceNotifier` (sic!)
Expand Down
22 changes: 20 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -358,20 +358,38 @@ end
#### performance_stats

Configures [Airbrake Performance Monitoring][airbrake-performance-monitoring]
statistics collection (routes, SQL queries). These are displayed on the
statistics collection aggregated per route. These are displayed on the
Performance tab of your project. By default, it's enabled.

The statistics is sent via:

* [`Airbrake.notify_request`](#airbrakenotify_request)
* [`Airbrake.notify_query`](#airbrakenotify_query)

```ruby
Airbrake.configure do |c|
c.performance_stats = true
end
```

#### query_stats

Configures [Airbrake Performance Monitoring][airbrake-performance-monitoring]
query collection. These are displayed on the Performance tab of your project. If
`performance_stats` is `false`, setting this to `true` won't have effect because
`performance_stats` has higher precedence. By default, it's disabled.
**WARNING**: this feature is currently in alpha and it's not available for all
accounts.

The statistics is sent via:

* [`Airbrake.notify_query`](#airbrakenotify_query)

```ruby
Airbrake.configure do |c|
c.query_stats = false
end
```

### Asynchronous Airbrake options

The options listed below apply to [`Airbrake.notify`](#airbrakenotify), they do
Expand Down
25 changes: 22 additions & 3 deletions lib/airbrake-ruby/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,18 +83,24 @@ class Config
# @since v2.5.0
attr_accessor :code_hunks

# @return [Boolean] true if the library should send performance stats
# information to Airbrake (routes, SQL queries), false otherwise
# @return [Boolean] true if the library should send route performance stats
# to Airbrake, false otherwise
# @api public
# @since v3.2.0
attr_accessor :performance_stats

# @return [Integer] how many seconds to wait before sending collected route
# stats
# @api public
# @api private
# @since v3.2.0
attr_accessor :performance_stats_flush_period

# @return [Boolean] true if the library should send SQL stats to Airbrake,
# false otherwise
# @api public
# @since v4.6.0
attr_accessor :query_stats

class << self
# @return [Config]
attr_writer :instance
Expand Down Expand Up @@ -132,6 +138,7 @@ def initialize(user_config = {})
self.versions = {}
self.performance_stats = true
self.performance_stats_flush_period = 15
self.query_stats = false

merge(user_config)
end
Expand Down Expand Up @@ -197,6 +204,18 @@ def check_configuration
check_notify_ability
end

def check_performance_options(resource)
promise = Airbrake::Promise.new

if !performance_stats
promise.reject("The Performance Stats feature is disabled")
elsif resource.is_a?(Airbrake::Query) && !query_stats
promise.reject("The Query Stats feature is disabled")
else
promise
end
end

private

def set_option(option, value)
Expand Down
6 changes: 2 additions & 4 deletions lib/airbrake-ruby/performance_notifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,8 @@ def notify(resource)
promise = @config.check_configuration
return promise if promise.rejected?

promise = Airbrake::Promise.new
unless @config.performance_stats
return promise.reject("The Performance Stats feature is disabled")
end
promise = @config.check_performance_options(resource)
return promise if promise.rejected?

@filter_chain.refine(resource)
return if resource.ignored?
Expand Down
45 changes: 45 additions & 0 deletions spec/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
its(:whitelist_keys) { is_expected.to be_empty }
its(:performance_stats) { is_expected.to eq(true) }
its(:performance_stats_flush_period) { is_expected.to eq(15) }
its(:query_stats) { is_expected.to eq(false) }

describe "#new" do
context "when user config is passed" do
Expand Down Expand Up @@ -106,4 +107,48 @@
its(:check_configuration) { is_expected.not_to be_rejected }
end
end

describe "#check_performance_options" do
it "returns a promise" do
resource = Airbrake::Query.new(
method: '', route: '', query: '', start_time: Time.now
)
expect(subject.check_performance_options(resource))
.to be_an(Airbrake::Promise)
end

context "when performance stats are disabled" do
before { subject.performance_stats = false }

let(:resource) do
Airbrake::Request.new(
method: 'GET', route: '/foo', status_code: 200, start_time: Time.new
)
end

it "returns a rejected promise" do
promise = subject.check_performance_options(resource)
expect(promise.value).to eq(
'error' => "The Performance Stats feature is disabled"
)
end
end

context "when query stats are disabled" do
before { subject.query_stats = false }

let(:resource) do
Airbrake::Query.new(
method: 'GET', route: '/foo', query: '', start_time: Time.new
)
end

it "returns a rejected promise" do
promise = subject.check_performance_options(resource)
expect(promise.value).to eq(
'error' => "The Query Stats feature is disabled"
)
end
end
end
end
18 changes: 2 additions & 16 deletions spec/performance_notifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@
project_id: 1,
project_key: 'banana',
performance_stats: true,
performance_stats_flush_period: 0
performance_stats_flush_period: 0,
query_stats: true
)
end

Expand Down Expand Up @@ -284,21 +285,6 @@
expect(promise.value).to eq('' => nil)
end

it "doesn't send route stats when performance stats are disabled" do
Airbrake::Config.instance.merge(performance_stats: false)

promise = subject.notify(
Airbrake::Request.new(
method: 'GET', route: '/foo', status_code: 200, start_time: Time.new
)
)

expect(a_request(:put, routes)).not_to have_been_made
expect(promise.value).to eq(
'error' => "The Performance Stats feature is disabled"
)
end

it "doesn't send route stats when current environment is ignored" do
Airbrake::Config.instance.merge(
performance_stats: true, environment: 'test', ignore_environments: %w[test]
Expand Down

0 comments on commit 056ee63

Please sign in to comment.