Implement code hunks support #258

kyrylo · 2017-10-10T00:51:13Z

This feature is experimental. I am not documenting it in the README because
users shouldn't be using it yet.

houndci-bot · 2017-10-10T00:51:24Z

spec/fixtures/long_line.rb

@@ -0,0 +1 @@
+loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong line


Metrics/LineLength: Line is too long. [204/90] (https://github.com/bbatsov/ruby-style-guide#80-character-limits)

houndci-bot · 2017-10-10T12:10:19Z

spec/backtrace_spec.rb

+              93 => '        begin',
+              94 => '          json = @payload.to_json',
+              95 => '        rescue *JSON_EXCEPTIONS => ex',
+              # rubocop:disable Metrics/LineLength,Lint/InterpolationCheck


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

houndci-bot · 2017-10-10T12:15:09Z

spec/backtrace_spec.rb

+              # rubocop:enable Metrics/LineLength,Lint/InterpolationCheck
+              97 => '        else'
+            }
+          }


unexpected token tRCURLY

houndci-bot · 2017-10-10T12:15:09Z

spec/backtrace_spec.rb

+              93 => '        begin',
+              94 => '          json = @payload.to_json',
+              95 => '        rescue *JSON_EXCEPTIONS => ex',
+              # rubocop:disable Metrics/LineLength,Lint/InterpolationCheck


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

This feature is experimental. I am not documenting it in the README because users shouldn't be using it yet.

vmihailenco · 2017-10-10T13:04:34Z

lib/airbrake-ruby/backtrace.rb

+        return unless code_hunk
+
+        unless code_hunk.key?(0)
+          frame[:code_hunk] = code_hunk


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

vmihailenco · 2017-10-10T13:06:42Z

lib/airbrake-ruby/backtrace.rb

+        code_hunk = Airbrake::CodeHunk.new(frame[:file], frame[:line]).to_h
+        return unless code_hunk
+
+        unless code_hunk.key?(0)


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.

#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?

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

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.

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

vmihailenco · 2017-10-10T13:07:16Z

lib/airbrake-ruby/backtrace.rb

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


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

Files can never have line 0.

vmihailenco · 2017-10-10T13:08:44Z

lib/airbrake-ruby/code_hunk.rb

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


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

vmihailenco · 2017-10-10T13:11:48Z

spec/code_hunk_spec.rb

+      subject { described_class.new(fixture_path('empty_file.rb'), 1).to_h }
+
+      it { is_expected.to eql({}) }
+    end


For empty file I expect to see {1: ""}

vmihailenco · 2017-10-10T13:12:21Z

spec/code_hunk_spec.rb

+      subject { described_class.new(fixture_path('banana.rb'), 1).to_h }
+
+      it { is_expected.to be_nil }
+    end


Why not to throw an exception (return error)?

What would it buy us?

AFAIK that is how errors are returned in Ruby.

If you have examples, I'd look at them. I don't think that's how it works. Either way, it's not a big deal because in real world backtraces would unlikely point to non existing files.

vmihailenco · 2017-10-10T13:13:45Z

spec/code_hunk_spec.rb

+      end
+    end
+
+    context "when a file has less lines than a code hunk requests" do


Would be nice if you follow previous context and say something like less than INTERVAL lines before and after

vmihailenco · 2017-10-10T13:14:10Z

spec/code_hunk_spec.rb

+      end
+    end
+
+    context "when a line location is in the middle of a file" do


Looks like this should has enough lines before and after

vmihailenco · 2017-10-10T13:16:11Z

lib/airbrake-ruby/code_hunk.rb

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


code_hash -> lines

houndci-bot · 2017-10-10T13:52:10Z

spec/code_hunk_spec.rb

+            99 => '        end',
+            100 => '',
+            101 => '        break if truncate == 0',
+            102 => '      end',


Style/TrailingCommaInArguments: Avoid comma after the last parameter of a method call. (https://github.com/bbatsov/ruby-style-guide#no-trailing-params-comma)

kyrylo · 2017-10-10T13:52:51Z

Pending discussion in #258 (comment) and #258 (comment) but the rest is fixed.

PTAL

kyrylo · 2017-10-10T16:20:01Z

PTAL

vmihailenco · 2017-10-10T16:30:56Z

lib/airbrake-ruby/backtrace.rb

@@ -188,6 +193,13 @@ def match_frame(regexp, stackframe)

        Patterns::GENERIC.match(stackframe)
      end
+
+      def read_code_hunk(config, frame)
+        code_hunk = Airbrake::CodeHunk.new(config, frame[:file], frame[:line]).to_h


Is there a good reason this method is called to_h? I see none.

I would expect CodeHunk to be shared and code look like this:

def constructor @code_hunk = Airbrake::CodeHunk.new(config) def populate_code(frame) @code = @code_hunk.get(file, line) if @code frame[:code] = @code

It looks ok to me, but the problem is that Backtrace doesn't have a constructor. We use Backtrace.parse, not Backtrace.new.parse.

No big deal, I can init at the same place.

vmihailenco · 2017-10-10T16:31:45Z

lib/airbrake-ruby/backtrace.rb

+          function: match[:function]
+        }
+
+        read_code_hunk(config, frame) if config.code_hunks


This is supposed to read something, but it returns nil.

vmihailenco · 2017-10-10T16:32:11Z

lib/airbrake-ruby/code_hunk.rb

+
+    private
+
+    def fetch_code


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

vmihailenco · 2017-10-10T16:33:13Z

lib/airbrake-ruby/code_hunk.rb

+      return unless File.exist?(@file)
+
+      begin
+        fetch_code


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

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.

vmihailenco · 2017-10-10T16:33:39Z

lib/airbrake-ruby/code_hunk.rb

+        return
+      end
+
+      @lines = { 1 => '' } if @lines.empty?


Why this is here and not in fetch_code?

vmihailenco · 2017-10-10T16:33:49Z

lib/airbrake-ruby/code_hunk.rb

+
+      begin
+        fetch_code
+      rescue StandardError => ex


This is an example you asked for.

vmihailenco · 2017-10-10T16:40:42Z

With this change notifier reads every file in backtrace for every notice which is a bit wasteful. I think we need to add a fixed size cache (say last 100 entries) in separate PR to make situation better.

kyrylo · 2017-10-10T17:53:12Z

With this change notifier reads every file in backtrace for every notice which is a bit wasteful. I think we need to add a fixed size cache (say last 100 entries) in separate PR to make situation better.

I agree.

I simplified code.

PTAL

houndci-bot reviewed Oct 10, 2017

View reviewed changes

kyrylo force-pushed the send-code-hunks branch 2 times, most recently from 8384f4a to 2b25010 Compare October 10, 2017 12:10

houndci-bot reviewed Oct 10, 2017

View reviewed changes

kyrylo force-pushed the send-code-hunks branch from 2b25010 to 13cbb0a Compare October 10, 2017 12:14

houndci-bot reviewed Oct 10, 2017

View reviewed changes

kyrylo force-pushed the send-code-hunks branch from 13cbb0a to b6eff5c Compare October 10, 2017 12:21

Implement code hunks support

1eab0dd

This feature is experimental. I am not documenting it in the README because users shouldn't be using it yet.

kyrylo force-pushed the send-code-hunks branch from b6eff5c to 1eab0dd Compare October 10, 2017 12:25

vmihailenco reviewed Oct 10, 2017

View reviewed changes

houndci-bot reviewed Oct 10, 2017

View reviewed changes

kyrylo force-pushed the send-code-hunks branch from b19be87 to ed7f03f Compare October 10, 2017 13:56

vmihailenco reviewed Oct 10, 2017

View reviewed changes

lib/airbrake-ruby/code_hunk.rb Outdated

return

end

@lines = { 1 => '' } if @lines.empty?

Copy link

vmihailenco Oct 10, 2017

Choose a reason for hiding this comment

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

Why this is here and not in fetch_code?

vmihailenco reviewed Oct 10, 2017

View reviewed changes

lib/airbrake-ruby/code_hunk.rb Outdated

begin

fetch_code

rescue StandardError => ex

Copy link

vmihailenco Oct 10, 2017

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.

kyrylo force-pushed the send-code-hunks branch from ee08390 to 319dc26 Compare October 10, 2017 17:42

+

1ee9384

kyrylo force-pushed the send-code-hunks branch from 319dc26 to 1ee9384 Compare October 10, 2017 17:43

vmihailenco approved these changes Oct 11, 2017

View reviewed changes

kyrylo merged commit d5b31f8 into master Oct 11, 2017

kyrylo deleted the send-code-hunks branch October 11, 2017 16:12

		@@ -0,0 +1 @@
		loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong line

Implement code hunks support #258

Implement code hunks support #258

Conversation

kyrylo commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmihailenco Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrylo commented Oct 10, 2017

kyrylo commented Oct 10, 2017

vmihailenco Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmihailenco Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmihailenco commented Oct 10, 2017

kyrylo commented Oct 10, 2017

vmihailenco Oct 10, 2017 •

edited

Loading

vmihailenco Oct 10, 2017 •

edited

Loading

vmihailenco Oct 10, 2017 •

edited

Loading