Merge branch 'ci-platform-metrics-bulk-insert' into 'master'

Updates CiPlatformMetrics to do bulk insertions

Closes #245321

See merge request gitlab-org/gitlab!41617

app/models/ci_platform_metric.rb

 # frozen_string_literal: true
 
 class CiPlatformMetric < ApplicationRecord
+  include BulkInsertSafe
+
+  PLATFORM_TARGET_MAX_LENGTH = 255
+
   validates :recorded_at, presence: true
-  validates :platform_target, presence: true, length: { maximum: 255 }
-  validates :count, presence: true
+  validates :platform_target,
+    exclusion: [nil], # allow '' (the empty string), but not nil
+    length: { maximum: PLATFORM_TARGET_MAX_LENGTH }
+  validates :count,
+    presence: true,
+    numericality: { only_integer: true, greater_than: 0 }
 
-  CI_VARIABLE_KEY = "AUTO_DEVOPS_PLATFORM_TARGET"
+  CI_VARIABLE_KEY = 'AUTO_DEVOPS_PLATFORM_TARGET'
 
-  def self.update!
+  def self.insert_auto_devops_platform_targets!
     # This work can NOT be done in-database because value is encrypted.
-    # However, for "AUTO_DEVOPS_PLATFORM_TARGET", these values are only
+    # However, for 'AUTO_DEVOPS_PLATFORM_TARGET', these values are only
     # encrypted as a matter of course, rather than as a need for secrecy.
     # So this is not a security risk, but exposing other keys possibly could be.
     variables = Ci::Variable.by_key(CI_VARIABLE_KEY)
-    update_recorded_at = Time.zone.now
+    recorded_at = Time.zone.now
     counts = variables.group_by(&:value).map do |value, variables|
-      {
-        recorded_at: update_recorded_at,
-        platform_target: value,
-        count: variables.count
-      }
+      target = value.truncate(PLATFORM_TARGET_MAX_LENGTH, separator: '', omission: '')
+      count = variables.count
+      self.new(recorded_at: recorded_at, platform_target: target, count: count)
     end
 
-    create(counts)
+    bulk_insert!(counts, validate: true)
   end
 end

changelogs/unreleased/ci-platform-metrics-bulk-insert.yml

+---
+title: Updates CiPlatformMetrics to do bulk insertions
+merge_request: 41617
+author:
+type: performance

db/migrate/20200908183231_add_check_positive_constraint_to_ci_platform_metrics.rb

+# frozen_string_literal: true
+
+class AddCheckPositiveConstraintToCiPlatformMetrics < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  CONSTRAINT_NAME = 'ci_platform_metrics_check_count_positive'
+
+  def up
+    add_check_constraint :ci_platform_metrics, 'count > 0', CONSTRAINT_NAME
+  end
+
+  def down
+    remove_check_constraint :ci_platform_metrics, CONSTRAINT_NAME
+  end
+end

db/schema_migrations/20200908183231

+9eadbb80f137ba0123c96e04ded21cc313560b7a293c241c6a72ebd35248a84b
\ No newline at end of file

db/structure.sql

@@ -10321,7 +10321,8 @@ CREATE TABLE public.ci_platform_metrics (
     recorded_at timestamp with time zone NOT NULL,
     platform_target text NOT NULL,
     count integer NOT NULL,
-    CONSTRAINT check_f922abc32b CHECK ((char_length(platform_target) <= 255))
+    CONSTRAINT check_f922abc32b CHECK ((char_length(platform_target) <= 255)),
+    CONSTRAINT ci_platform_metrics_check_count_positive CHECK ((count > 0))
 );
 
 CREATE SEQUENCE public.ci_platform_metrics_id_seq

spec/models/ci_platform_metric_spec.rb

@@ -5,63 +5,86 @@ require 'spec_helper'
 RSpec.describe CiPlatformMetric do
   subject { build(:ci_platform_metric) }
 
+  it_behaves_like 'a BulkInsertSafe model', CiPlatformMetric do
+    let(:valid_items_for_bulk_insertion) { build_list(:ci_platform_metric, 10) }
+    let(:invalid_items_for_bulk_insertion) { [] } # class does not have any non-constraint validations defined
+  end
+
   describe 'validations' do
     it { is_expected.to validate_presence_of(:recorded_at) }
     it { is_expected.to validate_presence_of(:count) }
-    it { is_expected.to validate_presence_of(:platform_target) }
+    it { is_expected.to validate_numericality_of(:count).only_integer.is_greater_than(0) }
+    it { is_expected.to allow_values('').for(:platform_target) }
+    it { is_expected.not_to allow_values(nil).for(:platform_target) }
     it { is_expected.to validate_length_of(:platform_target).is_at_most(255) }
   end
 
-  describe '.update!' do
+  describe '.insert_auto_devops_platform_targets!' do
     def platform_target_counts_by_day
       report = Hash.new { |hash, key| hash[key] = {} }
-      CiPlatformMetric.all.each do |metric|
+      described_class.all.each do |metric|
         date = metric.recorded_at.to_date
         report[date][metric.platform_target] = metric.count
       end
       report
     end
 
-    context "when there is already existing metrics data" do
+    context 'when there is already existing metrics data' do
       let!(:metric_1) { create(:ci_platform_metric) }
       let!(:metric_2) { create(:ci_platform_metric) }
 
-      it "does not erase any existing data" do
-        CiPlatformMetric.update!
+      it 'does not erase any existing data' do
+        described_class.insert_auto_devops_platform_targets!
 
-        expect(CiPlatformMetric.all.to_a).to contain_exactly(metric_1, metric_2)
+        expect(described_class.all.to_a).to contain_exactly(metric_1, metric_2)
       end
     end
 
-    context "when there are multiple platform target variables" do
+    context 'when there are multiple platform target variables' do
       let(:today) { Time.zone.local(1982, 4, 24) }
       let(:tomorrow) { today + 1.day }
 
-      it "updates platform target counts for that day" do
+      it 'inserts platform target counts for that day' do
         Timecop.freeze(today) do
-          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: "aws")
-          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: "aws")
-          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: "fargate")
-          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: "fargate")
-          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: "fargate")
-          CiPlatformMetric.update!
+          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: 'aws')
+          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: 'aws')
+          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: 'fargate')
+          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: 'fargate')
+          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: 'fargate')
+          described_class.insert_auto_devops_platform_targets!
         end
         Timecop.freeze(tomorrow) do
-          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: "fargate")
-          CiPlatformMetric.update!
+          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: 'fargate')
+          described_class.insert_auto_devops_platform_targets!
+        end
+
+        expect(platform_target_counts_by_day).to eq({
+          today.to_date => { 'aws' => 2, 'fargate' => 3 },
+          tomorrow.to_date => { 'aws' => 2, 'fargate' => 4 }
+        })
+      end
+    end
+
+    context 'when there are ci variable values too long for platform_target' do
+      let(:today) { Time.zone.local(1982, 4, 24) }
+
+      it 'truncates those values' do
+        max = described_class::PLATFORM_TARGET_MAX_LENGTH
+        Timecop.freeze(today) do
+          create(:ci_variable, key: described_class::CI_VARIABLE_KEY, value: 'F' * (max + 1))
+          described_class.insert_auto_devops_platform_targets!
         end
 
         expect(platform_target_counts_by_day).to eq({
-          today.to_date => { "aws" => 2, "fargate" => 3 },
-          tomorrow.to_date => { "aws" => 2, "fargate" => 4 }
+          today.to_date => { 'F' * max => 1 }
         })
       end
     end
 
-    context "when there are no platform target variables" do
-      it "does not generate any new platform metrics" do
-        create(:ci_variable, key: "KEY_WHATEVER", value: "aws")
-        CiPlatformMetric.update!
+    context 'when there are no platform target variables' do
+      it 'does not generate any new platform metrics' do
+        create(:ci_variable, key: 'KEY_WHATEVER', value: 'aws')
+        described_class.insert_auto_devops_platform_targets!
 
         expect(platform_target_counts_by_day).to eq({})
       end