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

Implement code hunks support #258

Merged
merged 2 commits into from
Oct 11, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/airbrake-ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
require 'airbrake-ruby/filters/thread_filter'
require 'airbrake-ruby/filter_chain'
require 'airbrake-ruby/notifier'
require 'airbrake-ruby/code_hunk'

##
# This module defines the Airbrake API. The user is meant to interact with
Expand Down
32 changes: 26 additions & 6 deletions lib/airbrake-ruby/backtrace.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ module Patterns
# @param [Exception] exception The exception, which contains a backtrace to
# parse
# @return [Array<Hash{Symbol=>String,Integer}>] the parsed backtrace
def self.parse(exception, logger)
def self.parse(config, exception)
return [] if exception.backtrace.nil? || exception.backtrace.none?

regexp = best_regexp_for(exception)
Expand All @@ -111,14 +111,14 @@ def self.parse(exception, logger)
frame = match_frame(regexp, stackframe)

unless frame
logger.error(
config.logger.error(
"can't parse '#{stackframe}' (please file an issue so we can fix " \
"it: https://github.com/airbrake/airbrake-ruby/issues/new)"
)
frame = { file: nil, line: nil, function: stackframe }
end

stack_frame(frame)
stack_frame(config, frame)
end
end

Expand Down Expand Up @@ -176,10 +176,15 @@ def execjs_exception?(exception)
end
# rubocop:enable Metrics/PerceivedComplexity, Metrics/CyclomaticComplexity

def stack_frame(match)
{ file: match[:file],
def stack_frame(config, match)
frame = {
file: match[:file],
line: (Integer(match[:line]) if match[:line]),
function: match[:function] }
function: match[:function]
}

read_code_hunk(config.logger, frame) if config.code_hunks
frame
end

def match_frame(regexp, stackframe)
Expand All @@ -188,6 +193,21 @@ def match_frame(regexp, stackframe)

Patterns::GENERIC.match(stackframe)
end

def read_code_hunk(logger, frame)
code_hunk = Airbrake::CodeHunk.new(frame[:file], frame[:line]).to_h
return unless code_hunk

unless code_hunk.key?(0)

Choose a reason for hiding this comment

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

I see 2 unless that check for code_hunk. I also don't understand what code_hunk.key?(0) does so it would be nice if you refactor this code or make it more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#to_h returns a hash of int => string. A file can never have a zero line. I thought the existence of it would mean there's an error.

First unless is when a file is empty. Second unless is when there's an error that we need to log.

Does that make sense?

Choose a reason for hiding this comment

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

It makes sense, but why not to throw an exception like any other Ruby code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be unfortunate to miss a notice just because we couldn't read a line of code. I think simply ignoring that error and moving on would be the best solution.

Choose a reason for hiding this comment

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

I don't suggest you throw that exception to users. You can catch and log it as usually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

frame[:code_hunk] = code_hunk

Choose a reason for hiding this comment

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

Let's rename to just code for brevity? Anyway in JSON it should be codeHunk, not code_hunk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

return
end

logger.error(
"#{LOG_LABEL} error while reading code hunk `#{file}:#{line}'. " \
"Reason: #{code_hunk[0]}"

Choose a reason for hiding this comment

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

Again, I don't understand why code_hunk[0] contains the reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Files can never have line 0.

)
end
end
end
end
51 changes: 51 additions & 0 deletions lib/airbrake-ruby/code_hunk.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
module Airbrake
##
# Represents a small hunk of code consisting of a base line and a couple lines
# around
class CodeHunk
##
# @return [Integer] the maximum length of a line
MAX_LINE_LEN = 200

##
# @return [Integer] how many lines should be read around the base line
INTERVAL = 3

Choose a reason for hiding this comment

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

NLINES would be a better name. Also I suggest to lower this to 2 so we have 2 lines before and 2 lines after.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


def initialize(file, line, interval = INTERVAL)
@file = file
@line = line

@start_line = [line - interval, 1].max
@end_line = line + interval

@code_hash = {}
Copy link

@vmihailenco vmihailenco Oct 10, 2017

Choose a reason for hiding this comment

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

code_hash -> lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

end

##
# @return [Hash{Integer=>String}, nil] code hunk around the base line. When
# an error occurrs, returns a zero key Hash
def to_h
return @code_hash unless @code_hash.empty?
return unless File.exist?(@file)

begin
fetch_code

Choose a reason for hiding this comment

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

What I see is that you check lines, then for some reason call fetch_code and then check lines again. It would be much more obvious if code was:

check @lines
set @lines
check @lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, checking lines on line 29 is not needed. It's just prevention of reading the same file twice (for the same results). I removed it for now.

rescue StandardError => ex

Choose a reason for hiding this comment

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

This is an example you asked for.

{ 0 => ex }
end

@code_hash
end

private

def fetch_code

Choose a reason for hiding this comment

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

This actually reads somethings, but it is called fetch and returns nil too.

File.foreach(@file).with_index(1) do |line, i|
next if i < @start_line
break if i > @end_line

@code_hash[i] = line[0...MAX_LINE_LEN].rstrip
end
end
end
end
7 changes: 7 additions & 0 deletions lib/airbrake-ruby/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ class Config
# @since 1.2.0
attr_accessor :whitelist_keys

##
# @return [Boolean] true if the library should attach code hunks to each
# frame in a backtrace, false otherwise
# @since v3.0.0
attr_accessor :code_hunks

##
# @param [Hash{Symbol=>Object}] user_config the hash to be used to build the
# config
Expand All @@ -81,6 +87,7 @@ def initialize(user_config = {})
self.proxy = {}
self.queue_size = 100
self.workers = 1
self.code_hunks = false

self.logger = Logger.new(STDOUT)
logger.level = Logger::WARN
Expand Down
6 changes: 3 additions & 3 deletions lib/airbrake-ruby/nested_exception.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,16 @@ class NestedException
# can unwrap. Exceptions that have a longer cause chain will be ignored
MAX_NESTED_EXCEPTIONS = 3

def initialize(exception, logger)
def initialize(config, exception)
@config = config
@exception = exception
@logger = logger
end

def as_json
unwind_exceptions.map do |exception|
{ type: exception.class.name,
message: exception.message,
backtrace: Backtrace.parse(exception, @logger) }
backtrace: Backtrace.parse(@config, exception) }
end
end

Expand Down
2 changes: 1 addition & 1 deletion lib/airbrake-ruby/notice.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def initialize(config, exception, params = {})
@config = config

@payload = {
errors: NestedException.new(exception, @config.logger).as_json,
errors: NestedException.new(config, exception).as_json,
context: context,
environment: {
program_name: $PROGRAM_NAME
Expand Down
87 changes: 54 additions & 33 deletions spec/backtrace_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
require 'spec_helper'

RSpec.describe Airbrake::Backtrace do
let(:config) do
Airbrake::Config.new.tap { |c| c.logger = Logger.new('/dev/null') }
end

describe ".parse" do
context "UNIX backtrace" do
let(:parsed_backtrace) do
Expand All @@ -23,7 +27,7 @@

it "returns a properly formatted array of hashes" do
expect(
described_class.parse(AirbrakeTestError.new, Logger.new('/dev/null'))
described_class.parse(config, AirbrakeTestError.new)
).to eq(parsed_backtrace)
end
end
Expand All @@ -44,9 +48,7 @@
end

it "returns a properly formatted array of hashes" do
expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end

Expand All @@ -71,7 +73,7 @@
allow(described_class).to receive(:java_exception?).and_return(true)

expect(
described_class.parse(JavaAirbrakeTestError.new, Logger.new('/dev/null'))
described_class.parse(config, JavaAirbrakeTestError.new)
).to eq(backtrace_array)
end
end
Expand Down Expand Up @@ -99,10 +101,7 @@

it "returns a properly formatted array of hashes" do
allow(described_class).to receive(:java_exception?).and_return(true)

expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end

Expand All @@ -126,9 +125,7 @@
let(:ex) { AirbrakeTestError.new.tap { |e| e.set_backtrace(backtrace) } }

it "returns a properly formatted array of hashes" do
expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end

Expand All @@ -153,9 +150,7 @@
end

it "returns a properly formatted array of hashes" do
expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end

Expand All @@ -173,9 +168,7 @@
end

it "returns a properly formatted array of hashes" do
expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end
end
Expand All @@ -187,15 +180,15 @@

it "returns array of hashes where each unknown frame is marked as 'function'" do
expect(
described_class.parse(ex, Logger.new('/dev/null'))
described_class.parse(config, ex)
).to eq([file: nil, line: nil, function: 'a b c 1 23 321 .rb'])
end

it "logs unknown frames as errors" do
out = StringIO.new
logger = Logger.new(out)
config.logger = Logger.new(out)

expect { described_class.parse(ex, logger) }.
expect { described_class.parse(config, ex) }.
to change { out.string }.
from('').
to(/ERROR -- : can't parse 'a b c 1 23 321 .rb'/)
Expand All @@ -216,9 +209,7 @@
end

it "returns a properly formatted array of hashes" do
expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end

Expand All @@ -241,9 +232,7 @@

it "returns a properly formatted array of hashes" do
stub_const('OCIError', AirbrakeTestError)
expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end

Expand Down Expand Up @@ -279,9 +268,7 @@
stub_const('ExecJS::RuntimeError', AirbrakeTestError)
stub_const('Airbrake::RUBY_20', false)

expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end

Expand All @@ -296,12 +283,46 @@
stub_const('ExecJS::RuntimeError', NameError)
stub_const('Airbrake::RUBY_20', true)

expect(
described_class.parse(ex, Logger.new('/dev/null'))
).to eq(parsed_backtrace)
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end
end
end

context "when code hunks are enabled" do
let(:config) do
config = Airbrake::Config.new
config.logger = Logger.new('/dev/null')
config.code_hunks = true
config
end

let(:parsed_backtrace) do
[
{
file: File.join(fixture_path('code.rb')),
line: 94,
function: 'to_json',
code_hunk: {
91 => ' def to_json',
92 => ' loop do',
93 => ' begin',
94 => ' json = @payload.to_json',
95 => ' rescue *JSON_EXCEPTIONS => ex',
# rubocop:disable Metrics/LineLength,Lint/InterpolationCheck

Choose a reason for hiding this comment

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

Lint/UnneededDisable: Unnecessary disabling of Lint/InterpolationCheck (did you mean Lint/LiteralInCondition?).

Choose a reason for hiding this comment

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

Lint/UnneededDisable: Unnecessary disabling of Lint/InterpolationCheck (did you mean Lint/LiteralInCondition?), Metrics/LineLength.

96 => ' @config.logger.debug("#{LOG_LABEL} `notice.to_json` failed: #{ex.class}: #{ex}")',
# rubocop:enable Metrics/LineLength,Lint/InterpolationCheck
97 => ' else'
}
}

Choose a reason for hiding this comment

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

unexpected token tRCURLY

]
end

it "attaches code to each frame" do
ex = RuntimeError.new
ex.set_backtrace([File.join(fixture_path('code.rb') + ":94:in `to_json'")])
expect(described_class.parse(config, ex)).to eq(parsed_backtrace)
end
end
end
end
Loading