Commit a73e4988 authored by Stan Hu's avatar Stan Hu

Move backtrace cleaner from Profiler into its own class

We call `GitLab::Profiler.clean_backtrace` in a lot of places where we
aren't actually profiling. It makes sense to break this out into its own
class method.

This change also reduces memory usage and speeds up the backtrace
cleaner since the regexp is computed once at load time.

Closes https://gitlab.com/gitlab-org/gitlab/issues/36645
parent 5a8ca1cb
# frozen_string_literal: true
module Gitlab
module BacktraceCleaner
IGNORE_BACKTRACES = %w[
config/initializers
ee/lib/gitlab/middleware/
lib/gitlab/correlation_id.rb
lib/gitlab/database/load_balancing/
lib/gitlab/etag_caching/
lib/gitlab/i18n.rb
lib/gitlab/metrics/
lib/gitlab/middleware/
lib/gitlab/performance_bar/
lib/gitlab/profiler.rb
lib/gitlab/query_limiting/
lib/gitlab/request_context.rb
lib/gitlab/request_profiler/
lib/gitlab/sidekiq_logging/
lib/gitlab/sidekiq_middleware/
lib/gitlab/sidekiq_status/
lib/gitlab/tracing/
lib/gitlab/webpack/dev_server_middleware.rb
].freeze
IGNORED_BACKTRACES_REGEXP = Regexp.union(IGNORE_BACKTRACES).freeze
def self.clean_backtrace(backtrace)
return unless backtrace
Array(Rails.backtrace_cleaner.clean(backtrace)).reject do |line|
line.match(IGNORED_BACKTRACES_REGEXP)
end
end
end
end
...@@ -42,7 +42,7 @@ module Gitlab ...@@ -42,7 +42,7 @@ module Gitlab
end end
def store_pull_request_error(pull_request, ex) def store_pull_request_error(pull_request, ex)
backtrace = Gitlab::Profiler.clean_backtrace(ex.backtrace) backtrace = Gitlab::BacktraceCleaner.clean_backtrace(ex.backtrace)
error = { type: :pull_request, iid: pull_request.iid, errors: ex.message, trace: backtrace, raw_response: pull_request.raw } error = { type: :pull_request, iid: pull_request.iid, errors: ex.message, trace: backtrace, raw_response: pull_request.raw }
Gitlab::ErrorTracking.log_exception(ex, error) Gitlab::ErrorTracking.log_exception(ex, error)
......
...@@ -13,7 +13,7 @@ module Gitlab ...@@ -13,7 +13,7 @@ module Gitlab
) )
if exception.backtrace if exception.backtrace
payload['exception.backtrace'] = Gitlab::Profiler.clean_backtrace(exception.backtrace) payload['exception.backtrace'] = Gitlab::BacktraceCleaner.clean_backtrace(exception.backtrace)
end end
end end
end end
......
...@@ -27,7 +27,7 @@ module Gitlab ...@@ -27,7 +27,7 @@ module Gitlab
feature: method_name, feature: method_name,
args: args, args: args,
duration: duration, duration: duration,
backtrace: Gitlab::Profiler.clean_backtrace(caller)) backtrace: Gitlab::BacktraceCleaner.clean_backtrace(caller))
end end
result result
......
...@@ -179,7 +179,7 @@ module Gitlab ...@@ -179,7 +179,7 @@ module Gitlab
self.query_time += duration self.query_time += duration
if Gitlab::PerformanceBar.enabled_for_request? if Gitlab::PerformanceBar.enabled_for_request?
add_call_details(feature: "#{service}##{rpc}", duration: duration, request: request_hash, rpc: rpc, add_call_details(feature: "#{service}##{rpc}", duration: duration, request: request_hash, rpc: rpc,
backtrace: Gitlab::Profiler.clean_backtrace(caller)) backtrace: Gitlab::BacktraceCleaner.clean_backtrace(caller))
end end
end end
...@@ -438,7 +438,7 @@ module Gitlab ...@@ -438,7 +438,7 @@ module Gitlab
def self.count_stack def self.count_stack
return unless Gitlab::SafeRequestStore.active? return unless Gitlab::SafeRequestStore.active?
stack_string = Gitlab::Profiler.clean_backtrace(caller).drop(1).join("\n") stack_string = Gitlab::BacktraceCleaner.clean_backtrace(caller).drop(1).join("\n")
Gitlab::SafeRequestStore[:stack_counter] ||= Hash.new Gitlab::SafeRequestStore[:stack_counter] ||= Hash.new
......
...@@ -107,7 +107,7 @@ module Gitlab ...@@ -107,7 +107,7 @@ module Gitlab
super super
Gitlab::Profiler.clean_backtrace(caller).each do |caller_line| Gitlab::BacktraceCleaner.clean_backtrace(caller).each do |caller_line|
stripped_caller_line = caller_line.sub("#{Rails.root}/", '') stripped_caller_line = caller_line.sub("#{Rails.root}/", '')
super(" ↳ #{stripped_caller_line}") super(" ↳ #{stripped_caller_line}")
...@@ -117,14 +117,6 @@ module Gitlab ...@@ -117,14 +117,6 @@ module Gitlab
end end
end end
def self.clean_backtrace(backtrace)
return unless backtrace
Array(Rails.backtrace_cleaner.clean(backtrace)).reject do |line|
line.match(Regexp.union(IGNORE_BACKTRACES))
end
end
def self.with_custom_logger(logger) def self.with_custom_logger(logger)
original_colorize_logging = ActiveSupport::LogSubscriber.colorize_logging original_colorize_logging = ActiveSupport::LogSubscriber.colorize_logging
original_activerecord_logger = ActiveRecord::Base.logger original_activerecord_logger = ActiveRecord::Base.logger
......
...@@ -18,7 +18,7 @@ module Gitlab ...@@ -18,7 +18,7 @@ module Gitlab
data.merge!(job_data) if job_data.present? data.merge!(job_data) if job_data.present?
end end
data[:error_backtrace] = Gitlab::Profiler.clean_backtrace(job_exception.backtrace) if job_exception.backtrace.present? data[:error_backtrace] = Gitlab::BacktraceCleaner.clean_backtrace(job_exception.backtrace) if job_exception.backtrace.present?
Sidekiq.logger.warn(data) Sidekiq.logger.warn(data)
end end
......
...@@ -32,7 +32,7 @@ module Peek ...@@ -32,7 +32,7 @@ module Peek
detail_store << { detail_store << {
duration: finish - start, duration: finish - start,
sql: data[:sql].strip, sql: data[:sql].strip,
backtrace: Gitlab::Profiler.clean_backtrace(caller) backtrace: Gitlab::BacktraceCleaner.clean_backtrace(caller)
} }
end end
end end
......
...@@ -23,7 +23,7 @@ module Gitlab ...@@ -23,7 +23,7 @@ module Gitlab
detail_store << { detail_store << {
cmd: args.first, cmd: args.first,
duration: duration, duration: duration,
backtrace: ::Gitlab::Profiler.clean_backtrace(caller) backtrace: ::Gitlab::BacktraceCleaner.clean_backtrace(caller)
} }
end end
......
...@@ -112,7 +112,7 @@ describe 'lograge', type: :request do ...@@ -112,7 +112,7 @@ describe 'lograge', type: :request do
expect(log_data['exception.class']).to eq('RuntimeError') expect(log_data['exception.class']).to eq('RuntimeError')
expect(log_data['exception.message']).to eq('bad request') expect(log_data['exception.message']).to eq('bad request')
expect(log_data['exception.backtrace']).to eq(Gitlab::Profiler.clean_backtrace(backtrace)) expect(log_data['exception.backtrace']).to eq(Gitlab::BacktraceCleaner.clean_backtrace(backtrace))
end end
end end
end end
......
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::BacktraceCleaner do
describe '.clean_backtrace' do
it 'uses the Rails backtrace cleaner' do
backtrace = []
expect(Rails.backtrace_cleaner).to receive(:clean).with(backtrace)
described_class.clean_backtrace(backtrace)
end
it 'removes lines from IGNORE_BACKTRACES' do
backtrace = [
"lib/gitlab/gitaly_client.rb:294:in `block (2 levels) in migrate'",
"lib/gitlab/gitaly_client.rb:331:in `allow_n_plus_1_calls'",
"lib/gitlab/gitaly_client.rb:280:in `block in migrate'",
"lib/gitlab/metrics/influx_db.rb:103:in `measure'",
"lib/gitlab/gitaly_client.rb:278:in `migrate'",
"lib/gitlab/git/repository.rb:1451:in `gitaly_migrate'",
"lib/gitlab/git/commit.rb:66:in `find'",
"app/models/repository.rb:1047:in `find_commit'",
"lib/gitlab/metrics/instrumentation.rb:159:in `block in find_commit'",
"lib/gitlab/metrics/method_call.rb:36:in `measure'",
"lib/gitlab/metrics/instrumentation.rb:159:in `find_commit'",
"app/models/repository.rb:113:in `commit'",
"lib/gitlab/i18n.rb:50:in `with_locale'",
"lib/gitlab/middleware/multipart.rb:95:in `call'",
"lib/gitlab/request_profiler/middleware.rb:14:in `call'",
"ee/lib/gitlab/database/load_balancing/rack_middleware.rb:37:in `call'",
"ee/lib/gitlab/jira/middleware.rb:15:in `call'"
]
expect(described_class.clean_backtrace(backtrace))
.to eq([
"lib/gitlab/gitaly_client.rb:294:in `block (2 levels) in migrate'",
"lib/gitlab/gitaly_client.rb:331:in `allow_n_plus_1_calls'",
"lib/gitlab/gitaly_client.rb:280:in `block in migrate'",
"lib/gitlab/gitaly_client.rb:278:in `migrate'",
"lib/gitlab/git/repository.rb:1451:in `gitaly_migrate'",
"lib/gitlab/git/commit.rb:66:in `find'",
"app/models/repository.rb:1047:in `find_commit'",
"app/models/repository.rb:113:in `commit'",
"ee/lib/gitlab/jira/middleware.rb:15:in `call'"
])
end
end
end
...@@ -39,7 +39,7 @@ describe Gitlab::GrapeLogging::Loggers::ExceptionLogger do ...@@ -39,7 +39,7 @@ describe Gitlab::GrapeLogging::Loggers::ExceptionLogger do
before do before do
current_backtrace = caller current_backtrace = caller
allow(exception).to receive(:backtrace).and_return(current_backtrace) allow(exception).to receive(:backtrace).and_return(current_backtrace)
expected['exception.backtrace'] = Gitlab::Profiler.clean_backtrace(current_backtrace) expected['exception.backtrace'] = Gitlab::BacktraceCleaner.clean_backtrace(current_backtrace)
end end
it 'includes the backtrace' do it 'includes the backtrace' do
......
...@@ -120,51 +120,6 @@ describe Gitlab::Profiler do ...@@ -120,51 +120,6 @@ describe Gitlab::Profiler do
end end
end end
describe '.clean_backtrace' do
it 'uses the Rails backtrace cleaner' do
backtrace = []
expect(Rails.backtrace_cleaner).to receive(:clean).with(backtrace)
described_class.clean_backtrace(backtrace)
end
it 'removes lines from IGNORE_BACKTRACES' do
backtrace = [
"lib/gitlab/gitaly_client.rb:294:in `block (2 levels) in migrate'",
"lib/gitlab/gitaly_client.rb:331:in `allow_n_plus_1_calls'",
"lib/gitlab/gitaly_client.rb:280:in `block in migrate'",
"lib/gitlab/metrics/influx_db.rb:103:in `measure'",
"lib/gitlab/gitaly_client.rb:278:in `migrate'",
"lib/gitlab/git/repository.rb:1451:in `gitaly_migrate'",
"lib/gitlab/git/commit.rb:66:in `find'",
"app/models/repository.rb:1047:in `find_commit'",
"lib/gitlab/metrics/instrumentation.rb:159:in `block in find_commit'",
"lib/gitlab/metrics/method_call.rb:36:in `measure'",
"lib/gitlab/metrics/instrumentation.rb:159:in `find_commit'",
"app/models/repository.rb:113:in `commit'",
"lib/gitlab/i18n.rb:50:in `with_locale'",
"lib/gitlab/middleware/multipart.rb:95:in `call'",
"lib/gitlab/request_profiler/middleware.rb:14:in `call'",
"ee/lib/gitlab/database/load_balancing/rack_middleware.rb:37:in `call'",
"ee/lib/gitlab/jira/middleware.rb:15:in `call'"
]
expect(described_class.clean_backtrace(backtrace))
.to eq([
"lib/gitlab/gitaly_client.rb:294:in `block (2 levels) in migrate'",
"lib/gitlab/gitaly_client.rb:331:in `allow_n_plus_1_calls'",
"lib/gitlab/gitaly_client.rb:280:in `block in migrate'",
"lib/gitlab/gitaly_client.rb:278:in `migrate'",
"lib/gitlab/git/repository.rb:1451:in `gitaly_migrate'",
"lib/gitlab/git/commit.rb:66:in `find'",
"app/models/repository.rb:1047:in `find_commit'",
"app/models/repository.rb:113:in `commit'",
"ee/lib/gitlab/jira/middleware.rb:15:in `call'"
])
end
end
describe '.with_custom_logger' do describe '.with_custom_logger' do
context 'when the logger is set' do context 'when the logger is set' do
it 'uses the replacement logger for the duration of the block' do it 'uses the replacement logger for the duration of the block' do
......
...@@ -33,7 +33,7 @@ describe Gitlab::SidekiqLogging::ExceptionHandler do ...@@ -33,7 +33,7 @@ describe Gitlab::SidekiqLogging::ExceptionHandler do
error_class: 'RuntimeError', error_class: 'RuntimeError',
error_message: exception_message, error_message: exception_message,
context: 'Test', context: 'Test',
error_backtrace: Gitlab::Profiler.clean_backtrace(backtrace) error_backtrace: Gitlab::BacktraceCleaner.clean_backtrace(backtrace)
) )
expect(logger).to receive(:warn).with(expected_data) expect(logger).to receive(:warn).with(expected_data)
......
...@@ -16,7 +16,7 @@ module ActiveRecord ...@@ -16,7 +16,7 @@ module ActiveRecord
def show_backtrace(values) def show_backtrace(values)
Rails.logger.debug("QueryRecorder SQL: #{values[:sql]}") Rails.logger.debug("QueryRecorder SQL: #{values[:sql]}")
Gitlab::Profiler.clean_backtrace(caller).each { |line| Rails.logger.debug(" --> #{line}") } Gitlab::BacktraceCleaner.clean_backtrace(caller).each { |line| Rails.logger.debug(" --> #{line}") }
end end
def callback(name, start, finish, message_id, values) def callback(name, start, finish, message_id, values)
......
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