Nightly checksum and checksum mismatch detection assertion #85

ericaporter · 2023-09-13T11:04:22Z

Ticket - Nightly checksum and checksum mismatch detection assertion

Changes:

To allow the addition of EntityTableCheckJob to InitialiseEvents, InitialiseEvents has been renamed to 'InitialisationEvents'
EntityTableCheckJob has been added to InitialisationEvents
To avoid running as a cron job, we are rescheduling the job to run again every 24hrs
The current timestamp is already sent as occurred_at so it wasn't explicitly added
Some Coordination is required with Dataform Analytics to expect the event and ensure the checksum is correct

N.B. Updates required to request specs - WIP

Ticket details:
As part of dfe-analytics, create a scheduled job that runs shortly before midnight each night
Make this job stream an event with type ‘entity_table_check’ (or similar) for each model listed in analytics.yml which contains the following fields in DATA:
entity_table_name
number_of_rows
Current Timestamp (When query was run)
checksum e.g. something that matches the output of the GoogleSQL

asatwal

Nice work @ericaporter. A few comments for your perusal.

lib/dfe/analytics/entity_table_check_job.rb

spec/dfe/analytics_spec.rb

spec/dfe/analytics/entity_table_check_job_spec.rb

lib/dfe/analytics/entity_table_check_job.rb

asatwal

Looking good - Just a few more comments

asatwal · 2023-09-21T09:01:32Z

lib/dfe/analytics/entity_table_check_job.rb

+        {
+          number_of_rows: model.count,
+          checksum: checksum_data(model)[:checksum],
+          timestamp: checksum_data(model)[:timestamp]
+        }


We should only include records created or updated before checksum_calculated_at, that way we don't have a moving target and the window for any inserts and updates creeping in is very small.
Also ordering by updated_at would mean we would detect missing updates.
@stevenleggdfe on the dataform side you would also have to use the checksum_calculated_at in a similar way.

Suggested change

{

number_of_rows: model.count,

checksum: checksum_data(model)[:checksum],

timestamp: checksum_data(model)[:timestamp]

}

checksum_calculated_at = Time.now.in_time_zone(TIME_ZONE).iso8601(6)

table_ids =

model

.where('updated_at < ?', checksum_calculated_at)

.order(updated: :asc)

.pluck(:id).join

{

row_count: table_ids.count,

checksum: Digest::SHA256.hexdigest(table_ids),

checksum_calculated_at: checksum_calculated_at

}

Yep, that works for me. I like the thinking.

@asatwal I don't think table_ids.count will work here, (or table_ids.size) after we join them, if the ids are double or treble figures etc., I think we would be getting the number of digits and not the distinct number of records. I've updated it to use table_ids count prior to ordering and joining and tried to reuse this for the checksum instead of doing two separate lookups. I've also had to parse the checksum_calculated_at in order to compare with updated_at

Oops my bad ! Indeed and well spotted. Is this better:

checksum_calculated_at = Time.now.in_time_zone(TIME_ZONE).iso8601(6) table_ids = model .where('updated_at < ?', checksum_calculated_at) .order(updated: :asc) .pluck(:id) { row_count: table_ids.count, checksum: Digest::SHA256.hexdigest(table_ids.join), checksum_calculated_at: checksum_calculated_at }

Your change is fine though and works and actually that parsing is needed. I'll let you decide which one is more efficient.

lib/dfe/analytics/entity_table_check_job.rb

spec/dfe/analytics/entity_table_check_job_spec.rb

asatwal

Just a few comments on the new config item.

Also we'll need to add some blurb to the README.md file - Just some brief notes on how to enable and what it does.

Thanks

asatwal · 2023-09-25T08:57:28Z

lib/dfe/analytics/entity_table_check_job.rb

+        self.class.set(wait_until: WAIT_TIME).perform_later
+      end
+
+      def allow_reschedule?


For config, there is already a pattern in lib\dfe\analytics.rb. I suggest we follow exactly the same pattern as the config item pseudonymise_web_request_user_id, except we call the config item entity_table_checks_enabled and also disable by default. Disabling by default means that we would enable these checks service by service. This is the safest options, although it will be more work to enable this for all the services.

Any opinions on disabling by default @ericaporter @stevenleggdfe ?

Following the above pattern, we should have a method called DfE::Analytics.entity_table_checks_enabled? to check the config item.

Also we can remove the allow_reschedule? method here.

@asatwal @ericaporter I'd be up for disabling by default personally unless we're worried about load on the database generating the checksum?

Also if we're adding configuration for this then can we add the configuration parameter to initialise_analytics as well so that dfe-analytics-dataform can switch the checksum checking on and off depending?

Defaulting it to false will give us more control and make sure this works as expected in production 👍

Yep, initially anyway. I wonder if in time we may want to get more forceful with teams and switch the default to true - but only once we're confident it works and doesn't have a significant performance impact.

asatwal · 2023-09-25T09:04:06Z

lib/dfe/analytics/initialisation_events.rb

        end

-        @@initialise_event_sent = true # rubocop:disable Style:ClassVars
+        DfE::Analytics::EntityTableCheckJob.perform_later


We should check if the job is enabled here:

DfE::Analytics::EntityTableCheckJob.perform_later if DfE::Analytics.entity_table_checks_enabled?

asatwal · 2023-09-25T09:12:19Z

lib/dfe/analytics/initialisation_events.rb

      end

      private

-      def initialisation_data
+      def initialise_analytics_data
        {
          analytics_version: DfE::Analytics::VERSION,
          config: {


It might be useful to send the entity_table_checks_enabled config item here in the initialise event. I'm not sure though if Analytics dataform will use it though. What do you think @stevenleggdfe ?

Yes please @asatwal @ericaporter ! This would enable us to switch checks on/off in Dataform depending

asatwal · 2023-09-26T08:18:22Z

spec/dfe/analytics/entity_table_check_job_spec.rb

+      })])
+    end
+
+    it 'does not send the event if updated_at is greater than checksum_calculated_at' do


asatwal

👍 Looks good. Nice work.

…eckJob.perform later conditional

ericaporter requested a review from asatwal September 13, 2023 11:54

asatwal reviewed Sep 15, 2023

View reviewed changes

ericaporter requested a review from stevenleggdfe September 18, 2023 09:03

stevenleggdfe reviewed Sep 18, 2023

View reviewed changes

lib/dfe/analytics/entity_table_check_job.rb Outdated Show resolved Hide resolved

lib/dfe/analytics/entity_table_check_job.rb Show resolved Hide resolved

asatwal reviewed Sep 21, 2023

View reviewed changes

asatwal reviewed Sep 25, 2023

View reviewed changes

asatwal reviewed Sep 26, 2023

View reviewed changes

asatwal approved these changes Sep 26, 2023

View reviewed changes

ericaporter added 13 commits September 27, 2023 15:34

Update Initalise.rb to be InitalisationEvents.rb

4836499

Add entity_table_check_job with a entity_table_check event

525f8f7

Removing whitespace

0e0513b

Appease rubocop

bc1578a

Fix spec file

1804583

Make reschedule job an exact time

3474ce4

Calculate checksum on table ids

20dd801

Add checksum timestamp and update checksum method

92b0617

Add config to reschedule_job, update checksum

12dab0d

Add line to end of file

5c43a48

Update config to send entity_table_checks_enabled, make EntityTableCh…

b83db3a

…eckJob.perform later conditional

Update readme, update checksum lookup

68917a5

Fix test to match event_type

4f4cfae

ericaporter force-pushed the nightly-checksum branch from fca3b8c to 4f4cfae Compare September 27, 2023 14:37

ericaporter merged commit 9caf19f into main Sep 27, 2023

ericaporter deleted the nightly-checksum branch September 27, 2023 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nightly checksum and checksum mismatch detection assertion #85

Nightly checksum and checksum mismatch detection assertion #85

ericaporter commented Sep 13, 2023 •

edited

Loading

asatwal left a comment

asatwal left a comment

asatwal Sep 21, 2023

stevenleggdfe Sep 21, 2023

ericaporter Sep 22, 2023 •

edited

Loading

asatwal Sep 25, 2023 •

edited

Loading

asatwal left a comment

asatwal Sep 25, 2023

stevenleggdfe Sep 25, 2023

ericaporter Sep 25, 2023

stevenleggdfe Sep 25, 2023

asatwal Sep 25, 2023

asatwal Sep 25, 2023

stevenleggdfe Sep 25, 2023

asatwal Sep 26, 2023

asatwal left a comment

-        {
-          number_of_rows: model.count,
-          checksum: checksum_data(model)[:checksum],
-          timestamp: checksum_data(model)[:timestamp]
-        }
+        checksum_calculated_at = Time.now.in_time_zone(TIME_ZONE).iso8601(6)
+        table_ids =
+          model
+          .where('updated_at < ?', checksum_calculated_at)
+          .order(updated: :asc)
+          .pluck(:id).join
+        {
+          row_count: table_ids.count,
+          checksum: Digest::SHA256.hexdigest(table_ids),
+          checksum_calculated_at: checksum_calculated_at
+        }

Nightly checksum and checksum mismatch detection assertion #85

Nightly checksum and checksum mismatch detection assertion #85

Conversation

ericaporter commented Sep 13, 2023 • edited Loading

asatwal left a comment

Choose a reason for hiding this comment

asatwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericaporter Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

asatwal Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

asatwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asatwal left a comment

Choose a reason for hiding this comment

ericaporter commented Sep 13, 2023 •

edited

Loading

ericaporter Sep 22, 2023 •

edited

Loading

asatwal Sep 25, 2023 •

edited

Loading