diff options
| -rw-r--r-- | .gitlab-ci.yml | 2 | ||||
| -rw-r--r-- | .gitlab/ci/global.gitlab-ci.yml | 4 | ||||
| -rw-r--r-- | Gemfile | 1 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/assets/stylesheets/framework/common.scss | 1 | ||||
| -rw-r--r-- | app/helpers/broadcast_messages_helper.rb | 2 | ||||
| -rw-r--r-- | app/views/admin/broadcast_messages/_form.html.haml | 2 | ||||
| -rw-r--r-- | changelogs/unreleased/39465-39469-issues-solutions.yml | 6 | ||||
| -rw-r--r-- | changelogs/unreleased/41934-replace-fa-bullhorn-with-gitlab-svg-bullhorn-icon.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/42158-stage-dropdown-lists-fix.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/sh-skip-lfs-updates-mirroring.yml | 5 | ||||
| -rw-r--r-- | config/initializers/0_marginalia.rb | 18 | ||||
| -rw-r--r-- | doc/development/README.md | 4 | ||||
| -rw-r--r-- | doc/development/database_query_comments.md | 56 | ||||
| -rw-r--r-- | lib/gitlab/marginalia.rb | 28 | ||||
| -rw-r--r-- | lib/gitlab/marginalia/active_record_instrumentation.rb | 12 | ||||
| -rw-r--r-- | lib/gitlab/marginalia/comment.rb | 42 | ||||
| -rw-r--r-- | spec/lib/marginalia_spec.rb | 173 | ||||
| -rw-r--r-- | spec/serializers/pipeline_serializer_spec.rb | 6 | ||||
| -rw-r--r-- | spec/spec_helper.rb | 3 |
20 files changed, 371 insertions, 8 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 0ac9fca5a71..eae82d81757 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -24,7 +24,7 @@ variables: FLAKY_RSPEC_SUITE_REPORT_PATH: rspec_flaky/report-suite.json BUILD_ASSETS_IMAGE: "false" ES_JAVA_OPTS: "-Xms256m -Xmx256m" - ELASTIC_URL: "http://elastic:changeme@docker.elastic.co-elasticsearch-elasticsearch:9200" + ELASTIC_URL: "http://elastic:changeme@elasticsearch:9200" after_script: - date diff --git a/.gitlab/ci/global.gitlab-ci.yml b/.gitlab/ci/global.gitlab-ci.yml index 3c24ce4f24d..9ebd28c7258 100644 --- a/.gitlab/ci/global.gitlab-ci.yml +++ b/.gitlab/ci/global.gitlab-ci.yml @@ -213,7 +213,7 @@ - name: postgres:9.6 command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"] - name: redis:alpine - - name: docker.elastic.co/elasticsearch/elasticsearch:5.6.12 + - name: elasticsearch:5.6.12 .use-pg10-ee: image: "registry.gitlab.com/gitlab-org/gitlab-build-images:ruby-2.6.3-golang-1.12-git-2.24-lfs-2.9-chrome-73.0-node-12.x-yarn-1.16-postgresql-10-graphicsmagick-1.3.33" @@ -221,7 +221,7 @@ - name: postgres:10.9 command: ["postgres", "-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"] - name: redis:alpine - - name: docker.elastic.co/elasticsearch/elasticsearch:5.6.12 + - name: elasticsearch:5.6.12 .only-ee: only: @@ -22,6 +22,7 @@ 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' diff --git a/Gemfile.lock b/Gemfile.lock index 55674a79cc2..9913d2d1df9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -596,6 +596,9 @@ 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) @@ -1245,6 +1248,7 @@ 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) diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 4cbb2f5ba71..4b7dda3a2ff 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -433,6 +433,7 @@ img.emoji { .block { display: block; } .flex { display: flex; } .vertical-align-top { vertical-align: top; } +.vertical-align-text-top { vertical-align: text-top; } .vertical-align-middle { vertical-align: middle; } .vertical-align-sub { vertical-align: sub; } .flex-align-self-center { align-self: center; } diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index ec653aed91b..21e57a8d391 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -9,7 +9,7 @@ module BroadcastMessagesHelper return unless message.present? content_tag :div, dir: 'auto', class: 'broadcast-message', style: broadcast_message_style(message) do - icon('bullhorn') << ' ' << render_broadcast_message(message) + sprite_icon('bullhorn', size: 16, css_class: 'vertical-align-text-top mr-2') << ' ' << render_broadcast_message(message) end end diff --git a/app/views/admin/broadcast_messages/_form.html.haml b/app/views/admin/broadcast_messages/_form.html.haml index 03b7ae76de9..44d57beec0f 100644 --- a/app/views/admin/broadcast_messages/_form.html.haml +++ b/app/views/admin/broadcast_messages/_form.html.haml @@ -1,5 +1,5 @@ .broadcast-message-preview{ style: broadcast_message_style(@broadcast_message) } - = icon('bullhorn') + = sprite_icon('bullhorn', size: 16, css_class:'vertical-align-text-top mr-2') .js-broadcast-message-preview - if @broadcast_message.message.present? = render_broadcast_message(@broadcast_message) diff --git a/changelogs/unreleased/39465-39469-issues-solutions.yml b/changelogs/unreleased/39465-39469-issues-solutions.yml new file mode 100644 index 00000000000..62d314bc096 --- /dev/null +++ b/changelogs/unreleased/39465-39469-issues-solutions.yml @@ -0,0 +1,6 @@ +--- +title: UI improvements in the views for new project from template and the user groups + and snippets +merge_request: 21524 +author: Hector Bustillos +type: changed diff --git a/changelogs/unreleased/41934-replace-fa-bullhorn-with-gitlab-svg-bullhorn-icon.yml b/changelogs/unreleased/41934-replace-fa-bullhorn-with-gitlab-svg-bullhorn-icon.yml new file mode 100644 index 00000000000..a58a553cb70 --- /dev/null +++ b/changelogs/unreleased/41934-replace-fa-bullhorn-with-gitlab-svg-bullhorn-icon.yml @@ -0,0 +1,5 @@ +--- +title: Replace Font Awesome bullhorn icon with GitLab bullhorn icon +merge_request: +author: +type: other diff --git a/changelogs/unreleased/42158-stage-dropdown-lists-fix.yml b/changelogs/unreleased/42158-stage-dropdown-lists-fix.yml new file mode 100644 index 00000000000..793c4815130 --- /dev/null +++ b/changelogs/unreleased/42158-stage-dropdown-lists-fix.yml @@ -0,0 +1,5 @@ +--- +title: Stage dropdown lists style corrections +merge_request: 21607 +author: Hector Bustillos +type: fixed diff --git a/changelogs/unreleased/sh-skip-lfs-updates-mirroring.yml b/changelogs/unreleased/sh-skip-lfs-updates-mirroring.yml new file mode 100644 index 00000000000..112a91a60b3 --- /dev/null +++ b/changelogs/unreleased/sh-skip-lfs-updates-mirroring.yml @@ -0,0 +1,5 @@ +--- +title: Skip updating LFS objects in mirror updates if repository has not changed +merge_request: 21744 +author: +type: performance diff --git a/config/initializers/0_marginalia.rb b/config/initializers/0_marginalia.rb new file mode 100644 index 00000000000..f88a90854e3 --- /dev/null +++ b/config/initializers/0_marginalia.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'marginalia' + +::Marginalia::Comment.extend(::Gitlab::Marginalia::Comment) + +# 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 + +Gitlab::Marginalia.set_feature_cache diff --git a/doc/development/README.md b/doc/development/README.md index 6aeaf31ed29..3a972c4c588 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -103,6 +103,10 @@ description: 'Learn how to contribute to GitLab.' - [Swapping tables](swapping_tables.md) - [Deleting migrations](deleting_migrations.md) +### Debugging + +- Tracing the source of an SQL query using query comments with [Marginalia](database_query_comments.md) + ### Best practices - [Merge Request checklist](database_merge_request_checklist.md) diff --git a/doc/development/database_query_comments.md b/doc/development/database_query_comments.md new file mode 100644 index 00000000000..c1a927ef234 --- /dev/null +++ b/doc/development/database_query_comments.md @@ -0,0 +1,56 @@ +# Database query comments with Marginalia + +The [Marginalia gem](https://github.com/basecamp/marginalia) is used to add +query comments containing application related context information to PostgreSQL +queries generated by ActiveRecord. + +It is very useful for tracing problematic queries back to the application source. + +A DB Engineer during an on-call incident will have the full context of a query +and its application source from the comments. + +## Metadata information in comments + +Queries generated from **Rails** include the following metadata in comments: + +- `application` +- `controller` +- `action` +- `correlation_id` +- `line` + +Queries generated from **Sidekiq** workers will include the following metadata +in comments: + +- `application` +- `jid` +- `job_class` +- `correlation_id` +- `line` + +Examples of queries with comments as observed in `development.log`: + +1. Rails: + + ```sql + SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = $1 LIMIT $2 [["project_id", 5], ["LIMIT", 1]] /*application:web,controller:jobs,action:trace,correlation_id:rYF4mey9CH3,line:/app/policies/project_policy.rb:504:in `feature_available?'*/ + ``` + +1. Sidekiq: + + ```sql + SELECT "ci_pipelines".* FROM "ci_pipelines" WHERE "ci_pipelines"."id" = $1 LIMIT $2 [["id", 64], ["LIMIT", 1]] /*application:sidekiq,jid:e7d6668a39a991e323009833,job_class:ExpireJobCacheWorker,correlation_id:rYF4mey9CH3,line:/app/workers/expire_job_cache_worker.rb:14:in `perform'*/ + ``` + +## Enable/Disable the feature + +Enabling or disabling the feature requires a **restart/SIGHUP** of the Web and +Sidekiq workers, as the feature flag's state is memoized upon starting up. + +The `feature_flag` for this feature is **disabled** by default. You can enable +or disable it with: + +```ruby +Feature.enable(:marginalia) +Feature.disable(:marginalia) +``` diff --git a/lib/gitlab/marginalia.rb b/lib/gitlab/marginalia.rb new file mode 100644 index 00000000000..2be96cecae3 --- /dev/null +++ b/lib/gitlab/marginalia.rb @@ -0,0 +1,28 @@ +# 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.cached_feature_enabled? + !!@enabled + end + + def self.set_feature_cache + # During db:create and db:bootstrap skip feature query as DB is not available yet. + return false unless ActiveRecord::Base.connected? && Gitlab::Database.cached_table_exists?('features') + + @enabled = Feature.enabled?(MARGINALIA_FEATURE_FLAG) + end + end +end diff --git a/lib/gitlab/marginalia/active_record_instrumentation.rb b/lib/gitlab/marginalia/active_record_instrumentation.rb new file mode 100644 index 00000000000..3266b9f8336 --- /dev/null +++ b/lib/gitlab/marginalia/active_record_instrumentation.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +# Patch to annotate sql only when the feature is enabled. +module Gitlab + module Marginalia + module ActiveRecordInstrumentation + def annotate_sql(sql) + Gitlab::Marginalia.cached_feature_enabled? ? super(sql) : sql + end + end + end +end diff --git a/lib/gitlab/marginalia/comment.rb b/lib/gitlab/marginalia/comment.rb new file mode 100644 index 00000000000..a0eee823763 --- /dev/null +++ b/lib/gitlab/marginalia/comment.rb @@ -0,0 +1,42 @@ +# 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 diff --git a/spec/lib/marginalia_spec.rb b/spec/lib/marginalia_spec.rb new file mode 100644 index 00000000000..5dc54af99ce --- /dev/null +++ b/spec/lib/marginalia_spec.rb @@ -0,0 +1,173 @@ +# 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) + allow(Gitlab::Marginalia).to receive(:cached_feature_enabled?).and_return(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 diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 7661c8acc13..f1f761a6fd0 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -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("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)) end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 2afe80f455e..1f0119108a8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -159,6 +159,9 @@ RSpec.configure do |config| .with(:force_autodevops_on_by_default, anything) .and_return(false) + # Enable Marginalia feature for all specs in the test suite. + allow(Gitlab::Marginalia).to receive(:cached_feature_enabled?).and_return(true) + # The following can be removed once Vue Issuable Sidebar # is feature-complete and can be made default in place # of older sidebar. |