Skip to content

Commit

Permalink
Fix performance breakdown not being sent when a group has 0 value
Browse files Browse the repository at this point in the history
The backend rejects performance breakdown if one of the groups is 0 with this
message: `routes: sum <= 0`.

Not every route executes database requests, therefore, many routes won't have a
chance to send their `view` data.
  • Loading branch information
kyrylo committed Mar 12, 2019
1 parent fa48585 commit 0ccde3e
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 16 deletions.
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ Airbrake Changelog

* Fixed the Rails performance breakdown hook not maintaining performance
precision ([#936](https://github.com/airbrake/airbrake/pull/936))
* Fix Rails performance breakdown not being sent if one of the groups is zero
([#935](https://github.com/airbrake/airbrake/pull/935))

### [v8.3.1][v8.3.1] (March 11, 2019)

* Fixes `TypeError` in the `ActionControllerPerformanceBreakdownSubscriber` when
it tries to pass `nil` as a `db` or `view` value
([#932](https://github.com/airbrake/airbrake/pull/932)
([#932](https://github.com/airbrake/airbrake/pull/932))

### [v8.3.0][v8.3.0] (March 11, 2019)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,29 @@ def call(*args)
payload = event.payload

routes.each do |route, method|
next if (groups = build_groups(payload)).none?

Airbrake.notify_performance_breakdown(
method: method,
route: route,
response_type: payload[:format],
groups: {
db: payload[:db_runtime] || 0,
view: payload[:view_runtime] || 0
},
groups: groups,
start_time: event.time
)
end
end

def build_groups(payload)
groups = {}

db_runtime = payload[:db_runtime] || 0
groups[:db] = db_runtime if db_runtime > 0

view_runtime = payload[:view_runtime] || 0
groups[:view] = view_runtime if view_runtime > 0

groups
end
end
end
end
9 changes: 8 additions & 1 deletion spec/apps/rails/dummy_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ class DummyApp < Rails::Application
routes.append do
get '/' => 'dummy#index'
get '/crash' => 'dummy#crash'
get '/breakdown' => 'dummy#breakdown'
get '/notify_airbrake_helper' => 'dummy#notify_airbrake_helper'
get '/notify_airbrake_sync_helper' => 'dummy#notify_airbrake_sync_helper'
get '/active_record_after_commit' => 'dummy#active_record_after_commit'
Expand Down Expand Up @@ -100,7 +101,8 @@ class DummyController < ActionController::Base
'dummy/active_record_after_rollback.html.erb' => 'active_record_after_rollback',
'dummy/active_job.html.erb' => 'active_job',
'dummy/resque.html.erb' => 'resque',
'dummy/delayed_job.html.erb' => 'delayed_job'
'dummy/delayed_job.html.erb' => 'delayed_job',
'dummy/breakdown.html.erb' => 'breakdown'
)
]

Expand All @@ -111,6 +113,11 @@ def crash
raise AirbrakeTestError
end

def breakdown
Book.create(title: 'breakdown')
Book.all
end

def notify_airbrake_helper
notify_airbrake(AirbrakeTestError.new)
end
Expand Down
9 changes: 3 additions & 6 deletions spec/integration/rails/rails_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -296,17 +296,14 @@
it "sends performance breakdown info to Airbrake" do
expect(Airbrake).to receive(:notify_performance_breakdown).with(
hash_including(
route: '/crash(.:format)',
route: '/breakdown(.:format)',
method: 'GET',
response_type: :html,
groups: {
db: anything,
view: anything
}
groups: hash_including(db: an_instance_of(Float))
)
).at_least(:once)

get '/crash'
get '/breakdown'
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@
context "and when view_runtime is nil" do
before { event.payload[:view_runtime] = nil }

it "sets the view group runtime to 0" do
it "omits view_runtime" do
expect(Airbrake).to receive(:notify_performance_breakdown).with(
hash_including(
route: '/test-route',
method: 'GET',
response_type: :html,
groups: { db: 0.5, view: 0 }
groups: { db: 0.5 }
)
)
subject.call([])
Expand All @@ -71,17 +71,61 @@
context "and when db_runtime is nil" do
before { event.payload[:db_runtime] = nil }

it "sets the view group runtime to 0" do
it "omits db_runtime" do
expect(Airbrake).to receive(:notify_performance_breakdown).with(
hash_including(
route: '/test-route',
method: 'GET',
response_type: :html,
groups: { db: 0, view: 0.5 }
groups: { view: 0.5 }
)
)
subject.call([])
end
end

context "when db_runtime is zero" do
before { event.payload[:db_runtime] = 0 }

it "omits db_runtime" do
expect(Airbrake).to receive(:notify_performance_breakdown).with(
hash_including(
route: '/test-route',
method: 'GET',
response_type: :html,
groups: { view: 0.5 }
)
)
subject.call([])
end
end

context "when view_runtime is zero" do
before { event.payload[:view_runtime] = 0 }

it "omits view_runtime" do
expect(Airbrake).to receive(:notify_performance_breakdown).with(
hash_including(
route: '/test-route',
method: 'GET',
response_type: :html,
groups: { db: 0.5 }
)
)
subject.call([])
end
end

context "when db_runtime and view_runtime are both zero" do
before do
event.payload[:db_runtime] = 0
event.payload[:view_runtime] = 0
end

it "doesn't notify Airbrake" do
expect(Airbrake).not_to receive(:notify_performance_breakdown)
subject.call([])
end
end
end
end

0 comments on commit 0ccde3e

Please sign in to comment.