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.

Sign up for GitHub

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

pkg/cmd: add metric for logical checks #2170

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

pkg/cmd: add metric for logical checks #2170

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions pkg/cmd/server/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/jzelinskie/cobrautil/v2/cobraproclimits"
"github.com/jzelinskie/cobrautil/v2/cobrazerolog"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
"github.com/prometheus/client_golang/prometheus/promhttp"
"github.com/rs/zerolog"
"go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc"
Expand All @@ -30,6 +31,8 @@ import (
"google.golang.org/grpc/codes"

"github.com/authzed/authzed-go/pkg/requestmeta"
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/authzed/grpcutil"

"github.com/authzed/spicedb/internal/dispatch"
"github.com/authzed/spicedb/internal/logging"
Expand Down Expand Up @@ -170,6 +173,7 @@ const (
DefaultMiddlewareGRPCAuth = "grpcauth"
DefaultMiddlewareGRPCProm = "grpcprom"
DefaultMiddlewareServerVersion = "serverversion"
DefaultMiddlewareLogicalChecks = "logicalchecks"

DefaultInternalMiddlewareDispatch = "dispatch"
DefaultInternalMiddlewareDatastore = "datastore"
Expand Down Expand Up @@ -333,6 +337,11 @@ func DefaultUnaryMiddleware(opts MiddlewareOption) (*MiddlewareChain[grpc.UnaryS
WithInterceptor(serverversion.UnaryServerInterceptor(opts.EnableVersionResponse)).
Done(),

NewUnaryMiddleware().
WithName(DefaultMiddlewareLogicalChecks).
WithInterceptor(LogicalChecksMetricUnary).
Done(),

NewUnaryMiddleware().
WithName(DefaultInternalMiddlewareDispatch).
WithInternal(true).
Expand Down Expand Up @@ -406,6 +415,11 @@ func DefaultStreamingMiddleware(opts MiddlewareOption) (*MiddlewareChain[grpc.St
WithInterceptor(serverversion.StreamServerInterceptor(opts.EnableVersionResponse)).
Done(),

NewStreamMiddleware().
WithName(DefaultMiddlewareLogicalChecks).
WithInterceptor(noopStreamInterceptor).
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Done(),

NewStreamMiddleware().
WithName(DefaultInternalMiddlewareDispatch).
WithInternal(true).
Expand All @@ -428,6 +442,10 @@ func DefaultStreamingMiddleware(opts MiddlewareOption) (*MiddlewareChain[grpc.St
return &chain, err
}

func noopStreamInterceptor(srv any, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
return handler(srv, stream)
}

func determineEventsToLog(opts MiddlewareOption) grpclog.Option {
eventsToLog := []grpclog.LoggableEvent{grpclog.FinishCall}
if opts.EnableRequestLog {
Expand Down Expand Up @@ -465,6 +483,29 @@ func DefaultDispatchMiddleware(logger zerolog.Logger, authFunc grpcauth.AuthFunc
}
}

var LogicalChecksCounter = promauto.NewCounterVec(prometheus.CounterOpts{
Namespace: "spicedb",
Subsystem: "logical",
Name: "permission_checks_total",
Comment on lines +487 to +489
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Help: "Count of the checks across individual and bulk requests",
}, []string{"grpc_method", "grpc_service", "grpc_type"})

func LogicalChecksMetricUnary(ctx context.Context, request any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) {
response, err := handler(ctx, request)
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

switch m := request.(type) {
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:
Comment on lines +497 to +503
Copy link
Contributor

Choose a reason for hiding this comment

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

please add unit test testing testing both request types, and with/without error

}
}
return response, err
}

func InterceptorLogger(l zerolog.Logger) grpclog.Logger {
return grpclog.LoggerFunc(func(ctx context.Context, lvl grpclog.Level, msg string, fields ...any) {
l := l.With().Fields(fields).Logger()
Expand Down
Loading