Create new column in historical_data called recorded_at

- Populate recorded_at from created_at or date because created_at
should accurately tell us when the row was recorded. But created_at
is not `null: false` so we need to fallback to date.
- Set a not null constraint on recorded_at.
- Remove the not null constraint on date since we won't be populating
this column anymore. The column will be kept for now in case we need to
go back and check old values. The column can be removed after a
sufficient amount of time has passed with no issues.
- Change the HistoricalData model to use recorded_at instead of date.

db/migrate/20201022094845_add_historical_data_recorded_at.rb

+# frozen_string_literal: true
+
+class AddHistoricalDataRecordedAt < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    add_column(:historical_data, :recorded_at, :timestamptz)
+  end
+
+  def down
+    remove_column(:historical_data, :recorded_at)
+  end
+end

db/migrate/20201022094845_change_historical_data_date_type.rb

-# frozen_string_literal: true
-
-class ChangeHistoricalDataDateType < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    execute(
-      <<SQL
-      create or replace function change_historical_date_to_timetamptz(date)
-        returns timestamptz
-        language sql
-        as
-      $$
-        SELECT ($1 + '12:00'::time) AT TIME ZONE '#{Time.zone&.tzinfo&.name || "Etc/UTC"}'
-      $$
-SQL
-    )
-
-    change_column_type_concurrently(:historical_data, :date, :timestamptz, type_cast_function: "change_historical_date_to_timetamptz")
-
-    execute("DROP FUNCTION IF EXISTS change_historical_date_to_timetamptz")
-  end
-
-  def down
-    undo_change_column_type_concurrently(:historical_data, :date)
-  end
-end

db/migrate/20201022094846_update_historical_data_recorded_at.rb

+# frozen_string_literal: true
+
+class UpdateHistoricalDataRecordedAt < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    update_value = Arel.sql("COALESCE(created_at, date + '12:00'::time AT TIME ZONE '#{Time.zone&.tzinfo&.name || "Etc/UTC"}')")
+
+    update_column_in_batches(:historical_data, :recorded_at, update_value) do |table, query|
+      query.where(table[:recorded_at].eq(nil))
+    end
+
+    add_not_null_constraint :historical_data, :recorded_at
+
+    change_column_null :historical_data, :date, true
+  end
+
+  def down
+    change_column_null :historical_data, :date, false
+
+    remove_not_null_constraint :historical_data, :recorded_at
+
+    update_column_in_batches(:historical_data, :recorded_at, nil) do |table, query|
+      query.where(table[:recorded_at].not_eq(nil))
+    end
+  end
+end

db/post_migrate/20201024103311_change_historical_data_date_type_cleanup.rb

-# frozen_string_literal: true
-
-class ChangeHistoricalDataDateTypeCleanup < ActiveRecord::Migration[6.0]
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    cleanup_concurrent_column_type_change(:historical_data, :date)
-  end
-
-  def down
-    undo_cleanup_concurrent_column_type_change(:historical_data, :date, :date)
-  end
-end

db/schema_migrations/20201022094846

+588c5f99d34652bbd5bde86351cbdb8c0455af0c31a440bfb63df02f12fd588f
\ No newline at end of file

db/schema_migrations/20201024103311

-7a1719eb6975d731f89ce7e269868956d220e8f63dfd1739371ae2ddfd0e2473
\ No newline at end of file

db/structure.sql

@@ -12702,8 +12702,9 @@ CREATE TABLE historical_data (
     active_user_count integer,
     created_at timestamp without time zone,
     updated_at timestamp without time zone,
-    date timestamp with time zone,
-    CONSTRAINT check_8f3547750e CHECK ((date IS NOT NULL))
+    date date,
+    recorded_at timestamp with time zone,
+    CONSTRAINT check_640e8cf66c CHECK ((recorded_at IS NOT NULL))
 );
 
 CREATE SEQUENCE historical_data_id_seq

ee/app/models/historical_data.rb

 # frozen_string_literal: true
 
 class HistoricalData < ApplicationRecord
-  validates :date, presence: true
+  validates :recorded_at, presence: true
 
   # HistoricalData.during((Time.current - 1.year)..Time.current).average(:active_user_count)
-  scope :during, ->(range) { where(date: range) }
+  scope :during, ->(range) { where(recorded_at: range) }
   # HistoricalData.up_until(Time.current - 1.month).average(:active_user_count)
-  scope :up_until, ->(date) { where("date <= :date", date: date) }
+  scope :up_until, ->(timestamp) { where("recorded_at <= :timestamp", timestamp: timestamp) }
 
   class << self
     def track!
       create!(
-        date:               Time.current,
+        recorded_at:        Time.current,
         active_user_count:  License.load_license&.current_active_users_count
       )
     end
 
     # HistoricalData.at(Date.new(2014, 1, 1)).active_user_count
     def at(date)
-      find_by(date: date.all_day)
+      find_by(recorded_at: date.all_day)
     end
 
     def max_historical_user_count(license: nil, from: nil, to: nil)

ee/app/services/historical_user_data/csv_service.rb

@@ -22,7 +22,7 @@ module HistoricalUserData
 
     def header_to_value_hash
       {
-        'Date' => -> (historical_datum) { historical_datum.date.to_s(:csv) },
+        'Date' => -> (historical_datum) { historical_datum.recorded_at.to_s(:csv) },
         'Active User Count' => 'active_user_count'
       }
     end

ee/spec/controllers/admin/application_settings_controller_spec.rb

@@ -293,7 +293,7 @@ RSpec.describe Admin::ApplicationSettingsController do
     end
 
     context 'when an admin user attempts a request' do
-      let_it_be(:yesterday) { Time.current.utc.yesterday.to_date }
+      let_it_be(:yesterday) { Time.current.utc.yesterday }
       let_it_be(:max_count) { 15 }
       let_it_be(:current_count) { 10 }
 
@@ -302,8 +302,8 @@ RSpec.describe Admin::ApplicationSettingsController do
       end
 
       before_all do
-        HistoricalData.create!(date: yesterday - 1.day, active_user_count: max_count)
-        HistoricalData.create!(date: yesterday, active_user_count: current_count)
+        HistoricalData.create!(recorded_at: yesterday - 1.day, active_user_count: max_count)
+        HistoricalData.create!(recorded_at: yesterday, active_user_count: current_count)
       end
 
       before do
@@ -318,7 +318,7 @@ RSpec.describe Admin::ApplicationSettingsController do
         body = response.body
         expect(body).to start_with('<span id="LC1" class="line" lang="json">')
         expect(body).to include('<span class="nl">"license_key"</span>')
-        expect(body).to include("<span class=\"s2\">\"#{yesterday}\"</span>")
+        expect(body).to include("<span class=\"s2\">\"#{yesterday.to_date}\"</span>")
         expect(body).to include("<span class=\"mi\">#{max_count}</span>")
         expect(body).to include("<span class=\"mi\">#{current_count}</span>")
       end

ee/spec/factories/historical_data.rb

@@ -2,7 +2,7 @@
 
 FactoryBot.define do
   factory :historical_data do
-    date { Time.current }
+    recorded_at { Time.current }
     active_user_count { 1 }
   end
 end

ee/spec/models/gitlab/seat_link_data_spec.rb

@@ -28,10 +28,10 @@ RSpec.describe Gitlab::SeatLinkData do
     let_it_be(:today_active_count) { 20 }
 
     before_all do
-      HistoricalData.create!(date: license_start_date, active_user_count: 10)
-      HistoricalData.create!(date: license_start_date + 1.day, active_user_count: max_before_today)
-      HistoricalData.create!(date: utc_date - 1.day, active_user_count: yesterday_active_count)
-      HistoricalData.create!(date: utc_date, active_user_count: today_active_count)
+      HistoricalData.create!(recorded_at: license_start_date, active_user_count: 10)
+      HistoricalData.create!(recorded_at: license_start_date + 1.day, active_user_count: max_before_today)
+      HistoricalData.create!(recorded_at: utc_date - 1.day, active_user_count: yesterday_active_count)
+      HistoricalData.create!(recorded_at: utc_date, active_user_count: today_active_count)
     end
 
     around do |example|

ee/spec/models/historical_data_spec.rb

@@ -5,7 +5,7 @@ require 'spec_helper'
 RSpec.describe HistoricalData do
   before do
     (1..12).each do |i|
-      described_class.create!(date: Date.new(2014, i, 1), active_user_count: i * 100)
+      described_class.create!(recorded_at: Date.new(2014, i, 1), active_user_count: i * 100)
     end
   end
 
@@ -39,7 +39,7 @@ RSpec.describe HistoricalData do
         data = described_class.last
         # Database time has microsecond precision, while Ruby time has nanosecond precision,
         # which is why we need the be_within matcher even though we're freezing time.
-        expect(data.date).to be_within(1e-6.seconds).of(Time.current)
+        expect(data.recorded_at).to be_within(1e-6.seconds).of(Time.current)
         expect(data.active_user_count).to eq(5)
       end
     end
@@ -49,7 +49,7 @@ RSpec.describe HistoricalData do
     context 'with multiple historical data points for the current license' do
       before do
         (1..3).each do |i|
-          described_class.create!(date: Time.current - i.days, active_user_count: i * 100)
+          described_class.create!(recorded_at: Time.current - i.days, active_user_count: i * 100)
         end
       end
 
@@ -110,7 +110,7 @@ RSpec.describe HistoricalData do
 
       context 'with stats before the license period' do
         before do
-          described_class.create!(date: license.starts_at.ago(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: license.starts_at.ago(2.days), active_user_count: 10)
         end
 
         it 'ignore those records' do
@@ -120,7 +120,7 @@ RSpec.describe HistoricalData do
 
       context 'with stats after the license period' do
         before do
-          described_class.create!(date: license.expires_at.in(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: license.expires_at.in(2.days), active_user_count: 10)
         end
 
         it 'ignore those records' do
@@ -130,8 +130,8 @@ RSpec.describe HistoricalData do
 
       context 'with stats inside license period' do
         before do
-          described_class.create!(date: license.starts_at.in(2.days), active_user_count: 10)
-          described_class.create!(date: license.starts_at.in(5.days), active_user_count: 15)
+          described_class.create!(recorded_at: license.starts_at.in(2.days), active_user_count: 10)
+          described_class.create!(recorded_at: license.starts_at.in(5.days), active_user_count: 15)
         end
 
         it 'returns max value for active_user_count' do
@@ -151,10 +151,10 @@ RSpec.describe HistoricalData do
     end
 
     before_all do
-      described_class.create!(date: license.starts_at - 1.day, active_user_count: 1)
-      described_class.create!(date: license.expires_at + 1.day, active_user_count: 2)
-      described_class.create!(date: now - 1.year - 1.day, active_user_count: 3)
-      described_class.create!(date: now + 1.day, active_user_count: 4)
+      described_class.create!(recorded_at: license.starts_at - 1.day, active_user_count: 1)
+      described_class.create!(recorded_at: license.expires_at + 1.day, active_user_count: 2)
+      described_class.create!(recorded_at: now - 1.year - 1.day, active_user_count: 3)
+      described_class.create!(recorded_at: now + 1.day, active_user_count: 4)
     end
 
     around do |example|

ee/spec/models/license_spec.rb

@@ -68,7 +68,7 @@ RSpec.describe License do
     describe "Historical active user count" do
       let(:active_user_count) { described_class.current_active_users.count + 10 }
       let(:date)              { described_class.current.starts_at }
-      let!(:historical_data)  { HistoricalData.create!(date: date, active_user_count: active_user_count) }
+      let!(:historical_data)  { HistoricalData.create!(recorded_at: date, active_user_count: active_user_count) }
 
       context "when there is no active user count restriction" do
         it "is valid" do
@@ -249,7 +249,7 @@ RSpec.describe License do
     describe 'downgrade' do
       context 'when more users were added in previous period' do
         before do
-          HistoricalData.create!(date: described_class.current.starts_at - 6.months, active_user_count: 15)
+          HistoricalData.create!(recorded_at: described_class.current.starts_at - 6.months, active_user_count: 15)
 
           set_restrictions(restricted_user_count: 5, previous_user_count: 10)
         end
@@ -261,7 +261,7 @@ RSpec.describe License do
 
       context 'when no users were added in the previous period' do
         before do
-          HistoricalData.create!(date: 6.months.ago, active_user_count: 15)
+          HistoricalData.create!(recorded_at: 6.months.ago, active_user_count: 15)
 
           set_restrictions(restricted_user_count: 10, previous_user_count: 15)
         end

ee/spec/services/historical_user_data/csv_service_spec.rb

@@ -89,10 +89,10 @@ RSpec.describe HistoricalUserData::CsvService do
 
   context 'User Count Table' do
     let_it_be(:historical_datum) do
-      create(:historical_data, date: license_start_date, active_user_count: 1)
+      create(:historical_data, recorded_at: license_start_date, active_user_count: 1)
     end
     let_it_be(:historical_datum2) do
-      create(:historical_data, date: license_start_date + 1.day, active_user_count: 2)
+      create(:historical_data, recorded_at: license_start_date + 1.day, active_user_count: 2)
     end
 
     it 'shows the header for the user counts table' do
@@ -101,11 +101,11 @@ RSpec.describe HistoricalUserData::CsvService do
 
     it 'includes proper values for each column type', :aggregate_failures do
       expect(csv[8]).to contain_exactly(
-        historical_datum.date.to_s(:db),
+        historical_datum.recorded_at.to_s(:db),
         historical_datum.active_user_count.to_s
       )
       expect(csv[9]).to contain_exactly(
-        historical_datum2.date.to_s(:db),
+        historical_datum2.recorded_at.to_s(:db),
         historical_datum2.active_user_count.to_s
       )
     end

ee/spec/workers/sync_seat_link_worker_spec.rb

@@ -11,13 +11,13 @@ RSpec.describe SyncSeatLinkWorker, type: :worker do
         # Setting the date as 12th March 2020 12:00 UTC for tests and creating new license
         create_current_license(starts_at: '2020-02-12'.to_date)
 
-        HistoricalData.create!(date: '2020-02-11'.to_date, active_user_count: 100)
-        HistoricalData.create!(date: '2020-02-12'.to_date, active_user_count: 10)
-        HistoricalData.create!(date: '2020-02-13'.to_date, active_user_count: 15)
+        HistoricalData.create!(recorded_at: '2020-02-11'.to_date, active_user_count: 100)
+        HistoricalData.create!(recorded_at: '2020-02-12'.to_date, active_user_count: 10)
+        HistoricalData.create!(recorded_at: '2020-02-13'.to_date, active_user_count: 15)
 
-        HistoricalData.create!(date: '2020-03-11'.to_date, active_user_count: 10)
-        HistoricalData.create!(date: '2020-03-12'.to_date, active_user_count: 20)
-        HistoricalData.create!(date: '2020-03-15'.to_date, active_user_count: 25)
+        HistoricalData.create!(recorded_at: '2020-03-11'.to_date, active_user_count: 10)
+        HistoricalData.create!(recorded_at: '2020-03-12'.to_date, active_user_count: 20)
+        HistoricalData.create!(recorded_at: '2020-03-15'.to_date, active_user_count: 25)
         allow(SyncSeatLinkRequestWorker).to receive(:perform_async).and_return(true)
       end
 

spec/migrations/update_historical_data_recorded_at_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require Rails.root.join('db', 'migrate', '20201022094846_update_historical_data_recorded_at.rb')
+
+RSpec.describe UpdateHistoricalDataRecordedAt do
+  let(:historical_data_table) { table(:historical_data) }
+
+  it 'reversibly populates recorded_at from created_at or date' do
+    row1 = historical_data_table.create!(
+      date: Date.current - 1.day,
+      created_at: Time.current - 1.day
+    )
+
+    row2 = historical_data_table.create!(date: Date.current - 2.days)
+    row2.update!(created_at: nil)
+
+    reversible_migration do |migration|
+      migration.before -> {
+        expect(row1.reload.recorded_at).to eq(nil)
+        expect(row2.reload.recorded_at).to eq(nil)
+      }
+
+      migration.after -> {
+        expect(row1.reload.recorded_at).to eq(row1.created_at)
+        expect(row2.reload.recorded_at).to eq(row2.date.in_time_zone(Time.zone).change(hour: 12))
+      }
+    end
+  end
+end