From 7dd8a57c259c1020785d9503e32be32f52120469 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Thu, 31 Aug 2023 10:08:05 +0200 Subject: [PATCH 1/4] GraphQL: Add lastLines argument to CiJobTrace.htmlSummary --- app/graphql/types/ci/job_trace_type.rb | 10 +++++++--- doc/api/graphql/reference/index.md | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/graphql/types/ci/job_trace_type.rb b/app/graphql/types/ci/job_trace_type.rb index a68e26106b8687..0081e3fe7c1803 100644 --- a/app/graphql/types/ci/job_trace_type.rb +++ b/app/graphql/types/ci/job_trace_type.rb @@ -8,10 +8,14 @@ class JobTraceType < BaseObject field :html_summary, GraphQL::Types::String, null: false, alpha: { milestone: '15.11' }, # As we want the option to change from 10 if needed - description: "HTML summary containing the last 10 lines of the trace." + description: "HTML summary containing the tail lines of the trace." do + argument :last_lines, Integer, + required: false, default_value: 10, + description: 'Number of tail lines to return.' + end - def html_summary - object.html(last_lines: 10).html_safe + def html_summary(last_lines:) + object.html(last_lines: last_lines).html_safe end end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 49bb9fdbefd021..86ae8b9abffa02 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -13866,11 +13866,23 @@ CI/CD variables for a GitLab instance. ### `CiJobTrace` -#### Fields +#### Fields with arguments + +##### `CiJobTrace.htmlSummary` + +HTML summary containing the tail lines of the trace. + +WARNING: +**Introduced** in 15.11. +This feature is an Experiment. It can be changed or removed at any time. + +Returns [`String!`](#string). + +###### Arguments | Name | Type | Description | | ---- | ---- | ----------- | -| `htmlSummary` **{warning-solid}** | [`String!`](#string) | **Introduced** in 15.11. This feature is an Experiment. It can be changed or removed at any time. HTML summary containing the last 10 lines of the trace. | +| `lastLines` | [`Int`](#int) | Number of tail lines to return. | ### `CiJobsDurationStatistics` -- GitLab From 2e1b257fb37a9858277eef24b4feafd777bc0e8a Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Mon, 4 Sep 2023 10:43:02 +0200 Subject: [PATCH 2/4] GraphQL: Fix N+1 issue with trace.html_summary --- app/graphql/resolvers/ci/all_jobs_resolver.rb | 11 ++++++++++- app/graphql/resolvers/ci/runner_jobs_resolver.rb | 11 ++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/app/graphql/resolvers/ci/all_jobs_resolver.rb b/app/graphql/resolvers/ci/all_jobs_resolver.rb index 8e9a1deb5419fd..54dd435d617d8f 100644 --- a/app/graphql/resolvers/ci/all_jobs_resolver.rb +++ b/app/graphql/resolvers/ci/all_jobs_resolver.rb @@ -39,9 +39,18 @@ def preloads browse_artifacts_path: [{ project: { namespace: [:route] } }], play_path: [{ project: { namespace: [:route] } }], web_path: [{ project: { namespace: [:route] } }], - tags: [:tags] + tags: [:tags], + trace: [{ project: [:namespace] }, :job_artifacts_trace] } end + + def nested_preloads + super.merge({ + trace: { + html_summary: [:trace_chunks] + } + }) + end end end end diff --git a/app/graphql/resolvers/ci/runner_jobs_resolver.rb b/app/graphql/resolvers/ci/runner_jobs_resolver.rb index 39908d8fd11332..c0a5057cddd070 100644 --- a/app/graphql/resolvers/ci/runner_jobs_resolver.rb +++ b/app/graphql/resolvers/ci/runner_jobs_resolver.rb @@ -44,9 +44,18 @@ def preloads play_path: [{ project: { namespace: [:route] } }], web_path: [{ project: { namespace: [:route] } }], short_sha: [:pipeline], - tags: [:tags] + tags: [:tags], + trace: [{ project: [:namespace] }, :job_artifacts_trace] } end + + def nested_preloads + super.merge({ + trace: { + html_summary: [:trace_chunks] + } + }) + end end end end -- GitLab From 59c4e4efa28729e23b2a5f8d9899528377d8b519 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Sep 2023 10:10:39 +0200 Subject: [PATCH 3/4] Clamp the number of lines returned --- app/graphql/types/ci/job_trace_type.rb | 8 +- doc/api/graphql/reference/index.md | 2 +- spec/graphql/types/ci/job_trace_type_spec.rb | 80 ++++++++++++++++++-- 3 files changed, 79 insertions(+), 11 deletions(-) diff --git a/app/graphql/types/ci/job_trace_type.rb b/app/graphql/types/ci/job_trace_type.rb index 0081e3fe7c1803..27c21fda5bf634 100644 --- a/app/graphql/types/ci/job_trace_type.rb +++ b/app/graphql/types/ci/job_trace_type.rb @@ -7,15 +7,15 @@ class JobTraceType < BaseObject graphql_name 'CiJobTrace' field :html_summary, GraphQL::Types::String, null: false, - alpha: { milestone: '15.11' }, # As we want the option to change from 10 if needed - description: "HTML summary containing the tail lines of the trace." do + alpha: { milestone: '15.11' }, + description: 'HTML summary containing the tail lines of the trace.' do argument :last_lines, Integer, required: false, default_value: 10, - description: 'Number of tail lines to return.' + description: 'Number of tail lines to return, up to a maximum of 100 lines.' end def html_summary(last_lines:) - object.html(last_lines: last_lines).html_safe + object.html(last_lines: last_lines.clamp(1, 100)).html_safe end end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 86ae8b9abffa02..299e116923420f 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -13882,7 +13882,7 @@ Returns [`String!`](#string). | Name | Type | Description | | ---- | ---- | ----------- | -| `lastLines` | [`Int`](#int) | Number of tail lines to return. | +| `lastLines` | [`Int`](#int) | Number of tail lines to return, up to a maximum of 100 lines. | ### `CiJobsDurationStatistics` diff --git a/spec/graphql/types/ci/job_trace_type_spec.rb b/spec/graphql/types/ci/job_trace_type_spec.rb index 71803aa9ece3f9..5f4f3097ace792 100644 --- a/spec/graphql/types/ci/job_trace_type_spec.rb +++ b/spec/graphql/types/ci/job_trace_type_spec.rb @@ -13,15 +13,83 @@ expect(described_class).to have_graphql_fields(*expected_fields) end - it 'shows the correct trace contents' do - job.trace.set('BUILD TRACE') + describe 'htmlSummary' do + subject(:resolved_field) { resolve_field(:html_summary, job.trace, args: args) } - expect_next_instance_of(Gitlab::Ci::Trace) do |trace| - expect(trace).to receive(:html).with(last_lines: 10).and_call_original + context 'when trace contains few lines' do + before do + job.trace.set('BUILD TRACE') + end + + context 'when last_lines is set to 10' do + let(:args) { { last_lines: 10 } } + + it 'shows the correct trace contents' do + expect_next_instance_of(Gitlab::Ci::Trace) do |trace| + expect(trace).to receive(:html).with(last_lines: 10).and_call_original + end + + is_expected.to eq('BUILD TRACE') + end + end end - resolved_field = resolve_field(:html_summary, job.trace) + context 'when trace contains many lines' do + before do + job.trace.set((1..200).map { |i| "Line #{i}" }.join("\n")) + end + + def expected_html_trace_contents(line_count) + "#{((200 - (line_count - 1))..200).map { |i| "Line #{i}" }.join('
')}" + end + + context 'when last_lines is not set' do + let(:args) { {} } + + it 'shows the last 10 lines of trace contents' do + expect_next_instance_of(Gitlab::Ci::Trace) do |trace| + expect(trace).to receive(:html).with(last_lines: 10).and_call_original + end + + is_expected.to eq expected_html_trace_contents(10) + end + end + + context 'when last_lines is set to a negative number' do + let(:args) { { last_lines: -10 } } - expect(resolved_field).to eq("BUILD TRACE") + it 'shows the last line of trace contents' do + expect_next_instance_of(Gitlab::Ci::Trace) do |trace| + expect(trace).to receive(:html).with(last_lines: 1).and_call_original + end + + is_expected.to eq expected_html_trace_contents(1) + end + end + + context 'when last_lines is set to 10' do + let(:args) { { last_lines: 10 } } + + it 'shows the correct trace contents' do + expect_next_instance_of(Gitlab::Ci::Trace) do |trace| + expect(trace).to receive(:html).with(last_lines: 10).and_call_original + end + + is_expected.to eq expected_html_trace_contents(10) + end + end + + context 'when last_lines is set to 150' do + let(:args) { { last_lines: 150 } } + + it 'shows the last 100 lines of trace contents' do + expect_next_instance_of(Gitlab::Ci::Trace) do |trace| + expect(trace).to receive(:html).with(last_lines: 100).and_call_original + end + + is_expected.to eq expected_html_trace_contents(100) + end + end + end end end -- GitLab From c483ad8857fddff70397545df3c3415fe97a1a07 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Sep 2023 16:25:33 +0200 Subject: [PATCH 4/4] Address MR review comment --- app/graphql/types/ci/job_trace_type.rb | 2 +- doc/api/graphql/reference/index.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/graphql/types/ci/job_trace_type.rb b/app/graphql/types/ci/job_trace_type.rb index 27c21fda5bf634..b959d37f327294 100644 --- a/app/graphql/types/ci/job_trace_type.rb +++ b/app/graphql/types/ci/job_trace_type.rb @@ -8,7 +8,7 @@ class JobTraceType < BaseObject field :html_summary, GraphQL::Types::String, null: false, alpha: { milestone: '15.11' }, - description: 'HTML summary containing the tail lines of the trace.' do + description: 'HTML summary that contains the tail lines of the trace.' do argument :last_lines, Integer, required: false, default_value: 10, description: 'Number of tail lines to return, up to a maximum of 100 lines.' diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 299e116923420f..a1fc29747439af 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -13870,7 +13870,7 @@ CI/CD variables for a GitLab instance. ##### `CiJobTrace.htmlSummary` -HTML summary containing the tail lines of the trace. +HTML summary that contains the tail lines of the trace. WARNING: **Introduced** in 15.11. -- GitLab