🧹 Make resolved policy execution use reporting queries #1496
Conversation
| @@ -21,6 +21,7 @@ type query struct { | |||
| codeBundle *llx.CodeBundle | |||
| requiredProps map[string]string | |||
| resolvedProperties map[string]*llx.Primitive | |||
| notifies []string | |||
There was a problem hiding this comment.
this gets filled in with the information from the reporting queries section
policy/executor/internal/builder.go
Outdated
| n := ge.addReportingQueryNode(queryID, q) | ||
| if len(q.notifies) > 0 { | ||
| for _, notify := range q.notifies { | ||
| ge.addEdge(n.id, NodeID(notify)) |
There was a problem hiding this comment.
creates the edge from the node we use to represent a reporting query to the node we use to represent the reporting job. This will be the reporting job for query code id
There was a problem hiding this comment.
Also note that the reporting query node doesn't fully match whats in the resolved policy. We create one of these for every query, scoring or data. Only scoring queries are in the reporting queries section of the resolved policy.
| var s *policy.Score | ||
| if v := nodeData.childScores[nodeData.queryID].score; v == nil { | ||
|
|
||
| if len(nodeData.childScores) == 0 { | ||
| s = &policy.Score{ |
There was a problem hiding this comment.
This is needed because reporting query nodes in our graph don't have children if they're data queries only.
policy/executor/internal/nodes.go
Outdated
| Type: policy.ScoreType_Result, | ||
| } | ||
| } else { | ||
| s = proto.Clone(v).(*policy.Score) |
There was a problem hiding this comment.
not sure how often this gets called but we had some issues with memory/cpu consumption a while back. A lot of those were caused by proto.Clone. It is doing a lot of allocations and reflection magic which is super inefficient. If we need to clone this structure often, it's best to manually write a function that does that
There was a problem hiding this comment.
i can change this in a follow up pr. Would like to avoid it being part of this one
jaym
force-pushed
the
jdm/ng-resolver-rqs
branch
2 times, most recently
from
07879d9
to
b0da7d9
Compare
The execution was previously implicitly tying together reporting queries and reporting jobs. This now uses the ReportingQueries section of the CollectorJob to do this. Also, part of this change, I've introduced an additional reporting job types. One represents the executed query(reporting job by code id). The other represents a thing that is both a check and a query. This was helpful in fixing an issue where we don't properly forward a score and intead rescore it. This would turn skipped scores into unscored. This last point is one of those non-backwards compatible changes. Once the new resolved policy is used everywhere, this bug will be seen in old clients. I don't see a way to fix it in a backwards compatible way, and its a small enough issue that I think its fine.
jaym
force-pushed
the
jdm/ng-resolver-rqs
branch
from
b0da7d9
to
7ccf232
Compare
The execution was previously implicitly tying together reporting queries and reporting jobs. This now uses the ReportingQueries section of the CollectorJob to do this.
Also, part of this change, I've introduced an additional reporting job types. One represents the executed query(reporting job by code id). The other represents a thing that is both a check and a query. This was helpful in fixing an issue where we don't properly forward a score and intead rescore it. This would turn skipped scores into unscored.
This last point is one of those non-backwards compatible changes. Once the new resolved policy is used everywhere, this bug will be seen in old clients. I don't see a way to fix it in a backwards compatible way, and its a small enough issue that I think its fine.
Note: While this should work with the old resolved policy, its probably best to wait to release it with the resolved policy rewrite.