Merge branch 'use-ansi2json-for-job-logs' into 'master'

Use Ansi2json via feature flag for job logs

See merge request gitlab-org/gitlab!18134

app/controllers/projects/jobs_controller.rb

@@ -11,7 +11,7 @@ class Projects::JobsController < Projects::ApplicationController
   before_action :authorize_erase_build!, only: [:erase]
   before_action :authorize_use_build_terminal!, only: [:terminal, :terminal_websocket_authorize]
   before_action :verify_api_request!, only: :terminal_websocket_authorize
-  before_action only: [:trace] do
+  before_action only: [:show] do
     push_frontend_feature_flag(:job_log_json)
   end
 
@@ -67,38 +67,27 @@ class Projects::JobsController < Projects::ApplicationController
   # rubocop: enable CodeReuse/ActiveRecord
 
   def trace
-    if Feature.enabled?(:job_log_json, @project)
-      json_trace
-    else
-      html_trace
-    end
-  end
-
-  def html_trace
     build.trace.read do |stream|
       respond_to do |format|
         format.json do
-          result = {
-            id: @build.id, status: @build.status, complete: @build.complete?
-          }
-
-          if stream.valid?
-            stream.limit
-            state = params[:state].presence
-            trace = stream.html_with_state(state)
-            result.merge!(trace.to_h)
-          end
-
-          render json: result
+          # TODO: when the feature flag is removed we should not pass
+          # content_format to serialize method.
+          content_format = Feature.enabled?(:job_log_json, @project) ? :json : :html
+
+          build_trace = Ci::BuildTrace.new(
+            build: @build,
+            stream: stream,
+            state: params[:state],
+            content_format: content_format)
+
+          render json: BuildTraceSerializer
+            .new(project: @project, current_user: @current_user)
+            .represent(build_trace)
         end
       end
     end
   end
 
-  def json_trace
-    # will be implemented with https://gitlab.com/gitlab-org/gitlab-foss/issues/66454
-  end
-
   def retry
     return respond_422 unless @build.retryable?
 

app/models/ci/build_trace.rb

+# frozen_string_literal: true
+
+module Ci
+  class BuildTrace
+    CONVERTERS = {
+      html: Gitlab::Ci::Ansi2html,
+      json: Gitlab::Ci::Ansi2json
+    }.freeze
+
+    attr_reader :trace, :build
+
+    delegate :state, :append, :truncated, :offset, :size, :total, to: :trace, allow_nil: true
+    delegate :id, :status, :complete?, to: :build, prefix: true
+
+    def initialize(build:, stream:, state:, content_format:)
+      @build = build
+      @content_format = content_format
+
+      if stream.valid?
+        stream.limit
+        @trace = CONVERTERS.fetch(content_format).convert(stream.stream, state)
+      end
+    end
+
+    def json?
+      @content_format == :json
+    end
+
+    def html?
+      @content_format == :html
+    end
+
+    def json_lines
+      @trace&.lines if json?
+    end
+
+    def html_lines
+      @trace&.html if html?
+    end
+  end
+end

app/serializers/build_trace_entity.rb

+# frozen_string_literal: true
+
+class BuildTraceEntity < Grape::Entity
+  expose :build_id, as: :id
+  expose :build_status, as: :status
+  expose :build_complete?, as: :complete
+
+  expose :state
+  expose :append
+  expose :truncated
+  expose :offset
+  expose :size
+  expose :total
+
+  expose :json_lines, as: :lines, if: ->(*) { object.json? }
+  expose :html_lines, as: :html, if: ->(*) { object.html? }
+end

app/serializers/build_trace_serializer.rb

+# frozen_string_literal: true
+
+class BuildTraceSerializer < BaseSerializer
+  entity BuildTraceEntity
+end

changelogs/unreleased/use-ansi2json-for-job-logs.yml

+---
+title: Use new Ansi2json job log converter via feature flag
+merge_request: 18134
+author:
+type: added

lib/gitlab/ci/ansi2html.rb

@@ -178,6 +178,8 @@ module Gitlab
 
           close_open_tags
 
+          # TODO: replace OpenStruct with a better type
+          # https://gitlab.com/gitlab-org/gitlab/issues/34305
           OpenStruct.new(
             html: @out.force_encoding(Encoding.default_external),
             state: state,

lib/gitlab/ci/ansi2json/converter.rb

@@ -37,6 +37,8 @@ module Gitlab
 
           flush_current_line
 
+          # TODO: replace OpenStruct with a better type
+          # https://gitlab.com/gitlab-org/gitlab/issues/34305
           OpenStruct.new(
             lines: @lines,
             state: @state.encode,

lib/gitlab/ci/trace/stream.rb

@@ -63,10 +63,6 @@ module Gitlab
           end.force_encoding(Encoding.default_external)
         end
 
-        def html_with_state(state = nil)
-          ::Gitlab::Ci::Ansi2html.convert(stream, state)
-        end
-
         def html(last_lines: nil)
           text = raw(last_lines: last_lines)
           buffer = StringIO.new(text)

spec/controllers/projects/jobs_controller_spec.rb

@@ -527,6 +527,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
 
   describe 'GET trace.json' do
     before do
+      stub_feature_flags(job_log_json: true)
       get_trace
     end
 
@@ -535,8 +536,119 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
 
       it 'returns a trace' do
         expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('job/build_trace')
         expect(json_response['id']).to eq job.id
         expect(json_response['status']).to eq job.status
+        expect(json_response['state']).to be_present
+        expect(json_response['append']).not_to be_nil
+        expect(json_response['truncated']).not_to be_nil
+        expect(json_response['size']).to be_present
+        expect(json_response['total']).to be_present
+        expect(json_response['lines'].count).to be_positive
+      end
+    end
+
+    context 'when job has a trace' do
+      let(:job) { create(:ci_build, :trace_live, pipeline: pipeline) }
+
+      it 'returns a trace' do
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('job/build_trace')
+        expect(json_response['id']).to eq job.id
+        expect(json_response['status']).to eq job.status
+        expect(json_response['lines']).to eq [{ 'content' => [{ 'text' => 'BUILD TRACE' }], 'offset' => 0 }]
+      end
+    end
+
+    context 'when job has no traces' do
+      let(:job) { create(:ci_build, pipeline: pipeline) }
+
+      it 'returns no traces' do
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('job/build_trace')
+        expect(json_response['id']).to eq job.id
+        expect(json_response['status']).to eq job.status
+        expect(json_response['lines']).to be_nil
+      end
+    end
+
+    context 'when job has a trace with ANSI sequence and Unicode' do
+      let(:job) { create(:ci_build, :unicode_trace_live, pipeline: pipeline) }
+
+      it 'returns a trace with Unicode' do
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('job/build_trace')
+        expect(json_response['id']).to eq job.id
+        expect(json_response['status']).to eq job.status
+        expect(json_response['lines'].flat_map {|l| l['content'].map { |c| c['text'] } }).to include("ヾ(´༎ຶД༎ຶ`)ﾉ")
+      end
+    end
+
+    context 'when trace artifact is in ObjectStorage' do
+      let(:url) { 'http://object-storage/trace' }
+      let(:file_path) { expand_fixture_path('trace/sample_trace') }
+      let!(:job) { create(:ci_build, :success, :trace_artifact, pipeline: pipeline) }
+
+      before do
+        allow_any_instance_of(JobArtifactUploader).to receive(:file_storage?) { false }
+        allow_any_instance_of(JobArtifactUploader).to receive(:url) { url }
+        allow_any_instance_of(JobArtifactUploader).to receive(:size) { File.size(file_path) }
+      end
+
+      context 'when there are no network issues' do
+        before do
+          stub_remote_url_206(url, file_path)
+
+          get_trace
+        end
+
+        it 'returns a trace' do
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response['id']).to eq job.id
+          expect(json_response['status']).to eq job.status
+          expect(json_response['lines'].count).to be_positive
+        end
+      end
+
+      context 'when there is a network issue' do
+        before do
+          stub_remote_url_500(url)
+        end
+
+        it 'returns a trace' do
+          expect { get_trace }.to raise_error(Gitlab::HttpIO::FailedToGetChunkError)
+        end
+      end
+    end
+
+    def get_trace
+      get :trace,
+        params: {
+          namespace_id: project.namespace,
+          project_id: project,
+          id: job.id
+        },
+        format: :json
+    end
+  end
+
+  describe 'GET legacy trace.json' do
+    before do
+      get_trace
+    end
+
+    context 'when job has a trace artifact' do
+      let(:job) { create(:ci_build, :trace_artifact, pipeline: pipeline) }
+
+      it 'returns a trace' do
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['id']).to eq job.id
+        expect(json_response['status']).to eq job.status
+        expect(json_response['state']).to be_present
+        expect(json_response['append']).not_to be_nil
+        expect(json_response['truncated']).not_to be_nil
+        expect(json_response['size']).to be_present
+        expect(json_response['total']).to be_present
         expect(json_response['html']).to eq(job.trace.html)
       end
     end
@@ -612,12 +724,13 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
     end
 
     def get_trace
-      get :trace, params: {
-                    namespace_id: project.namespace,
-                    project_id: project,
-                    id: job.id
-                  },
-                  format: :json
+      get :trace,
+        params: {
+          namespace_id: project.namespace,
+          project_id: project,
+          id: job.id
+        },
+        format: :json
     end
   end
 

spec/features/projects/jobs/permissions_spec.rb

@@ -10,6 +10,7 @@ describe 'Project Jobs Permissions' do
   let!(:job) { create(:ci_build, :running, :coverage, :trace_artifact, pipeline: pipeline) }
 
   before do
+    stub_feature_flags(job_log_json: true)
     sign_in(user)
 
     project.enable_ci
@@ -69,7 +70,7 @@ describe 'Project Jobs Permissions' do
         it_behaves_like 'recent job page details responds with status', 200 do
           it 'renders job details', :js do
             expect(page).to have_content "Job ##{job.id}"
-            expect(page).to have_css '.js-build-trace'
+            expect(page).to have_css '.log-line'
           end
         end
 

spec/fixtures/api/schemas/job/build_trace.json

+{
+  "description": "Build trace",
+  "type": "object",
+  "required": [
+    "id",
+    "status",
+    "complete",
+    "state",
+    "append",
+    "truncated",
+    "offset",
+    "size",
+    "total"
+  ],
+  "properties": {
+    "id": { "type": "integer" },
+    "status": { "type": "string" },
+    "complete": { "type": "boolean" },
+    "state": { "type": ["string", "null"] },
+    "append": { "type": ["boolean", "null"] },
+    "truncated": { "type": ["boolean", "null"] },
+    "offset": { "type": ["integer", "null"] },
+    "size": { "type": ["integer", "null"] },
+    "total": { "type": ["integer", "null"] },
+    "html": { "type": ["string", "null"] },
+    "lines": {
+      "type": ["array", "null"],
+      "items": { "$ref": "./build_trace_line.json" }
+    }
+  }
+}

spec/fixtures/api/schemas/job/build_trace_line.json

+{
+  "description": "Build trace line",
+  "type": "object",
+  "required": [
+    "offset",
+    "content"
+  ],
+  "properties": {
+    "offset": { "type": "integer" },
+    "content": {
+      "type": "array",
+      "items": { "$ref": "./build_trace_line_content.json" }
+    },
+    "section": "string",
+    "section_header": "boolean",
+    "section_duration": "string"
+  }
+}

spec/fixtures/api/schemas/job/build_trace_line_content.json

+{
+  "description": "Build trace line content",
+  "type": "object",
+  "required": [
+    "text"
+  ],
+  "properties": {
+    "text": { "type": "string" },
+    "style": { "type": "string" }
+  }
+}

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

@@ -248,60 +248,6 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
     end
   end
 
-  describe '#html_with_state' do
-    shared_examples_for 'html_with_states' do
-      it 'returns html content with state' do
-        result = stream.html_with_state
-
-        expect(result.html).to eq("<span>1234</span>")
-      end
-
-      context 'follow-up state' do
-        let!(:last_result) { stream.html_with_state }
-
-        before do
-          data_stream.seek(4, IO::SEEK_SET)
-          data_stream.write("5678")
-          stream.seek(0)
-        end
-
-        it "returns appended trace" do
-          result = stream.html_with_state(last_result.state)
-
-          expect(result.append).to be_truthy
-          expect(result.html).to eq("<span>5678</span>")
-        end
-      end
-    end
-
-    context 'when stream is StringIO' do
-      let(:data_stream) do
-        StringIO.new("1234")
-      end
-
-      let(:stream) do
-        described_class.new { data_stream }
-      end
-
-      it_behaves_like 'html_with_states'
-    end
-
-    context 'when stream is ChunkedIO' do
-      let(:data_stream) do
-        Gitlab::Ci::Trace::ChunkedIO.new(build).tap do |chunked_io|
-          chunked_io.write("1234")
-          chunked_io.seek(0, IO::SEEK_SET)
-        end
-      end
-
-      let(:stream) do
-        described_class.new { data_stream }
-      end
-
-      it_behaves_like 'html_with_states'
-    end
-  end
-
   describe '#html' do
     shared_examples_for 'htmls' do
       it "returns html" do

spec/models/ci/build_trace_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Ci::BuildTrace do
+  let(:build) { build_stubbed(:ci_build) }
+  let(:state) { nil }
+  let(:data) { StringIO.new('the-stream') }
+
+  let(:stream) do
+    Gitlab::Ci::Trace::Stream.new { data }
+  end
+
+  subject { described_class.new(build: build, stream: stream, state: state, content_format: content_format) }
+
+  shared_examples 'delegates methods' do
+    it { is_expected.to delegate_method(:state).to(:trace) }
+    it { is_expected.to delegate_method(:append).to(:trace) }
+    it { is_expected.to delegate_method(:truncated).to(:trace) }
+    it { is_expected.to delegate_method(:offset).to(:trace) }
+    it { is_expected.to delegate_method(:size).to(:trace) }
+    it { is_expected.to delegate_method(:total).to(:trace) }
+    it { is_expected.to delegate_method(:id).to(:build).with_prefix }
+    it { is_expected.to delegate_method(:status).to(:build).with_prefix }
+    it { is_expected.to delegate_method(:complete?).to(:build).with_prefix }
+  end
+
+  context 'with :json content format' do
+    let(:content_format) { :json }
+
+    it_behaves_like 'delegates methods'
+
+    it { is_expected.to be_json }
+
+    it 'returns formatted trace' do
+      expect(subject.trace.lines).to eq([
+        { offset: 0, content: [{ text: 'the-stream' }] }
+      ])
+    end
+  end
+
+  context 'with :html content format' do
+    let(:content_format) { :html }
+
+    it_behaves_like 'delegates methods'
+
+    it { is_expected.to be_html }
+
+    it 'returns formatted trace' do
+      expect(subject.trace.html).to eq('<span>the-stream</span>')
+    end
+  end
+end

spec/serializers/build_trace_entity_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe BuildTraceEntity do
+  let(:build) { build_stubbed(:ci_build) }
+  let(:request) { double('request') }
+
+  let(:stream) do
+    Gitlab::Ci::Trace::Stream.new do
+      StringIO.new('the-trace')
+    end
+  end
+
+  let(:build_trace) do
+    Ci::BuildTrace.new(build: build, stream: stream, content_format: content_format, state: nil)
+  end
+
+  let(:entity) do
+    described_class.new(build_trace, request: request)
+  end
+
+  subject { entity.as_json }
+
+  shared_examples 'includes build and trace metadata' do
+    it 'includes build attributes' do
+      expect(subject[:id]).to eq(build.id)
+      expect(subject[:status]).to eq(build.status)
+      expect(subject[:complete]).to eq(build.complete?)
+    end
+
+    it 'includes trace metadata' do
+      expect(subject).to include(:state)
+      expect(subject).to include(:append)
+      expect(subject).to include(:truncated)
+      expect(subject).to include(:offset)
+      expect(subject).to include(:size)
+      expect(subject).to include(:total)
+    end
+  end
+
+  context 'when content format is :json' do
+    let(:content_format) { :json }
+
+    it_behaves_like 'includes build and trace metadata'
+
+    it 'includes the trace content in json' do
+      expect(subject[:lines]).to eq([
+        { offset: 0, content: [{ text: 'the-trace' }] }
+      ])
+    end
+  end
+
+  context 'when content format is :html' do
+    let(:content_format) { :html }
+
+    it_behaves_like 'includes build and trace metadata'
+
+    it 'includes the trace content in json' do
+      expect(subject[:html]).to eq('<span>the-trace</span>')
+    end
+  end
+end