diff options
Diffstat (limited to 'lib/gitlab/diff')
| -rw-r--r-- | lib/gitlab/diff/custom_diff.rb | 43 | ||||
| -rw-r--r-- | lib/gitlab/diff/file.rb | 12 | ||||
| -rw-r--r-- | lib/gitlab/diff/line.rb | 17 | ||||
| -rw-r--r-- | lib/gitlab/diff/parallel_diff.rb | 2 | ||||
| -rw-r--r-- | lib/gitlab/diff/rendered/notebook/diff_file.rb | 51 |
5 files changed, 96 insertions, 29 deletions
diff --git a/lib/gitlab/diff/custom_diff.rb b/lib/gitlab/diff/custom_diff.rb index af1fd8fb03e..860f87a28a3 100644 --- a/lib/gitlab/diff/custom_diff.rb +++ b/lib/gitlab/diff/custom_diff.rb @@ -2,17 +2,29 @@ module Gitlab module Diff module CustomDiff + RENDERED_TIMEOUT_BACKGROUND = 20.seconds + RENDERED_TIMEOUT_FOREGROUND = 1.5.seconds + BACKGROUND_EXECUTION = 'background' + FOREGROUND_EXECUTION = 'foreground' + LOG_IPYNBDIFF_GENERATED = 'IPYNB_DIFF_GENERATED' + LOG_IPYNBDIFF_TIMEOUT = 'IPYNB_DIFF_TIMEOUT' + LOG_IPYNBDIFF_INVALID = 'IPYNB_DIFF_INVALID' + class << self def preprocess_before_diff(path, old_blob, new_blob) return unless path.ends_with? '.ipynb' - transformed_diff(old_blob&.data, new_blob&.data)&.tap do - transformed_for_diff(new_blob, old_blob) - Gitlab::AppLogger.info({ message: 'IPYNB_DIFF_GENERATED' }) + Timeout.timeout(timeout_time) do + transformed_diff(old_blob&.data, new_blob&.data)&.tap do + transformed_for_diff(new_blob, old_blob) + log_event(LOG_IPYNBDIFF_GENERATED) + end end + rescue Timeout::Error => e + rendered_timeout.increment(source: execution_source) + log_event(LOG_IPYNBDIFF_TIMEOUT, e) rescue IpynbDiff::InvalidNotebookError, IpynbDiff::InvalidTokenError => e - Gitlab::ErrorTracking.log_exception(e) - nil + log_event(LOG_IPYNBDIFF_INVALID, e) end def transformed_diff(before, after) @@ -50,6 +62,27 @@ module Gitlab blobs_with_transformed_diffs[b] = true if b end end + + def rendered_timeout + @rendered_timeout ||= Gitlab::Metrics.counter( + :ipynb_semantic_diff_timeouts_total, + 'Counts the times notebook rendering timed out' + ) + end + + def timeout_time + Gitlab::Runtime.sidekiq? ? RENDERED_TIMEOUT_BACKGROUND : RENDERED_TIMEOUT_FOREGROUND + end + + def execution_source + Gitlab::Runtime.sidekiq? ? BACKGROUND_EXECUTION : FOREGROUND_EXECUTION + end + + def log_event(message, error = nil) + Gitlab::AppLogger.info({ message: message }) + Gitlab::ErrorTracking.track_exception(error) if error + nil + end end end end diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index 89822af2455..61bb0c797b4 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -44,7 +44,13 @@ module Gitlab new_blob_lazy old_blob_lazy - diff.diff = Gitlab::Diff::CustomDiff.preprocess_before_diff(diff.new_path, old_blob_lazy, new_blob_lazy) || diff.diff unless use_renderable_diff? + if use_semantic_ipynb_diff? && !use_renderable_diff? + diff.diff = Gitlab::Diff::CustomDiff.preprocess_before_diff(diff.new_path, old_blob_lazy, new_blob_lazy) || diff.diff + end + end + + def use_semantic_ipynb_diff? + strong_memoize(:_use_semantic_ipynb_diff) { Feature.enabled?(:ipynb_semantic_diff, repository.project, default_enabled: :yaml) } end def use_renderable_diff? @@ -375,7 +381,7 @@ module Gitlab end def rendered - return unless use_renderable_diff? && ipynb? + return unless use_semantic_ipynb_diff? && use_renderable_diff? && ipynb? && modified_file? && !too_large? strong_memoize(:rendered) { Rendered::Notebook::DiffFile.new(self) } end @@ -410,7 +416,7 @@ module Gitlab end def ipynb? - modified_file? && file_path.ends_with?('.ipynb') + file_path.ends_with?('.ipynb') end # We can't use Object#try because Blob doesn't inherit from Object, but diff --git a/lib/gitlab/diff/line.rb b/lib/gitlab/diff/line.rb index c2b834c71b5..316a0d2815a 100644 --- a/lib/gitlab/diff/line.rb +++ b/lib/gitlab/diff/line.rb @@ -9,8 +9,8 @@ module Gitlab SERIALIZE_KEYS = %i(line_code rich_text text type index old_pos new_pos).freeze attr_reader :marker_ranges - attr_writer :text, :rich_text, :discussable - attr_accessor :index, :type, :old_pos, :new_pos, :line_code + attr_writer :text, :rich_text + attr_accessor :index, :old_pos, :new_pos, :line_code, :type def initialize(text, type, index, old_pos, new_pos, parent_file: nil, line_code: nil, rich_text: nil) @text = text @@ -24,9 +24,7 @@ module Gitlab # When line code is not provided from cache store we build it # using the parent_file(Diff::File or Conflict::File). @line_code = line_code || calculate_line_code - @marker_ranges = [] - @discussable = true end def self.init_from_hash(hash) @@ -81,23 +79,28 @@ module Gitlab end def added? - %w[new new-nonewline].include?(type) + %w[new new-nonewline new-nomappinginraw].include?(type) end def removed? - %w[old old-nonewline].include?(type) + %w[old old-nonewline old-nomappinginraw].include?(type) end def meta? %w[match new-nonewline old-nonewline].include?(type) end + def has_mapping_in_raw? + # Used for rendered diff, when the displayed line doesn't have a matching line in the raw diff + !type&.ends_with?('nomappinginraw') + end + def match? type == :match end def discussable? - @discussable && !meta? + has_mapping_in_raw? && !meta? end def suggestible? diff --git a/lib/gitlab/diff/parallel_diff.rb b/lib/gitlab/diff/parallel_diff.rb index 77b65fea726..cbfc20d3d62 100644 --- a/lib/gitlab/diff/parallel_diff.rb +++ b/lib/gitlab/diff/parallel_diff.rb @@ -44,7 +44,7 @@ module Gitlab free_right_index = nil i += 1 end - elsif line.meta? || line.unchanged? + elsif line.meta? || line.unchanged? || !line.has_mapping_in_raw? # line in the right panel is the same as in the left one lines << { left: line, diff --git a/lib/gitlab/diff/rendered/notebook/diff_file.rb b/lib/gitlab/diff/rendered/notebook/diff_file.rb index e700e730f20..cf97569ca31 100644 --- a/lib/gitlab/diff/rendered/notebook/diff_file.rb +++ b/lib/gitlab/diff/rendered/notebook/diff_file.rb @@ -6,6 +6,14 @@ module Gitlab include Gitlab::Utils::StrongMemoize class DiffFile < Gitlab::Diff::File + RENDERED_TIMEOUT_BACKGROUND = 10.seconds + RENDERED_TIMEOUT_FOREGROUND = 1.5.seconds + BACKGROUND_EXECUTION = 'background' + FOREGROUND_EXECUTION = 'foreground' + LOG_IPYNBDIFF_GENERATED = 'IPYNB_DIFF_GENERATED' + LOG_IPYNBDIFF_TIMEOUT = 'IPYNB_DIFF_TIMEOUT' + LOG_IPYNBDIFF_INVALID = 'IPYNB_DIFF_INVALID' + attr_reader :source_diff delegate :repository, :diff_refs, :fallback_diff_refs, :unfolded, :unique_identifier, @@ -52,14 +60,17 @@ module Gitlab def notebook_diff strong_memoize(:notebook_diff) do - Gitlab::AppLogger.info({ message: 'IPYNB_DIFF_GENERATED' }) - - IpynbDiff.diff(source_diff.old_blob&.data, source_diff.new_blob&.data, - raise_if_invalid_nb: true, - diffy_opts: { include_diff_info: true }) + Timeout.timeout(timeout_time) do + IpynbDiff.diff(source_diff.old_blob&.data, source_diff.new_blob&.data, + raise_if_invalid_nb: true, diffy_opts: { include_diff_info: true })&.tap do + log_event(LOG_IPYNBDIFF_GENERATED) + end + end + rescue Timeout::Error => e + rendered_timeout.increment(source: Gitlab::Runtime.sidekiq? ? BACKGROUND_EXECUTION : FOREGROUND_EXECUTION) + log_event(LOG_IPYNBDIFF_TIMEOUT, e) rescue IpynbDiff::InvalidNotebookError, IpynbDiff::InvalidTokenError => e - Gitlab::ErrorTracking.log_exception(e) - nil + log_event(LOG_IPYNBDIFF_INVALID, e) end end @@ -87,10 +98,7 @@ module Gitlab line.new_pos = removal_line_maps[line.old_pos] if line.new_pos == 0 && line.old_pos != 0 # Lines that do not appear on the original diff should not be commentable - - unless addition_line_maps[line.new_pos] || removal_line_maps[line.old_pos] - line.discussable = false - end + line.type = "#{line.type || 'unchanged'}-nomappinginraw" unless addition_line_maps[line.new_pos] || removal_line_maps[line.old_pos] line.line_code = line_code(line) line @@ -113,12 +121,29 @@ module Gitlab additions = {} source_diff.highlighted_diff_lines.each do |line| - removals[line.old_pos] = line.new_pos - additions[line.new_pos] = line.old_pos + removals[line.old_pos] = line.new_pos unless source_diff.new_file? + additions[line.new_pos] = line.old_pos unless source_diff.deleted_file? end [removals, additions] end + + def rendered_timeout + @rendered_timeout ||= Gitlab::Metrics.counter( + :ipynb_semantic_diff_timeouts_total, + 'Counts the times notebook diff rendering timed out' + ) + end + + def timeout_time + Gitlab::Runtime.sidekiq? ? RENDERED_TIMEOUT_BACKGROUND : RENDERED_TIMEOUT_FOREGROUND + end + + def log_event(message, error = nil) + Gitlab::AppLogger.info({ message: message }) + Gitlab::ErrorTracking.track_exception(error) if error + nil + end end end end |