Add MetricsQueryService grcp handler #3091

albertteoh · 2021-06-15T10:55:29Z

Signed-off-by: albertteoh [email protected]

Which problem is this PR solving?

Relates to "Monitor" tab for service health metrics #2954.

Short description of the changes

Adds the gRPC handler.

albertteoh · 2021-06-15T10:56:19Z

cmd/query/app/http_handler.go

-	defaultDependencyLookbackDuration = time.Hour * 24
-	defaultTraceQueryLookbackDuration = time.Hour * 24 * 2
-	defaultAPIPrefix                  = "api"


moved to default_params.go so all defaults are in one file, given they are shared between gRPC and HTTP handlers.

albertteoh · 2021-06-15T10:59:22Z

model/proto/metrics/metricsquery.proto

@@ -84,7 +84,7 @@ message MetricsQueryBaseRequest {
 message GetLatenciesRequest {
  MetricsQueryBaseRequest baseRequest = 1;
  // quantile is the quantile to compute from latency histogram metrics.
-  // Valid range: 0 - 1 (inclusive).
+  // Valid range: (0, 1]


This is non-nullable because it's mandatory for the GetLatencies endpoint. However, if a client fails to provide the Quantile, its zero value will be used which is 0.0, which should be caught and error returned.

albertteoh · 2021-06-15T11:01:08Z

cmd/query/app/grpc_handler.go

 }

 // NewGRPCHandler returns a GRPCHandler
-func NewGRPCHandler(queryService *querysvc.QueryService, logger *zap.Logger, tracer opentracing.Tracer) *GRPCHandler {
+func NewGRPCHandler(queryService *querysvc.QueryService, metricsQueryService querysvc.MetricsQueryService, logger *zap.Logger, tracer opentracing.Tracer, clock clock) *GRPCHandler {


is it worth adopting the functional options pattern here given the list of params is growing?

Lately I've been trying to stay away from functional options and just use an options/params struct, it's less work to implement and has better discoverability.

In this concrete example, we don't even need any additional struct, just remove this function and initialize GRPCHandler directly

codecov · 2021-06-15T11:02:29Z

Codecov Report

Merging #3091 (a26afca) into master (c9d6957) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3091      +/-   ##
==========================================
+ Coverage   95.82%   95.91%   +0.09%     
==========================================
  Files         235      235              
  Lines       10067    10143      +76     
==========================================
+ Hits         9647     9729      +82     
+ Misses        348      344       -4     
+ Partials       72       70       -2

Impacted Files	Coverage Δ
cmd/query/app/http_handler.go	`100.00% <ø> (ø)`
cmd/query/app/grpc_handler.go	`98.62% <100.00%> (+1.28%)`	⬆️
cmd/query/app/server.go	`97.20% <100.00%> (+1.58%)`	⬆️
cmd/query/app/static_handler.go	`96.77% <0.00%> (+1.61%)`	⬆️
pkg/config/tlscfg/cert_watcher.go	`94.80% <0.00%> (+2.59%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9d6957...a26afca. Read the comment docs.

Signed-off-by: albertteoh <[email protected]>

yurishkuro · 2021-06-15T15:38:18Z

cmd/query/app/grpc_handler.go

@@ -177,3 +190,123 @@ func (g *GRPCHandler) GetDependencies(ctx context.Context, r *api_v2.GetDependen

 	return &api_v2.GetDependenciesResponse{Dependencies: dependencies}, nil
 }
+
+// GetLatencies is the gRPC handler to fetch latency metrics.
+func (g *GRPCHandler) GetLatencies(ctx context.Context, r *metrics.GetLatenciesRequest) (*metrics.GetMetricsResponse, error) {


recently we've seen some tickets showing that apparently with some client libs it's possible to send a request where r==nil, which crashes the server.

I see you already have this check in g.newBaseQueryParameters, but should call this first then

yeah, at least the grpc clients generated in this project fail to marshal a nil message over the wire; but I trust in what you've seen from other client libs, so I've added a nil check for the request.

Note, the original nil check in g.newBaseQueryParameters was just for the MetricsQueryBaseRequest, which is a common attribute among all the request objects, but it doesn't check the parent request for nil.

yurishkuro · 2021-06-15T15:40:45Z

cmd/query/app/grpc_handler.go

+	switch {
+	case errors.Is(err, disabled.ErrDisabled):
+		return errGRPCMetricsQueryDisabled
+	case errors.Is(err, errMissingServiceNames), errors.Is(err, errMissingQuantile):


rather than enumerating, could we check if it's already an instance of status.Error?

yurishkuro · 2021-06-15T15:45:54Z

cmd/query/app/util.go

 	"github.com/jaegertracing/jaeger/storage/spanstore"
 )

+// clock is a wrapper around the time package to allow mocking of the current time.


You can avoid this type by using the function signature directly func() time.Time, and then pass time.Now as main impl

yurishkuro

lgtm, sans minor nits

Signed-off-by: albertteoh <[email protected]>

albertteoh · 2021-06-16T01:30:33Z

cmd/query/app/grpc_handler.go

@@ -177,3 +190,123 @@ func (g *GRPCHandler) GetDependencies(ctx context.Context, r *api_v2.GetDependen

 	return &api_v2.GetDependenciesResponse{Dependencies: dependencies}, nil
 }
+
+// GetLatencies is the gRPC handler to fetch latency metrics.
+func (g *GRPCHandler) GetLatencies(ctx context.Context, r *metrics.GetLatenciesRequest) (*metrics.GetMetricsResponse, error) {


yeah, at least the grpc clients generated in this project fail to marshal a nil message over the wire; but I trust in what you've seen from other client libs, so I've added a nil check for the request.

Note, the original nil check in g.newBaseQueryParameters was just for the MetricsQueryBaseRequest, which is a common attribute among all the request objects, but it doesn't check the parent request for nil.

albertteoh · 2021-06-16T01:36:47Z

cmd/query/app/grpc_handler.go

+	switch v := r.(type) {
+	case *metrics.GetLatenciesRequest:
+		baseRequest = v.BaseRequest
+	case *metrics.GetCallRatesRequest:
+		baseRequest = v.BaseRequest
+	case *metrics.GetErrorRatesRequest:
+		baseRequest = v.BaseRequest
+	}


is there a better way?

The reason for taking this approach is to avoid the same nil check in each of the endpoint handlers (GetLatencies, etc...) so they can just pass the parent request object in, and have a single place to perform the nil check.

The trade-off is given each of these request objects belong to different types, but I need to access the common BaseRequest out of each request, a type switch was necessary.

albertteoh · 2021-06-16T09:46:34Z

Thanks Yuri!

albertteoh commented Jun 15, 2021

View reviewed changes

Add grcp handler

313d636

Signed-off-by: albertteoh <[email protected]>

albertteoh force-pushed the 2954-grpc-handler branch from 2698575 to 313d636 Compare June 15, 2021 11:07

albertteoh marked this pull request as ready for review June 15, 2021 11:25

albertteoh requested a review from a team as a code owner June 15, 2021 11:25

albertteoh requested a review from jpkrohling June 15, 2021 11:25

yurishkuro changed the title ~~Add grcp handler~~ Add MetricsQueryService grcp handler Jun 15, 2021

yurishkuro reviewed Jun 15, 2021

View reviewed changes

yurishkuro previously approved these changes Jun 15, 2021

View reviewed changes

Address review comments

ea1dcac

Signed-off-by: albertteoh <[email protected]>

albertteoh dismissed yurishkuro’s stale review via ea1dcac June 16, 2021 01:00

Check nil request

a26afca

Signed-off-by: albertteoh <[email protected]>

albertteoh commented Jun 16, 2021

View reviewed changes

albertteoh mentioned this pull request Jun 16, 2021

"Monitor" tab for service health metrics #2954

Closed

14 tasks

yurishkuro approved these changes Jun 16, 2021

View reviewed changes

yurishkuro merged commit e15c981 into jaegertracing:master Jun 16, 2021

albertteoh deleted the 2954-grpc-handler branch June 16, 2021 09:46

jpkrohling added this to the Release 1.24.0 milestone Jun 22, 2021

vprithvi mentioned this pull request Dec 2, 2022

[Bug]: cmd/query/app.NewGRPCHandler removed without alternatives #4082

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MetricsQueryService grcp handler #3091

Add MetricsQueryService grcp handler #3091

albertteoh commented Jun 15, 2021

albertteoh Jun 15, 2021

albertteoh Jun 15, 2021 •

edited

Loading

albertteoh Jun 15, 2021

yurishkuro Jun 15, 2021

yurishkuro Jun 15, 2021

codecov bot commented Jun 15, 2021 •

edited

Loading

yurishkuro Jun 15, 2021

yurishkuro Jun 15, 2021

albertteoh Jun 16, 2021

yurishkuro Jun 15, 2021

yurishkuro Jun 15, 2021

yurishkuro left a comment

albertteoh Jun 16, 2021

albertteoh Jun 16, 2021

albertteoh commented Jun 16, 2021

Add MetricsQueryService grcp handler #3091

Add MetricsQueryService grcp handler #3091

Conversation

albertteoh commented Jun 15, 2021

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

albertteoh Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 15, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh commented Jun 16, 2021

albertteoh Jun 15, 2021 •

edited

Loading

codecov bot commented Jun 15, 2021 •

edited

Loading