Add metrics query capability to query service #3061
Conversation
Signed-off-by: albertteoh <albert.teoh@logz.io>
Codecov Report
@@ Coverage Diff @@
## master #3061 +/- ##
==========================================
+ Coverage 95.84% 95.85% +0.01%
==========================================
Files 233 234 +1
Lines 10057 10071 +14
==========================================
+ Hits 9639 9654 +15
Misses 347 347
+ Partials 71 70 -1
Continue to review full report at Codecov.
|
Signed-off-by: albertteoh <albert.teoh@logz.io>
| @@ -51,6 +54,9 @@ type QueryService struct { | |||
| options QueryServiceOptions | |||
| } | |||
|
|
|||
| // QSOption is the functional option for configuring QueryServiceOptions. | |||
| type QSOption func(qOpts *QueryServiceOptions) | |||
There was a problem hiding this comment.
Why do we need this as public?
There was a problem hiding this comment.
The original motivation was to use it here:
Line 125 in c5642b7
to avoid modifying the caller's signature if metrics querying is opted-out, but I think it's not a particularly large impact to simply add it as a parameter and pass in nil if opted-out, so I've removed this option for now.
| @@ -125,6 +131,32 @@ func (qs QueryService) GetDependencies(ctx context.Context, endTs time.Time, loo | |||
| return qs.dependencyReader.GetDependencies(ctx, endTs, lookback) | |||
| } | |||
|
|
|||
| // MetricsQueryEnabled returns whether if metric query capabilities are enabled, opted-in by setting the | |||
| // METRICS_STORAGE_TYPE environment variable. | |||
| func (qs QueryService) MetricsQueryEnabled() bool { | |||
There was a problem hiding this comment.
Why is this needed as public? For tests you can check qs.options directly
There was a problem hiding this comment.
To allow the HTTP and gRPC handlers to return early if the metrics query endpoints are called but not enabled like so:
HTTP Handler:
if !aH.queryService.MetricsQueryEnabled() {
aH.handleError(w, errMetricsQueryDisabled, http.StatusNotImplemented)
return
}
GRPC Handler:
if !g.queryService.MetricsQueryEnabled() {
return nil, status.Error(codes.Unimplemented, "metrics querying is currently disabled")
}
An alternative could be to remove this endpoint and have the QueryService.GetLatencies, etc. functions check for nil MetricsReader then return a MetricsQueryDisabled error type and use errors.Is to handle the "not implemented" case.
What do you think?
There was a problem hiding this comment.
Personally I would not design it this way. If you expose a method on the API like GetLatencies, it should be safe to call this method. It may return an error, but it should not crash the process. So the nil check has to be inside the service implementation.
Another way to do this is to have a different API altogether, MetricsQueryService. That would work better with your approach, but instead of having MetricsQueryEnabled() the RPC handler will simply check if the service reference is not nil.
There was a problem hiding this comment.
Okay, thanks for the suggestion, I like the MetricsQueryService idea as it mirrors the proto service design as well.
There was a problem hiding this comment.
@yurishkuro I've moved metrics-related endpoints into a new MetricsQueryService and also added nil checks from each endpoint.
|
|
||
| // GetLatencies is the queryService implementation of metricsstore.Reader. | ||
| func (qs QueryService) GetLatencies(ctx context.Context, params *metricsstore.LatenciesQueryParameters) (*metrics.MetricFamily, error) { | ||
| return qs.options.MetricsReader.GetLatencies(ctx, params) |
There was a problem hiding this comment.
I think you will want to test for nil
There was a problem hiding this comment.
As above, these nil checks are performed by the handlers so at this point, MetricsReader should never be nil. However, if we take the proposed alternative of introducing a MetricsQueryDisabled error, then we could perform the nil check here.
|
@yurishkuro, are there any further questions/concerns that need addressing in this PR? |
Signed-off-by: albertteoh <albert.teoh@logz.io>
|
|
||
| // GetLatencies is the queryService implementation of metricsstore.Reader. | ||
| func (mqs MetricsQueryService) GetLatencies(ctx context.Context, params *metricsstore.LatenciesQueryParameters) (*metrics.MetricFamily, error) { | ||
| if mqs.metricsReader == nil { |
There was a problem hiding this comment.
now that you have a dedicated service, I wouldn't do nil check in it. I would instead provide a "disabled implementation" that always returns error from all methods, and then use that implementation from the RPC handler when the reader is not configured. This way you don't have nil checks anywhere, yet everything is nil-safe.
|
Thanks Yuri! I'll look into your suggestion for the next PR. |
Signed-off-by: albertteoh albert.teoh@logz.io
Which problem is this PR solving?
Short description of the changes
QueryServiceOptionsinstead of being a mandatory reader like the span or dependencies reader.