Merge branch 'feature/gb/observable-live-build-trace' into 'master'

Improve live traces observability using counters

Closes #227182

See merge request gitlab-org/gitlab!42359

app/models/ci/build_trace_chunk.rb

@@ -11,6 +11,8 @@ module Ci
 
     default_value_for :data_store, :redis
 
+    after_create { metrics.increment_trace_operation(operation: :chunked) }
+
     CHUNK_SIZE = 128.kilobytes
     WRITE_LOCK_RETRY = 10
     WRITE_LOCK_SLEEP = 0.01.seconds
@@ -182,6 +184,8 @@ module Ci
       end
 
       current_store.append_data(self, value, offset).then do |stored|
+        metrics.increment_trace_operation(operation: :appended)
+
         raise ArgumentError, 'Trace appended incorrectly' if stored != new_size
       end
 
@@ -205,5 +209,9 @@ module Ci
          retries: WRITE_LOCK_RETRY,
          sleep_sec: WRITE_LOCK_SLEEP }]
     end
+
+    def metrics
+      @metrics ||= ::Gitlab::Ci::Trace::Metrics.new
+    end
   end
 end

lib/gitlab/ci/trace/metrics.rb

@@ -6,17 +6,21 @@ module Gitlab
       class Metrics
         extend Gitlab::Utils::StrongMemoize
 
-        OPERATIONS = [:appended, :mutated, :overwrite, :accepted,
-                      :finalized, :discarded, :flaky].freeze
+        OPERATIONS = [:appended, :streamed, :chunked, :mutated, :overwrite,
+                      :accepted, :finalized, :discarded, :conflict].freeze
 
         def increment_trace_operation(operation: :unknown)
           unless OPERATIONS.include?(operation)
-            raise ArgumentError, 'unknown trace operation'
+            raise ArgumentError, "unknown trace operation: #{operation}"
           end
 
           self.class.trace_operations.increment(operation: operation)
         end
 
+        def increment_trace_bytes(size)
+          self.class.trace_bytes.increment(by: size.to_i)
+        end
+
         def self.trace_operations
           strong_memoize(:trace_operations) do
             name = :gitlab_ci_trace_operations_total
@@ -25,6 +29,15 @@ module Gitlab
             Gitlab::Metrics.counter(name, comment)
           end
         end
+
+        def self.trace_bytes
+          strong_memoize(:trace_bytes) do
+            name = :gitlab_ci_trace_bytes_total
+            comment = 'Total amount of build trace bytes transferred'
+
+            Gitlab::Metrics.counter(name, comment)
+          end
+        end
       end
     end
   end

lib/gitlab/ci/trace/stream.rb

@@ -8,7 +8,7 @@ module Gitlab
         BUFFER_SIZE = 4096
         LIMIT_SIZE = 500.kilobytes
 
-        attr_reader :stream
+        attr_reader :stream, :metrics
 
         delegate :close, :tell, :seek, :size, :url, :truncate, to: :stream, allow_nil: true
 
@@ -16,9 +16,10 @@ module Gitlab
 
         alias_method :present?, :valid?
 
-        def initialize
+        def initialize(metrics = Trace::Metrics.new)
           @stream = yield
           @stream&.binmode
+          @metrics = metrics
         end
 
         def valid?
@@ -43,6 +44,9 @@ module Gitlab
         def append(data, offset)
           data = data.force_encoding(Encoding::BINARY)
 
+          metrics.increment_trace_operation(operation: :streamed)
+          metrics.increment_trace_bytes(data.bytesize)
+
           stream.seek(offset, IO::SEEK_SET)
           stream.write(data)
           stream.truncate(offset + data.bytesize)

spec/lib/gitlab/ci/trace/stream_spec.rb

@@ -151,6 +151,28 @@ RSpec.describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
 
       it_behaves_like 'appends'
     end
+
+    describe 'metrics' do
+      let(:metrics) { spy('metrics') }
+      let(:io) { StringIO.new }
+      let(:stream) { described_class.new(metrics) { io } }
+
+      it 'increments trace streamed operation' do
+        stream.append(+'123456', 0)
+
+        expect(metrics)
+          .to have_received(:increment_trace_operation)
+          .with(operation: :streamed)
+      end
+
+      it 'increments trace bytes counter' do
+        stream.append(+'123456', 0)
+
+        expect(metrics)
+          .to have_received(:increment_trace_bytes)
+          .with(6)
+      end
+    end
   end
 
   describe '#set' do

spec/models/ci/build_trace_chunk_spec.rb

@@ -21,6 +21,25 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
     stub_artifacts_object_storage
   end
 
+  describe 'chunk creation' do
+    let(:metrics) { spy('metrics') }
+
+    before do
+      allow(::Gitlab::Ci::Trace::Metrics)
+        .to receive(:new)
+        .and_return(metrics)
+    end
+
+    it 'increments trace operation chunked metric' do
+      build_trace_chunk.save!
+
+      expect(metrics)
+        .to have_received(:increment_trace_operation)
+        .with(operation: :chunked)
+        .once
+    end
+  end
+
   context 'FastDestroyAll' do
     let(:parent) { create(:project) }
     let(:pipeline) { create(:ci_pipeline, project: parent) }
@@ -346,6 +365,24 @@ RSpec.describe Ci::BuildTraceChunk, :clean_gitlab_redis_shared_state do
         it_behaves_like 'Scheduling no sidekiq worker'
       end
     end
+
+    describe 'append metrics' do
+      let(:metrics) { spy('metrics') }
+
+      before do
+        allow(::Gitlab::Ci::Trace::Metrics)
+          .to receive(:new)
+          .and_return(metrics)
+      end
+
+      it 'increments trace operation appended metric' do
+        build_trace_chunk.append('123456', 0)
+
+        expect(metrics)
+          .to have_received(:increment_trace_operation)
+          .with(operation: :appended)
+      end
+    end
   end
 
   describe '#truncate' do

spec/services/ci/update_build_state_service_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Ci::UpdateBuildStateService do
   let(:build) { create(:ci_build, :running, pipeline: pipeline) }
   let(:metrics) { spy('metrics') }
 
-  subject { described_class.new(build, params, metrics) }
+  subject { described_class.new(build, params) }
 
   before do
     stub_feature_flags(ci_enable_live_trace: true)
@@ -31,7 +31,7 @@ RSpec.describe Ci::UpdateBuildStateService do
       end
 
       it 'does not increment finalized trace metric' do
-        subject.execute
+        execute_with_stubbed_metrics!
 
         expect(metrics)
           .not_to have_received(:increment_trace_operation)
@@ -50,11 +50,16 @@ RSpec.describe Ci::UpdateBuildStateService do
     context 'when request payload carries a trace' do
       let(:params) { { state: 'success', trace: 'overwritten' } }
 
-      it 'overwrites a trace and updates trace operation metric' do
+      it 'overwrites a trace' do
         result = subject.execute
 
         expect(build.trace.raw).to eq 'overwritten'
         expect(result.status).to eq 200
+      end
+
+      it 'updates overwrite operation metric' do
+        execute_with_stubbed_metrics!
+
         expect(metrics)
           .to have_received(:increment_trace_operation)
           .with(operation: :overwrite)
@@ -96,7 +101,7 @@ RSpec.describe Ci::UpdateBuildStateService do
       end
 
       it 'increments trace finalized operation metric' do
-        subject.execute
+        execute_with_stubbed_metrics!
 
         expect(metrics)
           .to have_received(:increment_trace_operation)
@@ -130,7 +135,7 @@ RSpec.describe Ci::UpdateBuildStateService do
       end
 
       it 'increments trace accepted operation metric' do
-        subject.execute
+        execute_with_stubbed_metrics!
 
         expect(metrics)
           .to have_received(:increment_trace_operation)
@@ -172,7 +177,7 @@ RSpec.describe Ci::UpdateBuildStateService do
         end
 
         it 'increments discarded traces metric' do
-          subject.execute
+          execute_with_stubbed_metrics!
 
           expect(metrics)
             .to have_received(:increment_trace_operation)
@@ -180,7 +185,7 @@ RSpec.describe Ci::UpdateBuildStateService do
         end
 
         it 'does not increment finalized trace metric' do
-          subject.execute
+          execute_with_stubbed_metrics!
 
           expect(metrics)
             .not_to have_received(:increment_trace_operation)
@@ -203,7 +208,7 @@ RSpec.describe Ci::UpdateBuildStateService do
         end
 
         it 'increments conflict trace metric' do
-          subject.execute
+          execute_with_stubbed_metrics!
 
           expect(metrics)
             .to have_received(:increment_trace_operation)
@@ -224,4 +229,10 @@ RSpec.describe Ci::UpdateBuildStateService do
       end
     end
   end
+
+  def execute_with_stubbed_metrics!
+    described_class
+      .new(build, params, metrics)
+      .execute
+  end
 end