Change profile/rule status queries to use evaluation history table #4089

dmjb · 2024-08-06T11:48:18Z

Depends on: #4065

This change allows most of the functionality which previously used the rule_evaluations (and related) tables to use the new evaluation history tables instead.

Change in API behaviour: GetProfileStatusByName previously returned repo information when querying Github pull requests and artifacts. This behaviour relied on a quirk of the old rule evaluation tables, and is no longer carried forward in the new implementation.

Summary

Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.

Fixes #(related issue)

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-08-06T11:57:33Z

coverage: 54.032% (+0.06%) from 53.974%
when pulling cfa0841 on profile-status-quieries-eval-history
into da3b7b1 on main.

dmjb · 2024-08-15T12:19:24Z

cmd/dev/app/rule_type/rttst.go

@@ -132,15 +131,12 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
 		return fmt.Errorf("error reading fragment from file: %w", err)
 	}

-	remediateStatus := db.NullRemediationStatusTypes{}
+	remediateStatus := db.RemediationStatusTypesSkipped


A recurring theme with many of the code changes in this PR is that the old queries returned a lot more null types. Due to the schema of the new tables, and the fact that evaluation history is now inserted transactionally, there are far fewer nullable fields.

So "skipped" now means actually-skipped, or "didn't report a status"?

(I do like removing null values and replacing with sentinels where possible, I'm just wondering whether having "" as a default/valid value would be a good way to handle this.)

Keep in mind that this file implements the CLI command for testing profiles/rules. The use of "Skipped" as a sentinel value vs NULL is of little consequence since this command does not persist anything to the DB.

Oh, I completely forgot that this bit existed. Nevermind!

dmjb · 2024-08-15T13:50:15Z

internal/util/cli/table/simple/simple.go

@@ -117,7 +117,7 @@ func profileStatusLayout(table *tablewriter.Table) {
 func ruleEvaluationsLayout(table *tablewriter.Table) {
 	defaultLayout(table)
 	table.SetHeader([]string{
-		"Rule ID", "Rule Name", "Entity", "Status", "Remediation", "Entity Info"})
+		"Rule Type ID", "Rule Name", "Entity", "Status", "Remediation", "Entity Info"})


Spotted this during local testing. "Rule ID" is misleading - the table is in fact showing a rule type ID here.

dmjb · 2024-08-15T14:24:08Z

database/query/profile_status.sql

@@ -137,21 +138,23 @@ SELECT
    rt.id AS rule_type_id,
    rt.guidance as rule_type_guidance,
    rt.display_name as rule_type_display_name
-FROM rule_evaluations res


In the evaluation history tables, we are guaranteed that there will always be one alert and remediation row per evaluation status, and at least one evaluation status and latest evaluation status per evaluation rule entity. This is because the data is inserted transactionally into the DB.

This change allows most of the functionality which previously used the `rule_evaluations` (and related) tables to use the new evaluation history tables instead.

evankanderson

A bunch of smaller comments, but nothing that feels obviously-wrong / blocking.

evankanderson · 2024-08-15T17:17:27Z

cmd/dev/app/rule_type/rttst.go

@@ -132,15 +131,12 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
 		return fmt.Errorf("error reading fragment from file: %w", err)
 	}

-	remediateStatus := db.NullRemediationStatusTypes{}
+	remediateStatus := db.RemediationStatusTypesSkipped


So "skipped" now means actually-skipped, or "didn't report a status"?

evankanderson · 2024-08-15T17:18:21Z

cmd/dev/app/rule_type/rttst.go

@@ -132,15 +131,12 @@ func testCmdRun(cmd *cobra.Command, _ []string) error {
 		return fmt.Errorf("error reading fragment from file: %w", err)
 	}

-	remediateStatus := db.NullRemediationStatusTypes{}
+	remediateStatus := db.RemediationStatusTypesSkipped


(I do like removing null values and replacing with sentinels where possible, I'm just wondering whether having "" as a default/valid value would be a good way to handle this.)

evankanderson · 2024-08-15T17:21:39Z

database/query/profile_status.sql

+FROM latest_evaluation_statuses les
+         INNER JOIN evaluation_rule_entities ere ON ere.id = les.rule_entity_id
+         INNER JOIN eval_details ed ON ed.id = les.evaluation_history_id
+         INNER JOIN remediation_details rd ON rd.evaluation_id = les.evaluation_history_id
+         INNER JOIN alert_details ad ON ad.evaluation_id = les.evaluation_history_id
+         INNER JOIN rule_instances AS ri ON ri.id = ere.rule_id
+         INNER JOIN rule_type rt ON rt.id = ri.rule_type_id
+         LEFT JOIN repositories repo ON repo.id = ere.repository_id


I'm not thrilled about this pile of joins, but I don't have a substantially better solution at the moment.

evankanderson · 2024-08-15T17:23:31Z

database/query/profile_status.sql

    repo.repo_name,
    repo.repo_owner,
    repo.provider,


I agree that pulling this repo stuff out here is a bit weird (especially since it seems GitHub-specific!). How much does it improve things if we drop these contents from the RPC? (And, is anyone using them right now / when were they added?)

If we do need them, we could do a second query in the RPC, which feels like a better place, to be honest.

I think we should get rid of them, but removing them will impact the UI (from what I understand, one of the endpoints which uses this query powers the "Alerts" page in the UI) so we need to co-ordinate this change.

What do you think of collecting the repo and other entity details in a separate database call outside of this query?

If the frontend is using that information (and it makes sense that they would), it seems a shame to make them make two RPCs instead of one. At the same time, I don't want this query getting bigger if we can avoid it, so maybe doing some iteration in the Minder server is better than extending the query.

JAORMX · 2024-08-16T06:55:21Z

So, with this change, what would be the output of minder profile status list ... -d ?

…table (#4089)" This reverts commit 2137ce8.

…4195) * Revert "Fix issue with Alert URLs for pull requests and artifacts (#4183)" This reverts commit f5bd045. * Revert "Check for non-empty length of previous metadata (#4182)" This reverts commit 2fddec5. * Revert "Change profile/rule status queries to use evaluation history table (#4089)" This reverts commit 2137ce8.

The rule engine has a copy of the previous evaluation state as part of its context. Prior to this PR, if there is no previous state for a rule/entity pair, an empty struct would be passed around. This requires each part of the code which uses the previous state (particularly remediations and alerts) to test if the fields in the state struct are the default value. This problem was highlighted in PR #4089 when the types of some the fields changed, requiring the code to be changed to have different default checks. This PR changes the logic so that a nil pointer is returned if there is no existing state. This makes the check for a lack of previous state to be consistent irrespective of the fields needed or their type. Validated with unit tests locally.

@rdimitrov

The rule engine has a copy of the previous evaluation state as part of its context. Prior to this PR, if there is no previous state for a rule/entity pair, an empty struct would be passed around. This requires each part of the code which uses the previous state (particularly remediations and alerts) to test if the fields in the state struct are the default value. This problem was highlighted in PR #4089 when the types of some the fields changed, requiring the code to be changed to have different default checks. This PR changes the logic so that a nil pointer is returned if there is no existing state. This makes the check for a lack of previous state to be consistent irrespective of the fields needed or their type. @rdimitrov ran smoke tests locally to validate.

…4089) This change allows most of the functionality which previously used the `rule_evaluations` (and related) tables to use the new evaluation history tables instead.

@rdimitrov

Out of the three commits which were reverted, one of them was replaced by #4197 which solves the problem at source. This will reapply the other two. Some changes were made to fit with the changes introduced by #4197 Validated against smoke tests by @rdimitrov. I also ran the failing smoke tests against this branch locally. * Change profile/rule status queries to use evaluation history table (#4089) This change allows most of the functionality which previously used the `rule_evaluations` (and related) tables to use the new evaluation history tables instead. * Fix issue with Alert URLs for pull requests and artifacts (#4183)

dmjb force-pushed the profile-status-quieries-eval-history branch 5 times, most recently from cab716d to fea7fde Compare August 15, 2024 12:16

dmjb commented Aug 15, 2024

View reviewed changes

dmjb force-pushed the profile-status-quieries-eval-history branch from fea7fde to df4b01b Compare August 15, 2024 13:07

dmjb marked this pull request as ready for review August 15, 2024 13:18

dmjb requested a review from a team as a code owner August 15, 2024 13:18

dmjb marked this pull request as draft August 15, 2024 13:28

dmjb commented Aug 15, 2024

View reviewed changes

dmjb force-pushed the profile-status-quieries-eval-history branch from 1276ed6 to b0ed069 Compare August 15, 2024 14:22

dmjb commented Aug 15, 2024

View reviewed changes

dmjb added 3 commits August 15, 2024 15:46

Change profile/rule status queries to use evaluation history table

7c985b1

This change allows most of the functionality which previously used the `rule_evaluations` (and related) tables to use the new evaluation history tables instead.

use more descriptive CLI column name

ef5a421

left join on repo table

cfa0841

dmjb marked this pull request as ready for review August 15, 2024 14:46

dmjb force-pushed the profile-status-quieries-eval-history branch from b0ed069 to cfa0841 Compare August 15, 2024 14:46

dmjb mentioned this pull request Aug 15, 2024

[DO NOT MERGE] Stop writing to rule evaluation tables #4163

Closed

10 tasks

evankanderson approved these changes Aug 15, 2024

View reviewed changes

dmjb merged commit 2137ce8 into main Aug 16, 2024
21 checks passed

dmjb deleted the profile-status-quieries-eval-history branch August 16, 2024 09:46

rdimitrov added a commit that referenced this pull request Aug 19, 2024

Revert "Change profile/rule status queries to use evaluation history …

5201e46

…table (#4089)" This reverts commit 2137ce8.

rdimitrov mentioned this pull request Aug 19, 2024

Revert profile/rule status queries to use evaluation history table #4195

Merged

10 tasks

dmjb mentioned this pull request Aug 19, 2024

Use nil evaluation state if there is no previous evaluation #4197

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change profile/rule status queries to use evaluation history table #4089

Change profile/rule status queries to use evaluation history table #4089

dmjb commented Aug 6, 2024 •

edited

Loading

coveralls commented Aug 6, 2024 •

edited

Loading

dmjb Aug 15, 2024

evankanderson Aug 15, 2024

evankanderson Aug 15, 2024

dmjb Aug 15, 2024 •

edited

Loading

evankanderson Aug 15, 2024

dmjb Aug 15, 2024 •

edited

Loading

dmjb Aug 15, 2024

evankanderson left a comment

evankanderson Aug 15, 2024

evankanderson Aug 15, 2024

evankanderson Aug 15, 2024

evankanderson Aug 15, 2024

evankanderson Aug 15, 2024

dmjb Aug 15, 2024

evankanderson Aug 15, 2024

JAORMX commented Aug 16, 2024

Change profile/rule status queries to use evaluation history table #4089

Change profile/rule status queries to use evaluation history table #4089

Conversation

dmjb commented Aug 6, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

coveralls commented Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmjb Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmjb Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson 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

JAORMX commented Aug 16, 2024

dmjb commented Aug 6, 2024 •

edited

Loading

coveralls commented Aug 6, 2024 •

edited

Loading

dmjb Aug 15, 2024 •

edited

Loading

dmjb Aug 15, 2024 •

edited

Loading