Introduce and use lean findings finder in CompareSecurityReportsService

Lorenz van Herwaardenrequested to merge
poc/lean-mr-security-report-comparison into master

What does this MR do and why?

CompareSecurityReportsService loads findings for the base and head pipeline via Security::FindingsFinder, then SecurityFindingsReportsComparer compares them by uuid. The comparison only needs uuid, severity, and scanner_id, but today it loads every column (including the large finding_data JSON) and every association: scan, scanner, pipeline entities, vulnerability, state transitions, issue links, external issue links, merge request links, feedback. Vulnerabilities that flip between detected and resolved carry thousands of state transitions, so a single comparison can pull tens of thousands of rows into Ruby to compare a few hundred uuids, on every report refresh.

Behind a new default-off feature flag, lean_mr_security_report_comparison:

  • Security::FindingsFinder gets a lean param that selects only id, scanner_id, severity, uuid, context_unaware_uuid, partition_number and skips all preloads.
  • The service passes lean: true and skips with_api_scopes and the two policy preloads.
  • The comparer reloads only the capped added and fixed findings (at most 25 per list) with their associations, and runs policy checks on them before serialization.

Two behavior-neutral changes were needed for the lean finder:

  • Partial scan filtering now compares scanner_id against the project's scanner ids instead of scanner external ids, since lean findings don't have the scanner loaded.
  • The manual resolution check uses the comparison's report type instead of each finding's report type (all findings in a comparison share it).

Evidence

  • Flag on is never slower end to end than flag off.
  • With the full data set, flag on is about 10 s faster and loads 15,000 state transitions instead of 588,000.
  • hydrate reloads at most 25 added + 25 fixed findings.
  • Serialized output is identical between flag on and off in every run.
  • The reload DB query takes 16 ms.
  • Most of the end-to-end time is serializing state transitions, equally in both modes. Part of it is an N+1 on the transition author, which exists on master. Tracked in #631157.
Loading step only (local)

GDK, SAST comparison, 53 findings loaded across both pipelines, each vulnerability given 300 state transitions.

Flag off Flag on
Findings loaded 53 53
State transitions loaded with them 19,927 0
Columns per finding 12, including finding_data 7 (6 since dropping scan_id)
Queries (CI / main / sec) 13 / 8 / 15 10 / 0 / 6
CPU 0.79 s 0.02 s
Objects allocated 812,000 23,000
End-to-end benchmark (local)

GDK, run against MR head commit 1ac3c978. Flag off and flag on, one warm-up run each, query cache cleared before each run. SAST, every vulnerability has 300 state transitions. Runs repeated 2-3 times; wall times stable within about 2 s.

Scenario Flag Wall Allocations Queries (ci / sec / main) Findings / vulnerabilities / state transitions loaded
A: 50 shared, 3 fixed, 3 added off 3.7-4.0 s 10.5 M 24 / 31 / 11 106 / 106 / 31,800
on 3.1-3.2 s 10.0 M 24 / 33 / 11 112 / 6 / 1,800
B, worst case for lean: 30 fixed, 30 added, nothing shared off 25.8-27.9 s 82.2 M 24 / 75 / 12 60 / 60 / 18,000
on 25.4-25.8 s 82.0 M 24 / 77 / 12 110 / 50 / 15,000
C: 950 shared, 30 fixed, 30 added, security_mr_reports_tab_full_data_set on off 36.7-36.9 s 91.5 M 24 / 75 / 13 1,960 / 1,960 / 588,000
on 26.1-26.9 s 82.1 M 24 / 77 / 13 2,010 / 50 / 15,000
C with security_mr_reports_tab_full_data_set off off 1.1 s 1.3 M 18 / 16 / 5 312 / 312 / 93,600
on 0.02 s 0.03 M 12 / 6 / 0 312 / 0 / 0
  • Flag-on counts include the up to 50 reloaded findings (lean rows plus reloads).
  • The hydrated set is a subset of what flag off loads, so flag on never loads more state transitions; the only added cost is 2 extra sec queries.
  • In C with the full data set flag off, neither mode shows added or fixed findings because of the existing per-severity cap (unrelated to this MR).
  • Seeded head-only findings all have vulnerabilities, which is pessimistic for flag on; real added findings usually have none.
Seed and benchmark scripts

Run with bin/rails runner in the GDK.

seed_lean_comparison.rb

# Usage: SHARED=50 FIXED=3 ADDED=3 TRANSITIONS=300 NAME=lean-bench-a bin/rails runner seed_lean_comparison.rb
# Creates a project with a base and head pipeline, each with one latest successful SAST scan.
#   SHARED findings appear in both pipelines, FIXED only in base, ADDED only in head.
# Every finding has a vulnerability with TRANSITIONS state transitions.
require 'sidekiq/testing'
Sidekiq::Testing.fake!

shared = ENV.fetch('SHARED').to_i
fixed = ENV.fetch('FIXED').to_i
added = ENV.fetch('ADDED').to_i
transitions = ENV.fetch('TRANSITIONS', '300').to_i
name = ENV.fetch('NAME')

user = User.find_by_username!('root')
severities = %i[critical high medium low info unknown]

project = FactoryBot.create(:project, name: name, path: name, namespace: user.namespace, creator: user)
project.add_owner(user) unless project.owner == user
scanner = FactoryBot.create(:vulnerabilities_scanner, project: project, external_id: 'semgrep', name: 'Semgrep')

def pipeline_with_scan(project, ref)
  pipeline = FactoryBot.create(:ci_pipeline, :success, project: project, ref: ref, sha: SecureRandom.hex(20))
  build = FactoryBot.create(:ci_build, :success, pipeline: pipeline, project: project, name: 'semgrep-sast')
  scan = FactoryBot.create(:security_scan, :latest_successful, scan_type: :sast, build: build,
    pipeline: pipeline, project: project, findings_partition_number: Security::Finding.active_partition_number)
  [pipeline, scan]
end

base_pipeline, base_scan = pipeline_with_scan(project, 'master')
head_pipeline, head_scan = pipeline_with_scan(project, 'feature')

now = Time.current
make = ->(i, scans) do
  severity = severities[i % severities.size]
  vuln = FactoryBot.create(:vulnerability, :with_finding, :detected, project: project, author: user,
    severity: severity, report_type: :sast)
  vuln.finding.update_columns(scanner_id: scanner.id)

  rows = Array.new(transitions) do |t|
    from, to = t.even? ? [1, 3] : [3, 1] # detected <-> resolved
    { vulnerability_id: vuln.id, project_id: project.id, from_state: from, to_state: to,
      author_id: user.id, created_at: now, updated_at: now }
  end
  Vulnerabilities::StateTransition.insert_all(rows) if rows.any?

  scans.each do |scan|
    FactoryBot.create(:security_finding, :with_finding_data, scan: scan, scanner: scanner,
      severity: severity, uuid: vuln.finding.uuid, deduplicated: true)
  end
end

i = 0
shared.times { make.call(i += 1, [base_scan, head_scan]) }
fixed.times { make.call(i += 1, [base_scan]) }
added.times { make.call(i += 1, [head_scan]) }

puts "PROJECT_ID=#{project.id} BASE=#{base_pipeline.id} HEAD=#{head_pipeline.id}"

bench_lean_comparison.rb

# Usage (GDK):
#   PROJECT_ID=1 BASE=10 HEAD=11 REPORT_TYPE=sast FULL_DATA_SET=0 \
#     bin/rails runner /path/to/bench_lean_comparison.rb
#
# Measures the whole CompareSecurityReportsService#execute (finder -> comparer -> hydrate -> serializer),
# which is exactly what the reactive cache worker runs, with the lean flag off and on.

project = Project.find(ENV.fetch('PROJECT_ID'))
base = Ci::Pipeline.find(ENV.fetch('BASE'))
head = Ci::Pipeline.find(ENV.fetch('HEAD'))
user = project.first_owner
params = { report_type: ENV.fetch('REPORT_TYPE', 'sast'), scan_mode: ENV['SCAN_MODE'] }.compact

Feature.send(ENV['FULL_DATA_SET'] == '1' ? :enable : :disable, :security_mr_reports_tab_full_data_set, project)

def measure(project, user, params, base, head)
  queries = Hash.new(0)
  cache_hits = 0
  Gitlab::Database.database_base_models.each_value { |model| model.connection.clear_query_cache }
  rows = Hash.new(0)

  sql_sub = ActiveSupport::Notifications.subscribe('sql.active_record') do |*, payload|
    next if payload[:name] == 'SCHEMA'
    next cache_hits += 1 if payload[:cached]

    db = payload[:connection]&.pool&.db_config&.name || 'unknown'
    queries[db] += 1
  end
  inst_sub = ActiveSupport::Notifications.subscribe('instantiation.active_record') do |*, payload|
    rows[payload[:class_name]] += payload[:record_count]
  end

  GC.start
  alloc_before = GC.stat(:total_allocated_objects)
  cpu_before = Process.clock_gettime(Process::CLOCK_THREAD_CPUTIME_ID)
  wall_before = Process.clock_gettime(Process::CLOCK_MONOTONIC)

  result = Vulnerabilities::CompareSecurityReportsService.new(project, user, params).execute(base, head)

  {
    status: result[:status],
    added: result.dig(:data, 'added')&.size,
    fixed: result.dig(:data, 'fixed')&.size,
    wall_s: (Process.clock_gettime(Process::CLOCK_MONOTONIC) - wall_before).round(3),
    cpu_s: (Process.clock_gettime(Process::CLOCK_THREAD_CPUTIME_ID) - cpu_before).round(3),
    allocations: GC.stat(:total_allocated_objects) - alloc_before,
    queries: queries,
    cache_hits: cache_hits,
    rows: rows.slice('Security::Finding', 'Vulnerability', 'Vulnerabilities::StateTransition',
      'Vulnerabilities::Feedback', 'Vulnerabilities::IssueLink', 'Vulnerabilities::MergeRequestLink'),
    data: result[:data]
  }
ensure
  ActiveSupport::Notifications.unsubscribe(sql_sub)
  ActiveSupport::Notifications.unsubscribe(inst_sub)
end

results = [false, true].to_h do |lean|
  Feature.send(lean ? :enable : :disable, :lean_mr_security_report_comparison, project)
  measure(project, user, params, base, head) # warm-up (class loading, schema cache)
  [lean, measure(project, user, params, base, head)]
end

puts "Same serialized output: #{results[false][:data] == results[true][:data]}"
results.each do |lean, r|
  puts "\n== lean #{lean ? 'ON' : 'OFF'} =="
  r.except(:data).each { |k, v| puts "  #{k}: #{v}" }
end
Database queries

Lean finder query

SELECT "security_findings".*
FROM "security_scans",
     unnest('{1,2,4,5,6,7}'::smallint[]) AS "severities" ("severity"),
     LATERAL (
       SELECT "security_findings"."id", "security_findings"."scan_id", "security_findings"."scanner_id",
              "security_findings"."severity", "security_findings"."uuid",
              "security_findings"."context_unaware_uuid", "security_findings"."partition_number"
       FROM "security_findings"
       WHERE "security_findings"."scan_id" = "security_scans"."id"
         AND "security_findings"."severity" = "severities"."severity"
         AND "security_findings"."partition_number" IN (SELECT DISTINCT findings_partition_number FROM security_scans WHERE pipeline_id = 2818521991)
         AND "security_findings"."deduplicated" = TRUE
       ORDER BY "security_findings"."severity" DESC, "security_findings"."id" ASC
       LIMIT 1001
     ) AS "security_findings"
WHERE "security_scans"."pipeline_id" = 2818521991
  AND "security_scans"."latest" = TRUE
  AND "security_scans"."status" = 1
  AND "security_scans"."scan_type" = 2
ORDER BY "security_findings"."severity" DESC, "security_findings"."id" ASC

Reload query

SELECT "security_findings".*
FROM "security_findings"
WHERE "security_findings"."partition_number" = 12
  AND "security_findings"."id" IN (1, 2, 3)
Query Rows Buffers Row width Time (cold) Plan
Current finder 945 945 (623 hit + 322 read) 1127 160 ms plan
Lean finder 945 945 (623 hit + 322 read) 62 36 ms plan
Reload of 50 findings 50 429 (33 hit + 396 read), including the id lookup used only in this test 1111 16 ms plan

Both finder queries read the same pages, since finding_data is stored inline in the row; the saving is in bytes returned and sorted (1,154 kB vs 98 kB of sort memory) and in the Ruby objects built from them, not in pages read. The reload runs once per comparison for at most 50 findings, replacing the association preloads that previously ran for every finding of both pipelines.

References

Related #628409

Screenshots or screen recordings

How to set up and validate locally

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Harrison Peters

Merge request reports