-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add EvaluationMeta empty interface to each Result #263
add EvaluationMeta empty interface to each Result #263
Conversation
71674ad
to
10743c9
Compare
@maxsmythe let me know what you think of the open questions above! 🙏🏼 |
constraint/pkg/client/client.go
Outdated
@@ -588,6 +595,13 @@ func (c *Client) review(ctx context.Context, target string, constraints []*unstr | |||
Trace: trace, | |||
Target: target, | |||
Results: results, | |||
ResponseMeta: &types.ResponseMeta{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intended use case for this feature would be: if I have 5 constraint templates and one of them is slow, I would like to identify which template is the culprit.
In order to do that, I'd need to be able to associate a given constraint template with its total runtime.
TL;DR we need to break down these Meta
objects by constraint template name... probably by modifying the engine interface to allow that.
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
10743c9
to
bc2edef
Compare
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
hey @maxsmythe thanks for the guidance here. Starting with 8057241, we have an interface exposing an |
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one question around whether we want to bake units into the field name for clarity. Thanks for this!
// RegoEvaluationMeta has rego specific metadata from evaluation. | ||
type RegoEvaluationMeta struct { | ||
// TemplateRunTime is the number of milliseconds it took to evaluate all constraints for a template. | ||
TemplateRunTime float64 `json:"templateRunTime"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add the units to this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we avoid that given that the godoc for the field calls out the units?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K8s API conventions prefer integrating them into the field:
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
AFAIU, the Here's an old gatekeeper test run: https://github.com/open-policy-agent/frameworks/actions/runs/3586143965/jobs/6037544599 (it seems mostly gator test related changes needed) |
This shouldn't be a breaking change, I'd expect Gatekeeper Test to pass, that being said, that test is informational |
Looking at the failure you linked, it seems like some of G8r's tests are written in a brittle fashion, such that the addition of a new field causes them to break. These are benign, though it does mean updating the dependency will take some work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
what
This patch adds an empty interface to keep evaluation metadata about a given
Result
.why
By collecting these measurements, we can expose them to consumers, like
gatekeeper
to use for reporting or logging.reviewer notes:
Signed-off-by: Alex Pana 8968914+acpana@users.noreply.github.com