Modify cop to discourage use of `:always` data consistency

- ensure good practice to try and leverage the non
  primary database

.rubocop.yml

@@ -979,4 +979,10 @@ Search/NamespacedClass:
     - 'spec/migrations/**/*.rb'
     - 'app/experiments/**/*_experiment.rb'
     - 'ee/app/experiments/**/*_experiment.rb'
-    - 'lib/gitlab/instrumentation/**/*.rb'
+    - 'lib/gitlab/instrumentation/**/*.rb'
\ No newline at end of file
+
+SidekiqLoadBalancing/WorkerDataConsistency:
+  Enabled: true
+  Include:
+    - 'app/workers/**/*'
+    - 'ee/app/workers/**/*'

doc/development/sidekiq/worker_attributes.md

@@ -242,7 +242,7 @@ can put unsustainable load on the primary database server. We therefore added th
 By configuring a worker's `data_consistency` field, we can then allow the scheduler to target read replicas
 under several strategies outlined below.
 
-## Trading immediacy for reduced primary load
+### Trading immediacy for reduced primary load
 
 We require Sidekiq workers to make an explicit decision around whether they need to use the
 primary database node for all reads and writes, or whether reads can be served from replicas. This is
@@ -259,7 +259,8 @@ that mostly or exclusively perform writes, or workers that read their own writes
 into data consistency issues should a stale record be read back from a replica. **Try to avoid
 these scenarios, since `:always` should be considered the exception, not the rule.**
 
-To allow for reads to be served from replicas, we added two additional consistency modes: `:sticky` and `:delayed`.
+To allow for reads to be served from replicas, we added two additional consistency modes: `:sticky` and `:delayed`. A RuboCop rule
+reminds the developer when `:always` data consistency mode is used. If workers require the primary database, you can disable the rule in-line.
 
 When you declare either `:sticky` or `:delayed` consistency, workers become eligible for database
 load-balancing.
@@ -268,18 +269,17 @@ In both cases, if the replica is not up-to-date and the time from scheduling the
  the jobs sleep up to the minimum delay interval (0.8 seconds). This gives the replication process time to finish.
 The difference is in what happens when there is still replication lag after the delay: `sticky` workers
 switch over to the primary right away, whereas `delayed` workers fail fast and are retried once.
-If they still encounter replication lag, they also switch to the primary instead.
-**If your worker never performs any writes, it is strongly advised to apply one of these consistency settings,
-since it never needs to rely on the primary database node.**
+If the workers still encounter replication lag, they switch to the primary instead. **If your worker never performs any writes,
+it is strongly advised to apply `:sticky` or `:delayed` consistency settings, since the worker never needs to rely on the primary database node.**
 
 The table below shows the `data_consistency` attribute and its values, ordered by the degree to which
 they prefer read replicas and wait for replicas to catch up:
 
-| **Data Consistency**  | **Description**  |
-|--------------|-----------------------------|
-| `:always`    | The job is required to use the primary database (default). It should be used for workers that primarily perform writes, have strict requirements around data consistency when reading their own writes, or are cron jobs. |
-| `:sticky`    | The job prefers replicas, but switches to the primary for writes or when encountering replication lag. It should be used for jobs that require to be executed as fast as possible but can sustain a small initial queuing delay.  |
-| `:delayed`   | The job prefers replicas, but switches to the primary for writes. When encountering replication lag before the job starts, the job is retried once. If the replica is still not up to date on the next retry, it switches to the primary. It should be used for jobs where delaying execution further typically does not matter, such as cache expiration or web hooks execution. |
+| **Data consistency**  | **Description**  | **Guideline** |
+|--------------|-----------------------------|----------|
+| `:always`    | The job is required to use the primary database (default). | It should be used for workers that primarily perform writes, have strict requirements around data consistency when reading their own writes, or are cron jobs. |
+| `:sticky`    | The job prefers replicas, but switches to the primary for writes or when encountering replication lag. | It should be used for jobs that require to be executed as fast as possible but can sustain a small initial queuing delay.  |
+| `:delayed`   | The job prefers replicas, but switches to the primary for writes. When encountering replication lag before the job starts, the job is retried once. If the replica is still not up to date on the next retry, it switches to the primary. | It should be used for jobs where delaying execution further typically does not matter, such as cache expiration or web hooks execution. |
 
 In all cases workers read either from a replica that is fully caught up,
 or from the primary node, so data consistency is always ensured.

rubocop/cop/sidekiq_load_balancing/worker_data_consistency.rb

@@ -28,36 +28,29 @@ class WorkerDataConsistency < RuboCop::Cop::Base
 
         HELP_LINK = 'https://docs.gitlab.com/ee/development/sidekiq/worker_attributes.html#job-data-consistency-strategies'
 
-        MSG = <<~MSG
+        MISSING_DATA_CONSISTENCY_MSG = <<~MSG
           Should define data_consistency expectation.
-
-          It is encouraged for workers to use database replicas as much as possible by declaring
-          data_consistency to use the :delayed or :sticky modes. Mode :always will result in the
-          worker always hitting the primary database node for both reads and writes, which limits
-          scalability.
-
-          Some guidelines:
-
-          1. If your worker mostly writes or reads its own writes, use mode :always. TRY TO AVOID THIS.
-          2. If your worker performs mostly reads and can tolerate small delays, use mode :delayed.
-          3. If your worker performs mostly reads but cannot tolerate any delays, use mode :sticky.
-
           See #{HELP_LINK} for a more detailed explanation of these settings.
         MSG
 
+        DISCOURAGE_ALWAYS_MSG = "Refrain from using `:always` if possible." \
+                                "See #{HELP_LINK} for a more detailed explanation of these settings."
+
         def_node_search :application_worker?, <<~PATTERN
         `(send nil? :include (const nil? :ApplicationWorker))
         PATTERN
 
-        def_node_search :data_consistency_defined?, <<~PATTERN
-          `(send nil? :data_consistency ...)
+        def_node_matcher :data_consistency_value, <<~PATTERN
+          `(send nil? :data_consistency $(sym _) ...)
         PATTERN
 
         def on_class(node)
-          return unless in_worker?(node) && application_worker?(node)
-          return if data_consistency_defined?(node)
+          return unless application_worker?(node)
+
+          consistency = data_consistency_value(node)
+          return add_offense(node, message: MISSING_DATA_CONSISTENCY_MSG) if consistency.nil?
 
-          add_offense(node)
+          add_offense(consistency, message: DISCOURAGE_ALWAYS_MSG) if consistency.value == :always
         end
       end
     end

spec/rubocop/cop/sidekiq_load_balancing/worker_data_consistency_spec.rb

@@ -3,46 +3,95 @@
 require 'rubocop_spec_helper'
 require_relative '../../../../rubocop/cop/sidekiq_load_balancing/worker_data_consistency'
 
-RSpec.describe RuboCop::Cop::SidekiqLoadBalancing::WorkerDataConsistency do
-  before do
-    allow(cop)
-      .to receive(:in_worker?)
-      .and_return(true)
-  end
+RSpec.describe RuboCop::Cop::SidekiqLoadBalancing::WorkerDataConsistency, feature_category: :scalability do
+  context 'when data_consistency is not set' do
+    it 'adds an offense when not defining data_consistency' do
+      expect_offense(<<~CODE)
+        class SomeWorker
+        ^^^^^^^^^^^^^^^^ Should define data_consistency expectation.[...]
+          include ApplicationWorker
 
-  it 'adds an offense when not defining data_consistency' do
-    expect_offense(<<~CODE)
-      class SomeWorker
-      ^^^^^^^^^^^^^^^^ Should define data_consistency expectation.[...]
-        include ApplicationWorker
-
-        queue_namespace :pipeline_hooks
-        feature_category :continuous_integration
-        urgency :high
-      end
-    CODE
-  end
+          queue_namespace :pipeline_hooks
+          feature_category :continuous_integration
+          urgency :high
+        end
+      CODE
+    end
+
+    it 'adds no offense when defining data_consistency' do
+      expect_no_offenses(<<~CODE)
+        class SomeWorker
+          include ApplicationWorker
 
-  it 'adds no offense when defining data_consistency' do
-    expect_no_offenses(<<~CODE)
-      class SomeWorker
-        include ApplicationWorker
-
-        queue_namespace :pipeline_hooks
-        feature_category :continuous_integration
-        data_consistency :delayed
-        urgency :high
-      end
-    CODE
+          queue_namespace :pipeline_hooks
+          feature_category :continuous_integration
+          data_consistency :delayed
+          urgency :high
+        end
+      CODE
+    end
+
+    it 'adds no offense when worker is not an ApplicationWorker' do
+      expect_no_offenses(<<~CODE)
+        class SomeWorker
+          queue_namespace :pipeline_hooks
+          feature_category :continuous_integration
+          urgency :high
+        end
+      CODE
+    end
   end
 
-  it 'adds no offense when worker is not an ApplicationWorker' do
-    expect_no_offenses(<<~CODE)
-      class SomeWorker
-        queue_namespace :pipeline_hooks
-        feature_category :continuous_integration
-        urgency :high
-      end
-    CODE
+  context 'when data_consistency set to :always' do
+    it 'adds an offense when using `always` data_consistency' do
+      expect_offense(<<~CODE)
+        class SomeWorker
+          include ApplicationWorker
+          data_consistency :always
+                           ^^^^^^^ Refrain from using `:always` if possible.[...]
+
+          queue_namespace :pipeline_hooks
+          feature_category :continuous_integration
+          urgency :high
+        end
+      CODE
+    end
+
+    it 'adds no offense when using `sticky` data_consistency' do
+      expect_no_offenses(<<~CODE)
+        class SomeWorker
+          include ApplicationWorker
+
+          data_consistency :sticky
+          queue_namespace :pipeline_hooks
+          feature_category :continuous_integration
+          urgency :high
+        end
+      CODE
+    end
+
+    it 'adds no offense when using `delayed` data_consistency' do
+      expect_no_offenses(<<~CODE)
+        class SomeWorker
+          include ApplicationWorker
+
+          data_consistency :delayed
+          queue_namespace :pipeline_hooks
+          feature_category :continuous_integration
+          urgency :high
+        end
+      CODE
+    end
+
+    it 'adds no offense when worker is not an ApplicationWorker' do
+      expect_no_offenses(<<~CODE)
+        class SomeWorker
+          data_consistency :always
+          queue_namespace :pipeline_hooks
+          feature_category :continuous_integration
+          urgency :high
+        end
+      CODE
+    end
   end
 end