Hook up MetricsQueryService to main funcs #3079

albertteoh · 2021-06-10T02:40:25Z

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

Instantiates the MetricsQueryService from both the all-in-one and query executables to be passed on to the HTTP and gRPC handlers.
MetricsQueryService exposes an Enabled() endpoint to determine if the MetricsQuery feature is enabled (METRICS_STORAGE_TYPE env var is set), allowing HTTP and gRPC handlers to respond with a sensible error message/code.

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

albertteoh · 2021-06-10T02:41:16Z

cmd/query/app/server.go

@@ -111,11 +111,15 @@ func createGRPCServer(querySvc *querysvc.QueryService, options *QueryOptions, lo
 	server := grpc.NewServer(grpcOpts...)

 	handler := NewGRPCHandler(querySvc, logger, tracer)
+
+	// TODO: Register MetricsQueryService


Will be addressed in a follow-up PR implementing MetricsQuery support for the gRPC handler.

albertteoh · 2021-06-10T02:41:27Z

cmd/query/app/server.go

 	return server, nil
 }

-func createHTTPServer(querySvc *querysvc.QueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) (*http.Server, error) {
+func createHTTPServer(querySvc *querysvc.QueryService, metricsQuerySvc *querysvc.MetricsQueryService, queryOpts *QueryOptions, tracer opentracing.Tracer, logger *zap.Logger) (*http.Server, error) {
+	// TODO: Add HandlerOptions.MetricsQueryService


Will be addressed in a follow-up PR implementing MetricsQuery support for the HTTP handler.

albertteoh · 2021-06-10T02:42:01Z

cmd/query/app/querysvc/metrics_query_service.go

 func NewMetricsQueryService(reader metricsstore.Reader) *MetricsQueryService {
 	return &MetricsQueryService{
 		metricsReader: reader,
 	}
 }

+func (mqs MetricsQueryService) Enabled() bool {


Hopefully I understood your suggestion correctly, @yurishkuro.

no, what I meant was adding this:

type nullMetricsQueryService struct {} func (nullMetricsQueryService) GetLatencies(ctx context.Context, params *metricsstore.LatenciesQueryParameters) (*metrics.MetricFamily, error) { return nil, errNotEnabled } ...

and using it in RPC handlers. Basically, solve the problem with polymorphism, not with a bunch of nil checks all over the place.

that's pretty cool :) although, unless if I'm mistaken, I see two minor issues:

The handlers will need to parse inputs, etc. then call GetLatencies, before it knows that Metrics Querying is disabled. I think it would be better to return early; but not sure it's possible with this approach.

Once calling GetLatencies, the handler needs to differentiate the errNotEnabled from an "exceptional" error case so it can return an appropriate error code rather than the generic StatusInternalServerError code, which is important as the UI needs to handle both cases differently.
We could follow a pattern like this but there's no guarantee the error could be wrapped up, so I imagine we'd need use something like errors.Is.

what do you think?

point (1) is probably not particularly important, what's the downside of parsing inputs?

point (2) is valid if you want to differentiate real failure from not-implemented (grpc has UNIMPLEMENTED status code iirc). But the handler should look like this:

handler() { res, err := service.call() if err != nil { return nil, errWithStatusCode(err) } }

There will be only one place in the code, errWithStatusCode(), that needs to understand errNotEnabled and transform it. Easy to test, rather than each handler branching to make checks.

and yes, you'd want to use error.Is, it's the new Go way, what we have with if err == spanstore.ErrTraceNotFound { predates that.

what's the downside of parsing inputs?

not a large downside, and the emphasis wasn't so much on the input parsing but on when the handler knows if the feature is enabled or disabled. I feel the intent is clearer when reading the handler code instead of needing to follow it to the point where the error is handled:

metricsHandler() { if notEnabled { return notImplementedErrorCode } res, err := service.call() ... }

In any case, it's a small matter I think, and happy to go ahead with your suggestion.

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

codecov · 2021-06-10T02:55:27Z

Codecov Report

Merging #3079 (a8a420f) into master (e33977e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3079   +/-   ##
=======================================
  Coverage   95.83%   95.84%           
=======================================
  Files         234      236    +2     
  Lines       10068    10073    +5     
=======================================
+ Hits         9649     9654    +5     
+ Misses        349      348    -1     
- Partials       70       71    +1

Impacted Files	Coverage Δ
cmd/query/app/querysvc/metrics_query_service.go	`100.00% <ø> (ø)`
cmd/query/app/server.go	`97.08% <100.00%> (ø)`
plugin/metrics/disabled/factory.go	`100.00% <100.00%> (ø)`
plugin/metrics/disabled/reader.go	`100.00% <100.00%> (ø)`
plugin/metrics/factory.go	`100.00% <100.00%> (ø)`
cmd/query/app/static_handler.go	`94.35% <0.00%> (-2.42%)`	⬇️
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️
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 e33977e...a8a420f. Read the comment docs.

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

cmd/query/app/querysvc/metrics_query_service.go

yurishkuro · 2021-06-10T23:34:49Z

cmd/all-in-one/main.go

+}
+
+func createMetricsReaderFactory(fc metricsPlugin.FactoryConfig) (*metricsPlugin.Factory, error) {
+	if !metricsQueryEnabled(fc) {


are these checks still necessary? wouldn't you always get something back (perhaps disabled service)?

Yeah, it's still necessary otherwise: Cannot initialize metrics store factory: unknown metrics type "". Valid types are [prometheus]

Do you think it's worth doing something similar with the factories? i.e. have disabledMetricsReaderFactory that simply returns a nil Reader? That way, these checks won't be necessary.

I think you can extend the same approach - consider default metrics store type to be "none" and return a "disabled reader" in that case. Basically push this all the way down (you won't need "disabled service" in this case).

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

albertteoh · 2021-06-11T05:57:10Z

plugin/metrics/factory.go

+	case DisabledStorageType:
+		return disabled.NewFactory(), nil


Here's the point where the "default" storage factory is created that returns the disabled metrics reader.

yurishkuro · 2021-06-11T23:29:28Z

cmd/query/app/querysvc/metrics_query_service.go

 	"time"

 	"github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics"
 	"github.com/jaegertracing/jaeger/storage/metricsstore"
 )

-// MetricsQueryService contains the underlying reader required for querying the metrics store.
+// MetricsQueryService provides a means of querying R.E.D metrics from an underlying metrics store.
 type MetricsQueryService struct {


What is MetricsQueryService supposed to add on top of metricsstore.Reader?

Note that we used to have HTTP handler use SpanReader directly, but then refactored it into QueryService because we needed to share it with GRPC Handler and the query service was an abstraction on top of two span readers: primary and archive. In your case there doesn't seem to be any additional abstraction needed, you just proxy all calls into the reader directly. Not a huge issue to leave as is, but just curious if there's any thinking that a layer of abstraction would be needed in the future.

Indeed MetricsQueryService adds little on top of metricsstore.Reader. It has exactly the same function signature as the Reader and yes, it simply forwards the call. I also don't see a need for multiple metrics readers at runtime, for now at least.

It was added to follow the same design as span/dependency querying to avoid confusing/surprising those following the code and seeing a QueryService being used by handlers to query spans/dependencies, then a Reader used to query metrics; when they may expect the handlers to refer to "QueryService" abstractions for querying spans, dependencies and metrics.

One possibility to avoid confusion, yet minimizing unnecessary code, is to make MetricsQueryService an interface, embedding metricsstore.Reader, meaning we can remove the MetricsQueryService struct and its proxy functions altogether and simply give the handlers a metricsstore.Reader instance. That way, handlers still refer to the "MetricsQueryService" alias for metrics, but instead using a Reader implementation. Something like:

type MetricsQueryService interface { metricsstore.Reader }

yurishkuro · 2021-06-11T23:34:34Z

plugin/metrics/factory.go

 	"github.com/jaegertracing/jaeger/plugin/metrics/prometheus"
 	"github.com/jaegertracing/jaeger/storage"
 	"github.com/jaegertracing/jaeger/storage/metricsstore"
 )

 const (
+	// DisabledStorageType is the storage type used when METRICS_STORAGE_TYPE is unset.
+	DisabledStorageType = ""


doesn't seem to be used outside of this pkg.

yurishkuro · 2021-06-11T23:36:42Z

plugin/metrics/disabled/metricsstore/reader_test.go

@@ -0,0 +1,74 @@
+// Copyright (c) 2021 The Jaeger Authors.


plugin/metrics/disabled/metricsstore/

a bit of overkill with nesting of packages, I would've kept it next to Factory under metrics/disabled

yurishkuro

small nits, lgtm otherwise

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

Hook up MetricsQueryService to main funcs

29cc2da

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

albertteoh commented Jun 10, 2021

View reviewed changes

Fix lint error

b68914e

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

Fix nil ref

3756c35

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

albertteoh mentioned this pull request Jun 10, 2021

"Monitor" tab for service health metrics #2954

Closed

14 tasks

albertteoh marked this pull request as ready for review June 10, 2021 03:19

albertteoh requested a review from a team as a code owner June 10, 2021 03:19

albertteoh requested a review from vprithvi June 10, 2021 03:19

albertteoh added 3 commits June 10, 2021 21:43

Merge branch 'master' into 2954-hookup-mains

188c1a1

Address review comments

f562a1f

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

Code coverage

99ea523

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

yurishkuro reviewed Jun 10, 2021

View reviewed changes

albertteoh added 3 commits June 11, 2021 10:58

Address review comments

53a486b

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

Add disabled reader

c81a23c

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

Remove redundancy

2eaa9c3

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

albertteoh commented Jun 11, 2021

View reviewed changes

yurishkuro reviewed Jun 11, 2021

View reviewed changes

yurishkuro previously approved these changes Jun 11, 2021

View reviewed changes

albertteoh added 2 commits June 12, 2021 22:51

Merge branch 'master' into 2954-hookup-mains

64fa935

Address review comments

a8a420f

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

albertteoh dismissed yurishkuro’s stale review via a8a420f June 12, 2021 12:52

yurishkuro approved these changes Jun 12, 2021

View reviewed changes

yurishkuro merged commit 013efad into jaegertracing:master Jun 12, 2021

albertteoh deleted the 2954-hookup-mains branch June 13, 2021 05:04

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hook up MetricsQueryService to main funcs #3079

Hook up MetricsQueryService to main funcs #3079

albertteoh commented Jun 10, 2021

albertteoh Jun 10, 2021

albertteoh Jun 10, 2021

albertteoh Jun 10, 2021 •

edited

Loading

yurishkuro Jun 10, 2021

albertteoh Jun 10, 2021

yurishkuro Jun 10, 2021

yurishkuro Jun 10, 2021

albertteoh Jun 10, 2021

codecov bot commented Jun 10, 2021 •

edited

Loading

yurishkuro Jun 10, 2021

albertteoh Jun 11, 2021

yurishkuro Jun 11, 2021

albertteoh Jun 11, 2021

yurishkuro Jun 11, 2021

albertteoh Jun 12, 2021 •

edited

Loading

yurishkuro Jun 11, 2021

yurishkuro Jun 11, 2021

yurishkuro left a comment

Hook up MetricsQueryService to main funcs #3079

Hook up MetricsQueryService to main funcs #3079

Conversation

albertteoh commented Jun 10, 2021

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh Jun 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 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

albertteoh Jun 12, 2021 • edited Loading

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

albertteoh Jun 10, 2021 •

edited

Loading

codecov bot commented Jun 10, 2021 •

edited

Loading

albertteoh Jun 12, 2021 •

edited

Loading