Added ListEvaluationHistory RPC implementation. #3784

blkt · 2024-07-04T12:40:31Z

Summary

ListEvaluationHistory RPC returns a paginated list of evaluation events based on the given cursor and filter.

Routines for managing a filter are implemented as well, in a way that should be reusable within other endpoints supporting filters. This implementation only allows and-joined predicates. Such predicates allow only or-joined equality/inequality checks. Simply put, if a filter entry starts with the exclamation mark, it is added to the inequality check, while it is added to the equality check otherwise. Finally, timerange based filtering is supported.

Routines for managing a cursor are implemented as well, but are not intended to be generic. Cursors are tightly coupled with the underlying extraction logic and are harder to refactor, and the additional effort was not considered valuable at this time.

Fixes #3746

Change Type

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

unit tests
manual tests (locally)

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.

proto/minder/v1/minder.proto

coveralls · 2024-07-04T12:54:19Z

coverage: 52.772% (+0.8%) from 52.002%
when pulling 01b6441 on history-service-impl
into ff2be84 on main.

coveralls · 2024-07-04T13:44:38Z

coverage: 52.777% (+0.8%) from 52.002%
when pulling 474f589 on history-service-impl
into ff2be84 on main.

coveralls · 2024-07-04T16:09:45Z

coverage: 52.984% (+0.8%) from 52.213%
when pulling d06da73 on history-service-impl
into 2e004e1 on main.

eleftherias · 2024-07-05T07:20:56Z

database/query/eval_history.sql

+        OR sqlc.narg(tots)::timestamp without time zone IS NULL
+        OR s.most_recent_evaluation BETWEEN sqlc.narg(fromts) AND sqlc.narg(tots))
+ ORDER BY s.most_recent_evaluation DESC
+ LIMIT sqlc.arg(size)::integer;


This may have already been discussed, but have you considered using a SQL generator, like https://github.com/Masterminds/squirrel? I'm worried this raw SQL will become unmaintainable and fragile, especially as we add these types of filters to more endpoints.

I started off thinking it was not possible to implement this without an SQL generator, and decided to prove that.
It turned out to be possible instead, so I haven't looked any further.

It is worth noting that the simple filtering logic we decided to implement only allows where conditions of the form

(X = val1 OR X = val2) AND (Y = val3 OR Y = val4)

These conditions are always definable in static SQL, with an additional column1 IS NOT NULL OR at the beginning. Filtering and pagination as discussed in design docs do not allow for more complex filters, and in such cases general search should be implemented instead.

I was not aware of the existence of squirrel, while I originally considered goqu, I'll have a look at both. Besides, the query is fairly efficient and using sqlc we have all the structs and bindings autogenerated, which I don't think is the case using squirrel.

I understand the query generator approach may still be preferable, I'll test squirrel on top of this branch to see what's the impact.

coveralls · 2024-07-05T09:54:19Z

coverage: 52.989% (+0.8%) from 52.204%
when pulling f48bcf2 on history-service-impl
into 6274fe5 on main.

internal/history/service.go

internal/history/models.go

coveralls · 2024-07-08T12:28:51Z

coverage: 53.085% (+1.0%) from 52.131%
when pulling e1c933d on history-service-impl
into 3931da3 on main.

proto/minder/v1/minder.proto

evankanderson · 2024-07-08T16:38:49Z

proto/minder/v1/minder.proto

+    message Entity {
+        // id is the ID of the entity.
+        string id = 1;

-    // rule contains details of the rule which the entity was evaluated against.
-    EvaluationHistoryRule rule = 2;
+        // type is the entity type.
+        minder.v1.Entity type = 2;

-    // status contains the evaluation status.
-    EvaluationHistoryStatus status = 3;
+        // name is the entity name.
+        string name = 3;
+    }


Two comments:

I've rarely been happy in the future with where a nested message has been placed -- I'd lean towards making this a top-level message.

Can you include comments about uniqueness of the various fields, and ability to use those fields in future calls? (For example, I assume that id is globally unique, but is name globally unique, or only unique within the scope of a Provider?)

@eleftherias on provider-scoping entity names, as I know she was looking to remove that linkage (and I was advocating for keeping it).

evankanderson · 2024-07-08T16:40:19Z

proto/minder/v1/minder.proto

+    message Rule {
+        // name is the name of the rule instance.
+        string name = 1;

-    // remediation contains details of the remediation for this evaluation.
-    EvaluationHistoryRemediation remediation = 5;
-}
+        // type is the name of the rule type.
+        string type = 2;

-message EvaluationHistoryEntity {
-    // id is the ID of the entity.
-    string id = 1;
+        // profile is the name of the profile which contains the rule.
+        string profile = 3;
+    }


It feels odd to have type fields in both the Entity and Rule messages that mean different things -- Entity.type is an enum (IIRC), while Rule.type is effectively the name of a template. Can we call this rule_type to disambiguate?

(Also, the lack of id here means that profile + name is the distinguishing set of fields for future follow-up queries.)

@evankanderson Are you referring to the rule type ID, or the rule instance ID?

I think it would be a rule instance ID if you wanted to be able to do a unique follow-up query. (i.e. looking for more history for items which don't have any history or have some suspicious changes over a shorter time window)

evankanderson · 2024-07-08T16:42:37Z

proto/minder/v1/minder.proto

+        // details contains optional details about the evaluation.
+        // the structure and contents are rule type specific, and are subject to change.
+        string details = 2;


I'm not a big fan of having a top-level stringly-typed details that has "rule type specific" contents and structure. That may be where we are at right now, but it seems like this could mean that one rule emits markdown, another plain-text, and a third XML fragments.

Same comment as for line 2604

evankanderson · 2024-07-08T16:43:22Z

proto/minder/v1/minder.proto

+        // details contains optional details about the remediation.
+        // the structure and contents are remediation specific, and are subject to change.
+        string details = 2;


Ditto on stringly-typed details. Again, I'm willing to accept this as a bridge, but we should get to a more well-defined outcome.

This was discussed during the design review - the proposal was to match the current state of Minder and work on improving remediations/alerts later.

evankanderson · 2024-07-08T16:44:32Z

proto/minder/v1/minder.proto

-    // type is the name of the rule type.
-    string type = 2;
+    message Remediation {
+        // status is one of (success, error, failure, skipped, not available)


It seems like Remediation has one extra status (not available) compared with Status. It feels odd to me that Remediation isn't part of the status and doesn't have a Timestamp -- can we have a Remediation without a Status, or a different time-scale?

Right now, we can't have a remediation that happens at a different time to an evaluation (within the margin of a few seconds or so). The engine also tracks the status of the evaluation and the remediation separately.

evankanderson · 2024-07-08T16:45:48Z

proto/minder/v1/minder.proto

+    message Alert {
+        // status is one of (on, off, error, skipped, not available)
+        // not using enums to mirror the behaviour of the existing API contracts.
+        string status = 1;

-    // details contains optional details about the evaluation.
-    // the structure and contents are rule type specific, and are subject to change.
-    string details = 2;
-}
+        // details contains optional details about the alert.
+        // the structure and contents are alert specific, and are subject to change.
+        string details = 2;
+    }


The comments on Remediation also apply to Alert. In fact, I'm wondering whether an Alert is a particular type of remediation. @puerco @ethomson

At this moment in time, they might. I am not so sure that they will remain the same as we evolve them in future.

evankanderson · 2024-07-08T16:51:36Z

proto/minder/v1/minder.proto

+    // status contains the evaluation status.
+    EvaluationHistory.Status status = 3;
+
+    // alert contains details of the alerts for this evaluation.
+    EvaluationHistory.Alert alert = 4;

-    // details contains optional details about the alert.
-    // the structure and contents are alert specific, and are subject to change.
-    string details = 2;
+    // remediation contains details of the remediation for this evaluation.
+    EvaluationHistory.Remediation remediation = 5;


Do we want a single status / alert / remediation for each entry? It feels like we'll end up repeating a lot of the envelope (rule, entity) many times when querying history. I'd sort of prefer to see something like this:

message HistoryResult { Timestamp start = 1; uint32 occurrences = 2; string status = 3; // Include both rule eval and alert / remediate result // Temp, while we work out schema string status_detail = 4; string alert_detail = 5; string remediate_detail = 6; } message EntityRuleEvaluationHistory { EntityInfo entity = 1; RuleInfo rule = 2; // history is ordered from most recent to oldest start time. (reverse sort) repeated HistoryResult history = 3; } message ListEvaluationHistoryResult { Pagination pagination = 1; // History for one entity will be complete in results before there is history for another entity. (e.g. paging is by entity, not within an entity's history) repeated EntityRuleEvaluationHistory results = 2; }

I am confused by this comment, both ListEvaluationHistoryResult and EntityRuleEvaluationHistory embed a repeated HistoryResult, that's a typo, right?

I don't know how to make progress on this, I'd be happy to discuss if we want to change message structure.

Oops, I meant for ListEvaluationHistoryResult to embed a repeated EntityRuleEvaluationHistory. Updated.

It turns out that we're trying to present something different (a log-history based view of history), so my suggestion doesn't really work.

evankanderson

A few more comments looking at the SQL query (which I'm very impressed by!).

database/query/eval_history.sql

proto/minder/v1/minder.proto

ListEvaluationHistory RPC returns a paginated list of evaluation events based on the given cursor and filter. Routines for managing a filter are implemented as well, in a way that should be reusable within other endpoints supporting filters. This implementation only allows and-joined predicates. Such predicates allow only or-joined equality/inequality checks. Simply put, if a filter entry starts with the exclamation mark, it is added to the inequality check, while it is added to the equality check otherwise. Finally, timerange based filtering is supported. Routines for managing a cursor are implemented as well, but are not intended to be generic. Cursors are tightly coupled with the underlying extraction logic and are harder to refactor, and the additional effort was not considered valuable at this time. Fixes #3746

This was necessary for local testing because of the following issue. #3775

This reverts commit 572ab4e.

ListEvaluationHistory RPC returns a paginated list of evaluation events based on the given cursor and filter. Routines for managing a filter are implemented as well, in a way that should be reusable within other endpoints supporting filters. This implementation only allows and-joined predicates. Such predicates allow only or-joined equality/inequality checks. Simply put, if a filter entry starts with the exclamation mark, it is added to the inequality check, while it is added to the equality check otherwise. Finally, timerange based filtering is supported. Routines for managing a cursor are implemented as well, but are not intended to be generic. Cursors are tightly coupled with the underlying extraction logic and are harder to refactor, and the additional effort was not considered valuable at this time. Fixes #3746

blkt self-assigned this Jul 4, 2024

blkt requested a review from a team as a code owner July 4, 2024 12:40

blkt commented Jul 4, 2024

View reviewed changes

proto/minder/v1/minder.proto Outdated Show resolved Hide resolved

blkt force-pushed the history-service-impl branch from 474f589 to d06da73 Compare July 4, 2024 16:00

eleftherias reviewed Jul 5, 2024

View reviewed changes

blkt force-pushed the history-service-impl branch from d06da73 to f48bcf2 Compare July 5, 2024 09:45

dmjb reviewed Jul 8, 2024

View reviewed changes

internal/history/service.go Outdated Show resolved Hide resolved

jhrozek reviewed Jul 8, 2024

View reviewed changes

internal/history/models.go Show resolved Hide resolved

jhrozek reviewed Jul 8, 2024

View reviewed changes

internal/history/models.go Outdated Show resolved Hide resolved

jhrozek reviewed Jul 8, 2024

View reviewed changes

internal/history/models.go Outdated Show resolved Hide resolved

blkt force-pushed the history-service-impl branch from e3cbf20 to 1edbb12 Compare July 8, 2024 12:24

evankanderson reviewed Jul 8, 2024

View reviewed changes

database/query/eval_history.sql Outdated Show resolved Hide resolved

database/query/eval_history.sql Show resolved Hide resolved

database/query/eval_history.sql Outdated Show resolved Hide resolved

dmjb reviewed Jul 9, 2024

View reviewed changes

proto/minder/v1/minder.proto Outdated Show resolved Hide resolved

blkt force-pushed the history-service-impl branch from 073c41f to 4f9a857 Compare July 9, 2024 13:01

blkt mentioned this pull request Jul 10, 2024

ListEvaluationHistory query is generated dynamically. #3796

Closed

10 tasks

blkt added 9 commits July 10, 2024 19:00

Handler-to-Service wiring.

bdf55ef

Filter and Cursor routines fully tested.

ea8bbf3

Added extraction form database.

c4e7618

Added service tests.

dfbd2d2

Prevent filtering for both inclusion and exclusion.

b7b255b

Revert changes to feature flag check.

fd073f2

This was necessary for local testing because of the following issue. #3775

Setting query params in a more readable fashion.

280dd88

Refactoring cursor parser for better readability.

dc7e1c9

blkt added 3 commits July 10, 2024 19:01

Embed proto messages specific to EvaluationHistory.

f332e1d

Revert "Embed proto messages specific to EvaluationHistory."

e5f701d

This reverts commit 572ab4e.

Added project id as mandatory filter.

c9b4400

blkt force-pushed the history-service-impl branch 2 times, most recently from c873780 to b3f5df7 Compare July 10, 2024 17:09

Minor tweaks to protobuf messages.

e1c933d

blkt force-pushed the history-service-impl branch from b3f5df7 to e1c933d Compare July 10, 2024 17:11

evankanderson approved these changes Jul 10, 2024

View reviewed changes

blkt merged commit 87416ec into main Jul 10, 2024
21 of 22 checks passed

blkt deleted the history-service-impl branch July 10, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ListEvaluationHistory RPC implementation. #3784

Added ListEvaluationHistory RPC implementation. #3784

blkt commented Jul 4, 2024

coveralls commented Jul 4, 2024

coveralls commented Jul 4, 2024

coveralls commented Jul 4, 2024

eleftherias Jul 5, 2024

blkt Jul 5, 2024

coveralls commented Jul 5, 2024

coveralls commented Jul 8, 2024 •

edited

Loading

evankanderson Jul 8, 2024

evankanderson Jul 8, 2024

evankanderson Jul 8, 2024

dmjb Jul 9, 2024

evankanderson Jul 9, 2024

evankanderson Jul 8, 2024

dmjb Jul 9, 2024

evankanderson Jul 8, 2024

dmjb Jul 9, 2024

evankanderson Jul 8, 2024

dmjb Jul 9, 2024

evankanderson Jul 8, 2024

dmjb Jul 9, 2024

evankanderson Jul 8, 2024 •

edited

Loading

blkt Jul 9, 2024

evankanderson Jul 9, 2024

evankanderson Jul 10, 2024

evankanderson left a comment

Added ListEvaluationHistory RPC implementation. #3784

Added ListEvaluationHistory RPC implementation. #3784

Conversation

blkt commented Jul 4, 2024

Summary

Change Type

Testing

Review Checklist:

coveralls commented Jul 4, 2024

coveralls commented Jul 4, 2024

coveralls commented Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 5, 2024

coveralls commented Jul 8, 2024 • edited Loading

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

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

evankanderson Jul 8, 2024 • edited Loading

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

evankanderson left a comment

Choose a reason for hiding this comment

coveralls commented Jul 8, 2024 •

edited

Loading

evankanderson Jul 8, 2024 •

edited

Loading