Merge branch '33658-reducing-build-trace-update-frequency' into 'master'

Implement sending update frequency of build traces to runner

See merge request gitlab-org/gitlab!20841

app/controllers/projects/jobs_controller.rb

@@ -51,6 +51,8 @@ class Projects::JobsController < Projects::ApplicationController
     build.trace.read do |stream|
       respond_to do |format|
         format.json do
+          build.trace.being_watched!
+
           # TODO: when the feature flag is removed we should not pass
           # content_format to serialize method.
           content_format = Feature.enabled?(:job_log_json, @project, default_enabled: true) ? :json : :html

changelogs/unreleased/33658-reducing-build-trace-update-frequency.yml

+---
+title: Request less frequent updates from Runner when job log is not being watched
+merge_request: 20841
+author:
+type: performance

lib/api/runner.rb

@@ -200,6 +200,10 @@ module API
         status 202
         header 'Job-Status', job.status
         header 'Range', "0-#{stream_size}"
+
+        if Feature.enabled?(:runner_job_trace_update_interval_header, default_enabled: true)
+          header 'X-GitLab-Trace-Update-Interval', job.trace.update_interval.to_s
+        end
       end
 
       desc 'Authorize artifacts uploading for job' do

lib/gitlab/ci/trace.rb

@@ -9,6 +9,10 @@ module Gitlab
       LOCK_TTL = 10.minutes
       LOCK_RETRIES = 2
       LOCK_SLEEP = 0.001.seconds
+      WATCH_FLAG_TTL = 10.seconds
+
+      UPDATE_FREQUENCY_DEFAULT = 30.seconds
+      UPDATE_FREQUENCY_WHEN_BEING_WATCHED = 3.seconds
 
       ArchiveError = Class.new(StandardError)
       AlreadyArchivedError = Class.new(StandardError)
@@ -119,6 +123,22 @@ module Gitlab
         end
       end
 
+      def update_interval
+        being_watched? ? UPDATE_FREQUENCY_WHEN_BEING_WATCHED : UPDATE_FREQUENCY_DEFAULT
+      end
+
+      def being_watched!
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set(being_watched_cache_key, true, ex: WATCH_FLAG_TTL)
+        end
+      end
+
+      def being_watched?
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.exists(being_watched_cache_key)
+        end
+      end
+
       private
 
       def unsafe_write!(mode, &blk)
@@ -236,6 +256,10 @@ module Gitlab
       def trace_artifact
         job.job_artifacts_trace
       end
+
+      def being_watched_cache_key
+        "gitlab:ci:trace:#{job.id}:watched"
+      end
     end
   end
 end

spec/controllers/projects/jobs_controller_spec.rb

@@ -556,6 +556,12 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
         expect(json_response['status']).to eq job.status
         expect(json_response['lines']).to eq [{ 'content' => [{ 'text' => 'BUILD TRACE' }], 'offset' => 0 }]
       end
+
+      it 'sets being-watched flag for the job' do
+        expect(response).to have_gitlab_http_status(:ok)
+
+        expect(job.trace.being_watched?).to be(true)
+      end
     end
 
     context 'when job has no traces' do

spec/lib/gitlab/ci/trace_spec.rb

@@ -26,4 +26,66 @@ describe Gitlab::Ci::Trace, :clean_gitlab_redis_shared_state do
 
     it_behaves_like 'trace with enabled live trace feature'
   end
+
+  describe '#update_interval' do
+    context 'it is not being watched' do
+      it 'returns 30 seconds' do
+        expect(trace.update_interval).to eq(30.seconds)
+      end
+    end
+
+    context 'it is being watched' do
+      before do
+        trace.being_watched!
+      end
+
+      it 'returns 3 seconds' do
+        expect(trace.update_interval).to eq(3.seconds)
+      end
+    end
+  end
+
+  describe '#being_watched!' do
+    let(:cache_key) { "gitlab:ci:trace:#{build.id}:watched" }
+
+    it 'sets gitlab:ci:trace:<job.id>:watched in redis' do
+      trace.being_watched!
+
+      result = Gitlab::Redis::SharedState.with do |redis|
+        redis.exists(cache_key)
+      end
+
+      expect(result).to eq(true)
+    end
+
+    it 'updates the expiry of gitlab:ci:trace:<job.id>:watched in redis', :clean_gitlab_redis_shared_state do
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.set(cache_key, true, ex: 4.seconds)
+      end
+
+      expect do
+        trace.being_watched!
+      end.to change { Gitlab::Redis::SharedState.with { |redis| redis.pttl(cache_key) } }
+    end
+  end
+
+  describe '#being_watched?' do
+    context 'gitlab:ci:trace:<job.id>:watched in redis is set', :clean_gitlab_redis_shared_state do
+      before do
+        Gitlab::Redis::SharedState.with do |redis|
+          redis.set("gitlab:ci:trace:#{build.id}:watched", true)
+        end
+      end
+
+      it 'returns true' do
+        expect(trace.being_watched?).to be(true)
+      end
+    end
+
+    context 'gitlab:ci:trace:<job.id>:watched in redis is not set' do
+      it 'returns false' do
+        expect(trace.being_watched?).to be(false)
+      end
+    end
+  end
 end

spec/requests/api/runner_spec.rb

@@ -1154,6 +1154,7 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
           expect(job.reload.trace.raw).to eq 'BUILD TRACE appended'
           expect(response.header).to have_key 'Range'
           expect(response.header).to have_key 'Job-Status'
+          expect(response.header).to have_key 'X-GitLab-Trace-Update-Interval'
         end
 
         context 'when job has been updated recently' do
@@ -1291,6 +1292,41 @@ describe API::Runner, :clean_gitlab_redis_shared_state do
             expect(response.header['Job-Status']).to eq 'canceled'
           end
         end
+
+        context 'when build trace is being watched' do
+          before do
+            job.trace.being_watched!
+          end
+
+          it 'returns X-GitLab-Trace-Update-Interval as 3' do
+            patch_the_trace
+
+            expect(response.status).to eq 202
+            expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('3')
+          end
+        end
+
+        context 'when build trace is not being watched' do
+          it 'returns X-GitLab-Trace-Update-Interval as 30' do
+            patch_the_trace
+
+            expect(response.status).to eq 202
+            expect(response.header['X-GitLab-Trace-Update-Interval']).to eq('30')
+          end
+        end
+
+        context 'when feature flag runner_job_trace_update_interval_header is disabled' do
+          before do
+            stub_feature_flags(runner_job_trace_update_interval_header: { enabled: false })
+          end
+
+          it 'does not return X-GitLab-Trace-Update-Interval header' do
+            patch_the_trace
+
+            expect(response.status).to eq 202
+            expect(response.header).not_to have_key 'X-GitLab-Trace-Update-Interval'
+          end
+        end
       end
 
       context 'when Runner makes a force-patch' do