Merge branch 'marginalia-remove-fields' into 'master'

Remove controller, actions, and job class fields from Marginalia

See merge request gitlab-org/gitlab!55854

Gemfile

@@ -312,7 +312,7 @@ gem 'pg_query', '~> 1.3.0'
 gem 'premailer-rails', '~> 1.10.3'
 
 # LabKit: Tracing and Correlation
-gem 'gitlab-labkit', '~> 0.16.1'
+gem 'gitlab-labkit', '~> 0.16.2'
 # Thrift is a dependency of gitlab-labkit, we want a version higher than 0.14.0
 # because of https://gitlab.com/gitlab-org/gitlab/-/issues/321900
 gem 'thrift', '>= 0.14.0'

Gemfile.lock

@@ -455,7 +455,7 @@ GEM
       fog-xml (~> 0.1.0)
       google-api-client (>= 0.44.2, < 0.51)
       google-cloud-env (~> 1.2)
-    gitlab-labkit (0.16.1)
+    gitlab-labkit (0.16.2)
       actionpack (>= 5.0.0, < 7.0.0)
       activesupport (>= 5.0.0, < 7.0.0)
       grpc (~> 1.19)
@@ -1425,7 +1425,7 @@ DEPENDENCIES
   gitlab-experiment (~> 0.5.0)
   gitlab-fog-azure-rm (~> 1.0.1)
   gitlab-fog-google (~> 1.13)
-  gitlab-labkit (~> 0.16.1)
+  gitlab-labkit (~> 0.16.2)
   gitlab-license (~> 1.3)
   gitlab-mail_room (~> 0.0.9)
   gitlab-markup (~> 1.7.1)

config/initializers/0_marginalia.rb

@@ -13,7 +13,7 @@ require 'marginalia'
 # matching against the raw SQL, and prepending the comment prevents color
 # coding from working in the development log.
 Marginalia::Comment.prepend_comment = true if Rails.env.production?
-Marginalia::Comment.components = [:application, :controller, :action, :correlation_id, :jid, :job_class, :endpoint_id]
+Marginalia::Comment.components = [:application, :correlation_id, :jid, :endpoint_id]
 
 # As mentioned in https://github.com/basecamp/marginalia/pull/93/files,
 # adding :line has some overhead because a regexp on the backtrace has

doc/development/database_query_comments.md

@@ -20,9 +20,8 @@ and its application source from the comments.
 Queries generated from **Rails** include the following metadata in comments:
 
 - `application`
-- `controller`
-- `action`
 - `correlation_id`
+- `endpoint_id`
 - `line`
 
 Queries generated from **Sidekiq** workers will include the following metadata
@@ -30,20 +29,34 @@ in comments:
 
 - `application`
 - `jid`
-- `job_class`
 - `correlation_id`
+- `endpoint_id`
 - `line`
 
-Examples of queries with comments as observed in `development.log`:
+`endpoint_id` is a single field that can represent any endpoint in the application:
 
-1. Rails:
+- For Rails controllers, it's the controller and action. For example, `Projects::BlobController#show`.
+- For Grape API endpoints, it's the route. For example, `/api/:version/users/:id`.
+- For Sidekiq workers, it's the worker class name. For example, `UserStatusCleanup::BatchWorker`.
+
+`line` is not present in production logs due to the additional overhead required.
+
+Examples of queries with comments:
+
+- Rails:
+
+   ```sql
+   /*application:web,controller:blob,action:show,correlation_id:01EZVMR923313VV44ZJDJ7PMEZ,endpoint_id:Projects::BlobController#show*/ SELECT "routes".* FROM "routes" WHERE "routes"."source_id" = 75 AND "routes"."source_type" = 'Namespace' LIMIT 1
+   ```
+
+- Grape:
 
    ```sql
-   /*application:web,controller:jobs,action:trace,correlation_id:rYF4mey9CH3,line:/app/policies/project_policy.rb:504:in `feature_available?'*/ SELECT "project_features".* FROM "project_features" WHERE "project_features"."project_id" = $1 LIMIT $2 [["project_id", 5], ["LIMIT", 1]]
+   /*application:web,correlation_id:01EZVN0DAYGJF5XHG9N4VX8FAH,endpoint_id:/api/:version/users/:id*/ SELECT COUNT(*) FROM "users" INNER JOIN "user_follow_users" ON "users"."id" = "user_follow_users"."followee_id" WHERE "user_follow_users"."follower_id" = 1
    ```
 
-1. Sidekiq:
+- Sidekiq:
 
    ```sql
-   /*application:sidekiq,jid:e7d6668a39a991e323009833,job_class:ExpireJobCacheWorker,correlation_id:rYF4mey9CH3,line:/app/workers/expire_job_cache_worker.rb:14:in `perform'*/ SELECT "ci_pipelines".* FROM "ci_pipelines" WHERE "ci_pipelines"."id" = $1 LIMIT $2 [["id", 64], ["LIMIT", 1]]
+   /*application:sidekiq,correlation_id:df643992563683313bc0a0288fb55e23,jid:15fbc506590c625d7664b074,endpoint_id:UserStatusCleanup::BatchWorker,line:/app/workers/user_status_cleanup/batch_worker.rb:19:in `perform'*/ SELECT $1 AS one FROM "user_statuses" WHERE "user_statuses"."clear_status_at" <= $2 LIMIT $3
    ```

spec/lib/marginalia_spec.rb

@@ -3,18 +3,28 @@
 require 'spec_helper'
 
 RSpec.describe 'Marginalia spec' do
-  class MarginaliaTestController < ActionController::Base
+  class MarginaliaTestController < ApplicationController
+    skip_before_action :authenticate_user!, :check_two_factor_requirement
+
     def first_user
       User.first
       render body: nil
     end
+
+    private
+
+    [:auth_user, :current_user, :set_experimentation_subject_id_cookie, :signed_in?].each do |method|
+      define_method(method) { }
+    end
   end
 
   class MarginaliaTestJob
     include Sidekiq::Worker
 
     def perform
-      User.first
+      Gitlab::ApplicationContext.with_context(caller_id: self.class.name) do
+        User.first
+      end
     end
   end
 
@@ -30,10 +40,9 @@ RSpec.describe 'Marginalia spec' do
 
     let(:component_map) do
       {
-        "application"       => "test",
-        "controller"        => "marginalia_test",
-        "action"            => "first_user",
-        "correlation_id"    => correlation_id
+        "application"    => "test",
+        "endpoint_id"    => "MarginaliaTestController#first_user",
+        "correlation_id" => correlation_id
       }
     end
 
@@ -47,6 +56,7 @@ RSpec.describe 'Marginalia spec' do
   describe 'for Sidekiq worker jobs' do
     around do |example|
       with_sidekiq_server_middleware do |chain|
+        chain.add Labkit::Middleware::Sidekiq::Context::Server
         chain.add Marginalia::SidekiqInstrumentation::Middleware
         Marginalia.application_name = "sidekiq"
         example.run
@@ -66,10 +76,10 @@ RSpec.describe 'Marginalia spec' do
 
     let(:component_map) do
       {
-        "application"       => "sidekiq",
-        "job_class"         => "MarginaliaTestJob",
-        "correlation_id"    => sidekiq_job['correlation_id'],
-        "jid"               => sidekiq_job['jid']
+        "application"    => "sidekiq",
+        "endpoint_id"    => "MarginaliaTestJob",
+        "correlation_id" => sidekiq_job['correlation_id'],
+        "jid"            => sidekiq_job['jid']
       }
     end
 
@@ -80,19 +90,34 @@ RSpec.describe 'Marginalia spec' do
     end
 
     describe 'for ActionMailer delivery jobs' do
+      # We need to ensure that this runs through Sidekiq to take
+      # advantage of the middleware. There is a Rails bug that means we
+      # have to do some extra steps to make this happen:
+      # https://github.com/rails/rails/issues/37270#issuecomment-553927324
+      around do |example|
+        original_queue_adapter = ActiveJob::Base.queue_adapter
+        descendants = ActiveJob::Base.descendants + [ActiveJob::Base]
+
+        ActiveJob::Base.queue_adapter = :sidekiq
+        descendants.each(&:disable_test_adapter)
+        example.run
+        descendants.each { |a| a.enable_test_adapter(ActiveJob::QueueAdapters::TestAdapter.new) }
+        ActiveJob::Base.queue_adapter = original_queue_adapter
+      end
+
       let(:delivery_job) { MarginaliaTestMailer.first_user.deliver_later }
 
       let(:recorded) do
         ActiveRecord::QueryRecorder.new do
-          delivery_job.perform_now
+          Sidekiq::Worker.drain_all
         end
       end
 
       let(:component_map) do
         {
-          "application"  => "sidekiq",
-          "jid"          => delivery_job.job_id,
-          "job_class"    => delivery_job.arguments.first
+          "application" => "sidekiq",
+          "endpoint_id" => "ActionMailer::MailDeliveryJob",
+          "jid"         => delivery_job.job_id
         }
       end