Commit 31c4c4ce authored by Nick Thomas's avatar Nick Thomas

Remove metrics related to syntax highlighting performance

Two sets of metrics related to syntax highlighting seem to have been
forgotten about; their feature flags have been enabled on GitLab.com
for almost a year, while being default_enabled: false

Remove the FFs and accompanying metrics logic. We don't need a
changelog entry because they always stayed disabled by default.
parent 95b17b01
---
name: track_file_size_over_highlight_limit
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61273
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330374
milestone: '13.12'
type: development
group: group::code review
default_enabled: false
---
name: track_highlight_timeouts
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60956
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/329909
milestone: '13.12'
type: development
group: group::code review
default_enabled: false
......@@ -11,11 +11,7 @@ module Gitlab
end
def self.too_large?(size)
return false unless size.to_i > self.file_size_limit
over_highlight_size_limit.increment(source: "file size: #{self.file_size_limit}") if Feature.enabled?(:track_file_size_over_highlight_limit)
true
size.to_i > self.file_size_limit
end
attr_reader :blob_name
......@@ -74,14 +70,10 @@ module Gitlab
end
def highlight_rich(text, continue: true)
add_highlight_attempt_metric
tag = lexer.tag
tokens = lexer.lex(text, continue: continue)
Timeout.timeout(timeout_time) { @formatter.format(tokens, **context, tag: tag).html_safe }
rescue Timeout::Error => e
add_highlight_timeout_metric
Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
highlight_plain(text)
rescue StandardError
......@@ -95,38 +87,5 @@ module Gitlab
def link_dependencies(text, highlighted_text)
Gitlab::DependencyLinker.link(blob_name, text, highlighted_text)
end
def add_highlight_attempt_metric
return unless Feature.enabled?(:track_highlight_timeouts)
highlighting_attempt.increment(source: (@language || "undefined"))
end
def add_highlight_timeout_metric
return unless Feature.enabled?(:track_highlight_timeouts)
highlight_timeout.increment(source: Gitlab::Runtime.sidekiq? ? "background" : "foreground")
end
def highlighting_attempt
@highlight_attempt ||= Gitlab::Metrics.counter(
:file_highlighting_attempt,
'Counts the times highlighting has been attempted on a file'
)
end
def highlight_timeout
@highlight_timeout ||= Gitlab::Metrics.counter(
:highlight_timeout,
'Counts the times highlights have timed out'
)
end
def self.over_highlight_size_limit
@over_highlight_size_limit ||= Gitlab::Metrics.counter(
:over_highlight_size_limit,
'Count the times files have been over the highlight size limit'
)
end
end
end
......@@ -53,10 +53,6 @@ RSpec.describe Gitlab::Highlight do
stub_config(extra: { 'maximum_text_highlight_size_kilobytes' => 0.0001 } ) # 1.024 bytes
end
it 'increments the metric for oversized files' do
expect { result }.to change { over_highlight_size_limit('file size: 0.0001') }.by(1)
end
it 'returns plain version for long content' do
expect(result).to eq(%[<span id="LC1" class="line" lang="">(make-pathname :defaults name</span>\n<span id="LC2" class="line" lang="">:type "assem")</span>])
end
......@@ -126,79 +122,29 @@ RSpec.describe Gitlab::Highlight do
end
context 'timeout' do
subject { described_class.new('file.name', 'Contents') }
subject(:highlight) { described_class.new('file.rb', 'begin', language: 'ruby').highlight('Content') }
it 'utilizes timeout for web' do
expect(Timeout).to receive(:timeout).with(described_class::TIMEOUT_FOREGROUND).and_call_original
subject.highlight("Content")
highlight
end
it 'utilizes longer timeout for sidekiq' do
allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
expect(Timeout).to receive(:timeout).with(described_class::TIMEOUT_BACKGROUND).and_call_original
it 'falls back to plaintext on timeout' do
allow(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception)
expect(Timeout).to receive(:timeout).and_raise(Timeout::Error)
subject.highlight("Content")
end
end
expect(Rouge::Lexers::PlainText).to receive(:lex).and_call_original
describe 'highlight timeouts' do
let(:result) { described_class.highlight(file_name, content, language: "ruby") }
context 'when there is an attempt' do
it "increments the attempt counter with a defined language" do
expect { result }.to change { highlight_attempt_total("ruby") }
end
it "increments the attempt counter with an undefined language" do
expect do
described_class.highlight(file_name, content)
end.to change { highlight_attempt_total("undefined") }
end
highlight
end
context 'when there is a timeout error while highlighting' do
before do
allow(Timeout).to receive(:timeout).twice.and_raise(Timeout::Error)
# This is done twice because it's rescued first and then
# calls the original exception
end
it "increments the foreground counter if it's in the foreground" do
expect { result }
.to raise_error(Timeout::Error)
.and change { highlight_timeout_total('foreground') }.by(1)
.and not_change { highlight_timeout_total('background') }
end
it "increments the background counter if it's in the background" do
allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
it 'utilizes longer timeout for sidekiq' do
allow(Gitlab::Runtime).to receive(:sidekiq?).and_return(true)
expect(Timeout).to receive(:timeout).with(described_class::TIMEOUT_BACKGROUND).and_call_original
expect { result }
.to raise_error(Timeout::Error)
.and change { highlight_timeout_total('background') }.by(1)
.and not_change { highlight_timeout_total('foreground') }
end
highlight
end
end
end
def highlight_timeout_total(source)
Gitlab::Metrics
.counter(:highlight_timeout, 'Counts the times highlights have timed out')
.get(source: source)
end
def highlight_attempt_total(source)
Gitlab::Metrics
.counter(:file_highlighting_attempt, 'Counts the times highlighting has been attempted on a file')
.get(source: source)
end
def over_highlight_size_limit(source)
Gitlab::Metrics
.counter(:over_highlight_size_limit,
'Count the times text has been over the highlight size limit')
.get(source: source)
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment