Skip to content
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

feat: add more generalized Stats #283

Merged
merged 7 commits into from
Apr 11, 2023

Conversation

acpana
Copy link
Contributor

@acpana acpana commented Jan 27, 2023

Signed-off-by: Alex Pana 8968914+acpana@users.noreply.github.com

This patch reworks EvaluationMeta field as a more general struct.

The big win here is seeing Stats for non violating constraints
without manipulating the Result struct, which is not possible with the
implementation in #263. This was evident when we tried to integrate in gatekeeper.

reviewer notes

current papercuts

You will notice we modify the driver interface for Query() to give back a QueryResponse type. This breaks compatibility for drivers. We need to modify the Query() signature regardless to give back stateless, per call metrics. Adding the QueryResponse struct helps future proof for further additions.

UX in action

Here's what consuming this, say in g8r, would look like w fmt.Println:

scope: template, statsFor: K8sGoodRego, source: rego, statsName: templateRunTimeNanos, value: 205970, desc: the number of milliseconds it took to evaluate all constraints for a template 
scope: template, statsFor: K8sGoodRego, source: rego, statsName: constraintCount, value: 1, desc: the number of constraints that were evaluated at the same time for the given constraint kind 

These can be logged or sent to a metrics aggregator depending on needs.

call outs

  • note that since we are changing the Result struct, some tests in gatekeeper are failing and that's ok

@acpana acpana changed the title feat: add Reporter interface for generalized stats feat: add more generalized Stats Jan 31, 2023
Copy link
Contributor Author

@acpana acpana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some context;

constraint/pkg/client/drivers/local/driver_unit_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Patch coverage has no change and project coverage change: +1.24 🎉

Comparison is base (fc3dce7) 52.67% compared to head (f31e4ae) 53.92%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   52.67%   53.92%   +1.24%     
==========================================
  Files          67       67              
  Lines        4592     4671      +79     
==========================================
+ Hits         2419     2519     +100     
+ Misses       1902     1885      -17     
+ Partials      271      267       -4     
Flag Coverage Δ
unittests 53.92% <ø> (+1.24%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

constraint/pkg/client/drivers/local/args.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/instrumentation/instrumentation.go Outdated Show resolved Hide resolved
constraint/pkg/instrumentation/instrumentation.go Outdated Show resolved Hide resolved
constraint/pkg/instrumentation/instrumentation.go Outdated Show resolved Hide resolved
constraint/pkg/instrumentation/instrumentation.go Outdated Show resolved Hide resolved
constraint/pkg/instrumentation/instrumentation.go Outdated Show resolved Hide resolved
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the aggregation model that's starting to take shape here.

constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/instrumentation/instrumentation.go Outdated Show resolved Hide resolved
@acpana acpana requested review from davis-haba and maxsmythe and removed request for davis-haba February 7, 2023 07:07
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/local/driver.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
constraint/pkg/client/drivers/query_opts.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/query_opts.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/query_opts.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/rego/driver.go Outdated Show resolved Hide resolved
constraint/pkg/client/drivers/rego/driver.go Outdated Show resolved Hide resolved
constraint/pkg/instrumentation/instrumentation.go Outdated Show resolved Hide resolved
constraint/pkg/client/e2e_test.go Outdated Show resolved Hide resolved
constraint/pkg/client/e2e_test.go Outdated Show resolved Hide resolved
constraint/pkg/client/e2e_test.go Outdated Show resolved Hide resolved
constraint/pkg/client/e2e_test.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
@acpana acpana requested a review from maxsmythe April 3, 2023 21:03
@acpana acpana marked this pull request as ready for review April 3, 2023 23:18
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great job!

@maxsmythe maxsmythe requested a review from sozercan April 4, 2023 02:36
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments, otherwise LGTM

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>
}

desc, err := driver.GetDescriptionForStat(statName)
if err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to bubble this error up?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be persuaded to bubble the error up. I know that Max made a comment to bubble up the error from the driver.GetDescriptionForStat.

For the client UX, I thought it was better for the emission (logging) to not have to handle the error and always get in hand a string, the description requested or an "unknown description" string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should go all-in one direction or the other (either bubble up the error all the way or don't raise an error).

For now, I'm okay with leaving this as-is, we can revamp as we learn more about how this is used.

Copy link
Member

@ritazh ritazh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment
LGTM

@maxsmythe maxsmythe merged commit 3f237e2 into open-policy-agent:master Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants