Add query metrics to decision logs #1162
Conversation
| @@ -33,6 +33,7 @@ type Metrics interface { | |||
| Counter(name string) Counter | |||
| All() map[string]interface{} | |||
| Clear() | |||
| GetTimer() map[string]Timer | |||
There was a problem hiding this comment.
I'd rather not extend the metrics package with new APIs, especially since it's just for test purposes inside the decision logger package. Let's remove these and implement the behavior inside the decision logs package as needed.
| @@ -226,6 +232,7 @@ func (p *Plugin) Log(ctx context.Context, decision *server.Info) { | |||
| RequestedBy: decision.RemoteAddr, | |||
| Timestamp: decision.Timestamp, | |||
| Version: version.Version, | |||
| Metrics: serverMetrics, | |||
There was a problem hiding this comment.
Nit: the changes in this function could be more simply expressed as:
// after constructing event := EventV1{...}
if decision.Metrics != nil {
event.Metrics = decision.Metrics.All()
}| @@ -70,6 +72,7 @@ func TestPluginCustomBackend(t *testing.T) { | |||
| func TestPluginStartSameInput(t *testing.T) { | |||
There was a problem hiding this comment.
It's sufficient to test that the metrics are present in one test case. Let's minimize the number of tests that are getting updated in this PR to one.
| @@ -475,3 +506,21 @@ func setVersion(opaVersion string) { | |||
| func getVersion() string { | |||
| return version.Version | |||
| } | |||
|
|
|||
| func CreateMetrics() metrics.Metrics { | |||
There was a problem hiding this comment.
I think it would be cleaner if instead of constructing the metrics object and then zeroing the values, we obtained the metrics keys and checked for their existence. This avoids the int64/float64 issue and avoids the need to zero out values.
Something like this:
func getWellKnownMetricKeys() []string {
// We run evaluation to generate metric keys.
m := metrics.New()
r := rego.New(rego.Query("foo = 1"), rego.Module("foo.rego", "package x"), rego.Metrics(m))
ctx := context.Background()
r.Eval(ctx) // ignore result and error, we only care about the metric keys
var result []string
for k := range m.All() {
result = append(result, k)
}
return result
}Then above we can set the metrics map to nil (so that DeepEqual succeeds) and check that all of the keys exist.
result := chunk4[expLen4-1]
resultMetrics := result.Metrics
result.Metrics = nil
if !reflect.DeepEqual(result, exp) {
// Fail test
}
for _, k := range getWellKnownMetricKeys() {
if _, ok := resultMetrics[k]; !ok {
t.Fatalf("Expected metric key %q but not found. Metrics in event: %v", k, chunk4[expLen4-1].Metrics)
}
}
|
Closing in favour of #1165 |
Fixes #1033