pkg/cmd: add metric for logical checks #2170
Conversation
jzelinskie
added
priority/3 low
This tracks the "logical checks" which includes not only check requests but each item in a bulk check request.
jzelinskie
force-pushed
the
logic-checks
branch
from
caebe8b to
8ffa034
Compare
|
|
||
| func LogicalChecksMetricUnary(ctx context.Context, request any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { | ||
| response, err := handler(ctx, request) | ||
| if err == nil { |
There was a problem hiding this comment.
Why do we want to exclude errors? For a 100 elements bulk check, 99 could have been successfully executed, but the last one took longer and made the bulk check fail. Do we not count the other successful 99?
Perhaps you could add another label error that is true if the request failed.
| @@ -406,6 +415,11 @@ func DefaultStreamingMiddleware(opts MiddlewareOption) (*MiddlewareChain[grpc.St | |||
| WithInterceptor(serverversion.StreamServerInterceptor(opts.EnableVersionResponse)). | |||
| Done(), | |||
|
|
|||
| NewStreamMiddleware(). | |||
| WithName(DefaultMiddlewareLogicalChecks). | |||
| WithInterceptor(noopStreamInterceptor). | |||
There was a problem hiding this comment.
Should it be marked internal? I lean towards no, just thinking out loud. Internal middlewares should not be replaced or removed because they are crucial to SpiceDB's correct functioning. Nothing would break in SpiceDB, but if the metric disappears, it will affect folks running SpiceDB who rely on it.
| case *v1.CheckPermissionRequest: | ||
| svc, method := grpcutil.SplitMethodName(info.FullMethod) | ||
| LogicalChecksCounter.WithLabelValues(method, svc, "unary").Add(1) | ||
| case *v1.CheckBulkPermissionsRequest: | ||
| svc, method := grpcutil.SplitMethodName(info.FullMethod) | ||
| LogicalChecksCounter.WithLabelValues(method, svc, "unary").Add(float64(len(m.GetItems()))) | ||
| default: |
There was a problem hiding this comment.
please add unit test testing testing both request types, and with/without error
| Namespace: "spicedb", | ||
| Subsystem: "logical", | ||
| Name: "permission_checks_total", |
There was a problem hiding this comment.
Is there a better namespace for this metric? I'm not sure "logical" is a subsystem. Should it be aligned with the namespace of the grpc prom middleware? or perhaps the subsystem is a permissions service?
This tracks the "logical checks" which includes not only check requests but each item in a bulk check request.