Skip to content

G
gitlab-ce
Commit fec75513 authored by Stan Hu's avatar Stan Hu
Browse files
Options

Merge branch 'revert-d64c89a8' into 'master' 

Revert "Merge branch 'marginalia' into 'master'"

Closes #37897

See merge request gitlab-org/gitlab!21015
parents c24720fa a232ec03
Hide whitespace changes
Inline Side-by-side
Showing with 3 additions and 325 deletions
Gemfile
View file @ fec75513
......@@ -22,7 +22,6 @@ gem 'rugged', '~> 0.28'
gem 'grape-path-helpers', '~> 1.1'
gem 'faraday', '~> 0.12'
gem 'marginalia', '~> 1.8.0'
# Authentication libraries
gem 'devise', '~> 4.6'
......
Gemfile.lock
View file @ fec75513
......@@ -591,9 +591,6 @@ GEM
mail_room (0.9.1)
marcel (0.3.3)
mimemagic (~> 0.3.2)
marginalia (1.8.0)
actionpack (>= 2.3)
activerecord (>= 2.3)
memoist (0.16.0)
memoizable (0.4.2)
thread_safe (~> 0.3, >= 0.3.1)
......@@ -1246,7 +1243,6 @@ DEPENDENCIES
lograge (~> 0.5)
loofah (~> 2.2)
mail_room (~> 0.9.1)
marginalia (~> 1.8.0)
memory_profiler (~> 0.9)
method_source (~> 0.8)
mimemagic (~> 0.3.2)
......
config/initializers/0_marginalia.rb deleted 100644 → 0
View file @ c24720fa
# frozen_string_literal: true
require 'marginalia'
::Marginalia::Comment.extend(::Gitlab::Marginalia::Comment)
# Patch to include support for 'Marginalia.without_annotation' method.
::Marginalia.singleton_class.prepend(Gitlab::Marginalia::InlineAnnotation)
# Patch to modify 'Marginalia::ActiveRecordInstrumentation.annotate_sql' method with feature check.
# Orignal Marginalia::ActiveRecordInstrumentation is included to ActiveRecord::ConnectionAdapters::PostgreSQLAdapter in the Marginalia Railtie.
# Refer: https://github.com/basecamp/marginalia/blob/v1.8.0/lib/marginalia/railtie.rb#L67
ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.prepend(Gitlab::Marginalia::ActiveRecordInstrumentation)
Marginalia::Comment.components = [:application, :controller, :action, :correlation_id, :jid, :job_class, :line]
Gitlab::Marginalia.set_application_name
Gitlab::Marginalia.enable_sidekiq_instrumentation
ee/spec/lib/gitlab/geo/health_check_spec.rb
View file @ fec75513
......@@ -67,7 +67,6 @@ describe Gitlab::Geo::HealthCheck, :geo do
context 'streaming replication' do
it 'returns an error when replication is not working' do
allow(Gitlab::Database).to receive(:pg_last_wal_receive_lsn).and_return('pg_last_xlog_receive_location')
allow(Gitlab::Database).to receive(:cached_table_exists?).with('features').and_return(false)
allow(ActiveRecord::Base).to receive_message_chain('connection.execute').with(no_args).with('SELECT * FROM pg_last_xlog_receive_location() as result').and_return(['result' => 'fake'])
allow(ActiveRecord::Base).to receive_message_chain('connection.select_values').with(no_args).with('SELECT pid FROM pg_stat_wal_receiver').and_return([])
......@@ -80,7 +79,6 @@ describe Gitlab::Geo::HealthCheck, :geo do
allow(subject).to receive(:streaming_replication_enabled?).and_return(false)
allow(subject).to receive(:archive_recovery_replication_enabled?).and_return(true)
allow(Gitlab::Database).to receive(:pg_last_xact_replay_timestamp).and_return('pg_last_xact_replay_timestamp')
allow(Gitlab::Database).to receive(:cached_table_exists?).with('features').and_return(false)
allow(ActiveRecord::Base).to receive_message_chain('connection.execute').with(no_args).with('SELECT * FROM pg_last_xact_replay_timestamp() as result').and_return([{ 'result' => nil }])
expect(subject.perform_checks).to match(/Geo node does not appear to be replicating the database from the primary node/)
......
lib/gitlab/marginalia.rb deleted 100644 → 0
View file @ c24720fa
# frozen_string_literal: true
module Gitlab
module Marginalia
MARGINALIA_FEATURE_FLAG = :marginalia
def self.set_application_name
::Marginalia.application_name = Gitlab.process_name
end
def self.enable_sidekiq_instrumentation
if Sidekiq.server?
::Marginalia::SidekiqInstrumentation.enable!
end
end
def self.feature_enabled?
return false unless Gitlab::Database.cached_table_exists?('features')
Feature.enabled?(MARGINALIA_FEATURE_FLAG)
end
end
end
lib/gitlab/marginalia/active_record_instrumentation.rb deleted 100644 → 0
View file @ c24720fa
# frozen_string_literal: true
# Patch to annotate sql only when the feature is enabled.
module Gitlab
module Marginalia
module ActiveRecordInstrumentation
# CAUTION:
# Any method call which generates a query inside this function will get into a recursive loop unless called within `Marginalia.without_annotation` method.
def annotate_sql(sql)
if ActiveRecord::Base.connected? &&
::Marginalia.annotation_allowed? &&
::Marginalia.without_annotation { Gitlab::Marginalia.feature_enabled? }
super(sql)
else
sql
end
end
end
end
end
lib/gitlab/marginalia/comment.rb deleted 100644 → 0
View file @ c24720fa
# frozen_string_literal: true
# Module to support correlation_id and additional job details.
module Gitlab
module Marginalia
module Comment
private
def jid
bg_job["jid"] if bg_job.present?
end
def job_class
bg_job["class"] if bg_job.present?
end
def correlation_id
if bg_job.present?
bg_job["correlation_id"]
else
Labkit::Correlation::CorrelationId.current_id
end
end
def bg_job
job = ::Marginalia::Comment.marginalia_job
# We are using 'Marginalia::SidekiqInstrumentation' which does not support 'ActiveJob::Base'.
# Gitlab also uses 'ActionMailer::DeliveryJob' which inherits from ActiveJob::Base.
# So below condition is used to return metadata for such jobs.
if job && job.is_a?(ActionMailer::DeliveryJob)
{
"class" => job.arguments.first,
"jid" => job.job_id
}
else
job
end
end
end
end
end
lib/gitlab/marginalia/inline_annotation.rb deleted 100644 → 0
View file @ c24720fa
# frozen_string_literal: true
# Module with util methods to support ::Marginalia.without_annotation method.
module Gitlab
module Marginalia
module InlineAnnotation
def without_annotation(&block)
return unless block.present?
annotation_stack.push(false)
yield
ensure
annotation_stack.pop
end
def with_annotation(comment, &block)
annotation_stack.push(true)
super(comment, &block)
ensure
annotation_stack.pop
end
def annotation_stack
Thread.current[:annotation_stack] ||= []
end
def annotation_stack_top
annotation_stack.last
end
def annotation_allowed?
annotation_stack.empty? ? true : annotation_stack_top
end
end
end
end
spec/lib/feature_spec.rb
View file @ fec75513
......@@ -175,7 +175,6 @@ describe Feature do
let(:flag) { :some_feature_flag }
before do
stub_feature_flags(Gitlab::Marginalia::MARGINALIA_FEATURE_FLAG => false)
described_class.flipper.memoize = false
described_class.enabled?(flag)
end
......
spec/lib/marginalia_spec.rb deleted 100644 → 0
View file @ c24720fa
# frozen_string_literal: true
require 'spec_helper'
describe 'Marginalia spec' do
class MarginaliaTestController < ActionController::Base
def first_user
User.first
render body: nil
end
end
class MarginaliaTestJob
include Sidekiq::Worker
def perform
User.first
end
end
class MarginaliaTestMailer < BaseMailer
def first_user
User.first
end
end
def add_sidekiq_middleware
# Reference: https://github.com/mperham/sidekiq/wiki/Testing#testing-server-middlewaresidekiq
# Sidekiq test harness fakes worker without its server middlewares, so include instrumentation to 'Sidekiq::Testing' server middleware.
Sidekiq::Testing.server_middleware do |chain|
chain.add Marginalia::SidekiqInstrumentation::Middleware
end
end
def remove_sidekiq_middleware
Sidekiq::Testing.server_middleware do |chain|
chain.remove Marginalia::SidekiqInstrumentation::Middleware
end
end
def stub_feature(value)
stub_feature_flags(Gitlab::Marginalia::MARGINALIA_FEATURE_FLAG => value)
end
def make_request(correlation_id)
request_env = Rack::MockRequest.env_for('/')
::Labkit::Correlation::CorrelationId.use_id(correlation_id) do
MarginaliaTestController.action(:first_user).call(request_env)
end
end
describe 'For rails web requests' do
let(:correlation_id) { SecureRandom.uuid }
let(:recorded) { ActiveRecord::QueryRecorder.new { make_request(correlation_id) } }
let(:component_map) do
{
"application" => "test",
"controller" => "marginalia_test",
"action" => "first_user",
"line" => "/spec/support/helpers/query_recorder.rb",
"correlation_id" => correlation_id
}
end
context 'when the feature is enabled' do
before do
stub_feature(true)
end
it 'generates a query that includes the component and value' do
component_map.each do |component, value|
expect(recorded.log.last).to include("#{component}:#{value}")
end
end
end
context 'when the feature is disabled' do
before do
stub_feature(false)
end
it 'excludes annotations in generated queries' do
expect(recorded.log.last).not_to include("/*")
expect(recorded.log.last).not_to include("*/")
end
end
end
describe 'for Sidekiq worker jobs' do
before(:all) do
add_sidekiq_middleware
# Because of faking, 'Sidekiq.server?' does not work so implicitly set application name which is done in config/initializers/0_marginalia.rb
Marginalia.application_name = "sidekiq"
end
after(:all) do
MarginaliaTestJob.clear
remove_sidekiq_middleware
end
around do |example|
Sidekiq::Testing.fake! { example.run }
end
before do
MarginaliaTestJob.perform_async
end
let(:sidekiq_job) { MarginaliaTestJob.jobs.first }
let(:recorded) { ActiveRecord::QueryRecorder.new { MarginaliaTestJob.drain } }
let(:component_map) do
{
"application" => "sidekiq",
"job_class" => "MarginaliaTestJob",
"line" => "/spec/support/sidekiq_middleware.rb",
"correlation_id" => sidekiq_job['correlation_id'],
"jid" => sidekiq_job['jid']
}
end
context 'when the feature is enabled' do
before do
stub_feature(true)
end
it 'generates a query that includes the component and value' do
component_map.each do |component, value|
expect(recorded.log.last).to include("#{component}:#{value}")
end
end
describe 'for ActionMailer delivery jobs' do
let(:delivery_job) { MarginaliaTestMailer.first_user.deliver_later }
let(:recorded) do
ActiveRecord::QueryRecorder.new do
delivery_job.perform_now
end
end
let(:component_map) do
{
"application" => "sidekiq",
"line" => "/lib/gitlab/i18n.rb",
"jid" => delivery_job.job_id,
"job_class" => delivery_job.arguments.first
}
end
it 'generates a query that includes the component and value' do
component_map.each do |component, value|
expect(recorded.log.last).to include("#{component}:#{value}")
end
end
end
end
context 'when the feature is disabled' do
before do
stub_feature(false)
end
it 'excludes annotations in generated queries' do
expect(recorded.log.last).not_to include("/*")
expect(recorded.log.last).not_to include("*/")
end
end
end
end
spec/serializers/pipeline_serializer_spec.rb
View file @ fec75513
......@@ -130,10 +130,10 @@ describe PipelineSerializer do
it 'preloads related merge requests' do
recorded = ActiveRecord::QueryRecorder.new { subject }
expected_query = "SELECT \"merge_requests\".* FROM \"merge_requests\" " \
"WHERE \"merge_requests\".\"id\" IN (#{merge_request_1.id}, #{merge_request_2.id})"
expect(recorded.log).to include(a_string_starting_with(expected_query))
expect(recorded.log)
.to include("SELECT \"merge_requests\".* FROM \"merge_requests\" " \
"WHERE \"merge_requests\".\"id\" IN (#{merge_request_1.id}, #{merge_request_2.id})")
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7