Add perfinsights middleware which writes the latency of API calls #2383
Conversation
github-actions
bot
added
area/CLI
| @@ -0,0 +1,2 @@ | |||
| // Package perfinsights defines middleware that reports the latency of API calls to Prometheus. | |||
There was a problem hiding this comment.
We already have a middleware that does this - the grpc-ecosystem prometheus middleware.
Please clarify what is different about this (e.g. query shapes, more finer grained detail about
the API payload)
| @@ -170,6 +180,8 @@ func checkResultToAPITypes(cr *dispatch.ResourceCheckResult) (v1.CheckPermission | |||
| } | |||
|
|
|||
| func (ps *permissionServer) CheckBulkPermissions(ctx context.Context, req *v1.CheckBulkPermissionsRequest) (*v1.CheckBulkPermissionsResponse, error) { | |||
| perfinsights.SetInContext(ctx, perfinsights.NoLabels) | |||
There was a problem hiding this comment.
Add a comment clarifying insights are added down the line for the individual check elements
| DispatchChunkSize: defaultIfZero(config.DispatchChunkSize, 100), | ||
| MaxCheckBulkConcurrency: defaultIfZero(config.MaxCheckBulkConcurrency, 50), | ||
| CaveatTypeSet: caveattypes.TypeSetOrDefault(config.CaveatTypeSet), | ||
| ExpiringRelationshipsEnabled: config.ExpiringRelationshipsEnabled, |
There was a problem hiding this comment.
was expiring rels getting enabled by default?
There was a problem hiding this comment.
Only for the relationships APIs, but since it wasn't enabled for schema, it was basically a no-op (except it caused us to request the column)
|
|
||
| func labelsForFilter(filter *v1.RelationshipFilter) perfinsights.APIShapeLabels { | ||
| if filter == nil { | ||
| return perfinsights.APIShapeLabels{} |
There was a problem hiding this comment.
| return perfinsights.APIShapeLabels{} | |
| return perfinsights.NoLabels |
There was a problem hiding this comment.
Changed here and everywhere else
| @@ -575,3 +592,23 @@ func checkIfFilterIsEmpty(filter *v1.RelationshipFilter) error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func labelsForFilter(filter *v1.RelationshipFilter) perfinsights.APIShapeLabels { | |||
| } | ||
|
|
||
| // UnaryServerInterceptor returns a gRPC server-side interceptor that provides reporting for Unary RPCs. | ||
| func UnaryServerInterceptor(isEnabled bool) grpc.UnaryServerInterceptor { |
There was a problem hiding this comment.
Please create a unit test that mounts this interceptor over a gRPC server and confirms a request generates the metrics
| return | ||
| } | ||
|
|
||
| builder := ctxKey.Value(r.ctx) |
There was a problem hiding this comment.
super nit: maybe "shapeClosure"?
| if exemplarObserver, ok := o.(prometheus.ExemplarObserver); ok { | ||
| if spanCtx := trace.SpanContextFromContext(ctx); spanCtx.HasTraceID() { | ||
| traceID := prometheus.Labels{"traceID": spanCtx.TraceID().String()} | ||
| exemplarObserver.ObserveWithExemplar(duration.Seconds(), traceID) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
this also def needs testing
There was a problem hiding this comment.
The plan is to do end-to-end testing with this
| } | ||
|
|
||
| o := APIShapeLatency.WithLabelValues(labels...) | ||
| if exemplarObserver, ok := o.(prometheus.ExemplarObserver); ok { |
There was a problem hiding this comment.
Is there a way to type assert this into a global var, instead of keeping the base interface?
There was a problem hiding this comment.
Not in a nice way :/
| FilterLabel APIShapeLabel = "filter" | ||
| ) | ||
|
|
||
| var allLabels = []string{ |
There was a problem hiding this comment.
why not a slice of APIShapeLabel? you seem to be type-asserting in the main for loop
There was a problem hiding this comment.
We pass it as labels, and those must be strings
to a new native histogram bucket Should help users determine top-line API latency
josephschorr
force-pushed
the
perf-insights
branch
from
585adef to
40b86cf
Compare
|
Updated |
| // Unlike the gRPC middleware, the perf insights middleware records API calls by "shape", versus | ||
| // aggregating all calls simply by API kind. The shapes allow for more granular determination of |
There was a problem hiding this comment.
Nitpick: to me, "shape" and "kind" are kind of synonyms. Might be good to be define "shape"
EDIT: ah, it's defined in the code itself 👍 disregard
to a new native histogram bucket
Should help users determine top-line API latency