Merge branch 'mwaw/211433-metrics-dashboard-annotations-reaper-job-database-layer' into 'master'

Metrics dashboard annotation reaper job database layer changes

See merge request gitlab-org/gitlab!32433

app/models/metrics/dashboard/annotation.rb

@@ -3,6 +3,8 @@
 module Metrics
   module Dashboard
     class Annotation < ApplicationRecord
+      include DeleteWithLimit
+
       self.table_name = 'metrics_dashboard_annotations'
 
       belongs_to :environment, inverse_of: :metrics_dashboard_annotations
@@ -14,14 +16,25 @@ module Metrics
       validates :panel_xid, length: { maximum: 255 }
       validate :single_ownership
       validate :orphaned_annotation
+      validate :ending_at_after_starting_at
 
       scope :after, ->(after) { where('starting_at >= ?', after) }
       scope :before, ->(before) { where('starting_at <= ?', before) }
 
       scope :for_dashboard, ->(dashboard_path) { where(dashboard_path: dashboard_path) }
+      scope :ending_before, ->(timestamp) { where('COALESCE(ending_at, starting_at) < ?', timestamp) }
 
       private
 
+      # If annotation has NULL in ending_at column that indicates, that this annotation IS TIED TO SINGLE POINT
+      # IN TIME designated by starting_at timestamp. It does NOT mean that annotation is ever going starting from
+      # stating_at timestamp
+      def ending_at_after_starting_at
+        return if ending_at.blank? || starting_at.blank? || starting_at <= ending_at
+
+        errors.add(:ending_at, s_("Metrics::Dashboard::Annotation|can't be before starting_at time"))
+      end
+
       def single_ownership
         return if cluster.nil? ^ environment.nil?
 

changelogs/unreleased/mwaw-211433-metrics-dashboard-annotations-reaper-job-database-layer.yml

+---
+title: Table index added to `metrics_dashboard_annotations` for future pruning of stale metrics
+  Annotations for metrics dashboards are now checked for valid start and end dates.
+merge_request: 32433
+author:
+type: added

db/post_migrate/20200518133123_add_index_on_starting_ending_at_to_metrics_dashboard_annotations.rb

+# frozen_string_literal: true
+
+class AddIndexOnStartingEndingAtToMetricsDashboardAnnotations < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_metrics_dashboard_annotations_on_timespan_end'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :metrics_dashboard_annotations, 'COALESCE(ending_at, starting_at)', name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index :metrics_dashboard_annotations, 'COALESCE(ending_at, starting_at)', name: INDEX_NAME
+  end
+end

db/structure.sql

@@ -10082,6 +10082,8 @@ CREATE INDEX index_metrics_dashboard_annotations_on_cluster_id_and_3_columns ON
 
 CREATE INDEX index_metrics_dashboard_annotations_on_environment_id_and_3_col ON public.metrics_dashboard_annotations USING btree (environment_id, dashboard_path, starting_at, ending_at) WHERE (environment_id IS NOT NULL);
 
+CREATE INDEX index_metrics_dashboard_annotations_on_timespan_end ON public.metrics_dashboard_annotations USING btree (COALESCE(ending_at, starting_at));
+
 CREATE INDEX index_metrics_users_starred_dashboards_on_project_id ON public.metrics_users_starred_dashboards USING btree (project_id);
 
 CREATE INDEX index_milestone_releases_on_release_id ON public.milestone_releases USING btree (release_id);
@@ -13948,6 +13950,7 @@ COPY "schema_migrations" (version) FROM STDIN;
 20200514000340
 20200515155620
 20200518091745
+20200518133123
 20200519115908
 20200519171058
 20200519194042

locale/gitlab.pot

@@ -13715,6 +13715,9 @@ msgstr ""
 msgid "Metrics::Dashboard::Annotation|You are not authorized to delete this annotation"
 msgstr ""
 
+msgid "Metrics::Dashboard::Annotation|can't be before starting_at time"
+msgstr ""
+
 msgid "Metrics::UsersStarredDashboards|Dashboard with requested path can not be found"
 msgstr ""
 

spec/models/metrics/dashboard/annotation_spec.rb

@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Metrics::Dashboard::Annotation do
+  using RSpec::Parameterized::TableSyntax
+
   describe 'associations' do
     it { is_expected.to belong_to(:environment).inverse_of(:metrics_dashboard_annotations) }
     it { is_expected.to belong_to(:cluster).class_name('Clusters::Cluster').inverse_of(:metrics_dashboard_annotations) }
@@ -28,6 +30,26 @@ describe Metrics::Dashboard::Annotation do
       end
     end
 
+    context 'ending_at_after_starting_at' do
+      where(:starting_at, :ending_at, :valid?, :message) do
+        2.days.ago.beginning_of_day | 1.day.ago.beginning_of_day  | true  | nil
+        1.day.ago.beginning_of_day  | nil                         | true  | nil
+        1.day.ago.beginning_of_day  | 1.day.ago.beginning_of_day  | true  | nil
+        1.day.ago.beginning_of_day  | 2.days.ago.beginning_of_day | false | /Ending at can't be before starting_at time/
+        nil                         | 2.days.ago.beginning_of_day | false | /Starting at can't be blank/ # validation is covered by other method, be we need to assure, that ending_at_after_starting_at will not break with nil as starting_at
+        nil                         | nil                         | false | /Starting at can't be blank/ # validation is covered by other method, be we need to assure, that ending_at_after_starting_at will not break with nil as starting_at
+      end
+
+      with_them do
+        subject(:annotation) { build(:metrics_dashboard_annotation, starting_at: starting_at, ending_at: ending_at) }
+
+        it do
+          expect(annotation.valid?).to be(valid?)
+          expect(annotation.errors.full_messages).to include(message) if message
+        end
+      end
+    end
+
     context 'environments annotation' do
       subject { build(:metrics_dashboard_annotation) }
 
@@ -75,5 +97,16 @@ describe Metrics::Dashboard::Annotation do
         expect(described_class.for_dashboard('other_dashboard.yml')).to match_array [other_dashboard_annotation]
       end
     end
+
+    describe '#ending_before' do
+      it 'returns annotations only for appointed dashboard' do
+        Timecop.freeze do
+          twelve_minutes_old_annotation = create(:metrics_dashboard_annotation, starting_at: 15.minutes.ago, ending_at: 12.minutes.ago)
+          create(:metrics_dashboard_annotation, starting_at: 15.minutes.ago, ending_at: 11.minutes.ago)
+
+          expect(described_class.ending_before(11.minutes.ago)).to match_array [fifteen_minutes_old_annotation, twelve_minutes_old_annotation]
+        end
+      end
+    end
   end
 end