Skip to content

Commit

Permalink
Delete gRPC MetricsQueryService, metricsquery.proto and related code (#…
Browse files Browse the repository at this point in the history
…6616)

## Which problem is this PR solving?
- No code in Jaeger was dependent on having a gRPC endpoint for reading
metrics. UI's Monitor tab uses JSON endpoint.
- This code is just a pass-through proxy to the underlying metrics
storage with which we communicate using OpenMetrics API.

## Description of the changes
- Delete unnecessary code and proto-gen types.
- 🛑 This is a breaking change: the gRPC `MetricsQueryService` endpoint
is removed.

## How was this change tested?
- CI

---------

Signed-off-by: Yuri Shkuro <github@ysh.us>
  • Loading branch information
yurishkuro authored Jan 26, 2025
1 parent 5caf4e4 commit 7f72d96
Show file tree
Hide file tree
Showing 11 changed files with 38 additions and 3,393 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/ci-lint-checks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ jobs:
go-version: 1.23.x

- name: Verify Protobuf types are up to date
run: make proto && git diff --name-status --exit-code
run: make proto && { if git status --porcelain | grep '??'; then exit 1; else git diff --name-status --exit-code; fi }

- name: Verify Thrift types are up to date
run: make thrift && git diff --name-status --exit-code
run: make thrift && { if git status --porcelain | grep '??'; then exit 1; else git diff --name-status --exit-code; fi }

- name: Verify Mockery types are up to date
run: make generate-mocks && { if git status --porcelain | grep '??'; then exit 1; else git diff --name-status --exit-code; fi }
Expand Down
2 changes: 0 additions & 2 deletions Makefile.Protobuf.mk
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,6 @@ patch-api-v2:
proto-openmetrics:
$(call print_caption, Processing OpenMetrics Protos)
$(foreach file,$(OPENMETRICS_PROTO_FILES),$(call proto_compile, proto-gen/api_v2/metrics, $(file)))
@# TODO why is this file included in model/proto/metrics/ in the first place?
rm proto-gen/api_v2/metrics/otelmetric.pb.go

.PHONY: proto-storage-v1
proto-storage-v1:
Expand Down
5 changes: 1 addition & 4 deletions cmd/anonymizer/app/query/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"github.com/jaegertracing/jaeger-idl/proto-gen/api_v2"
"github.com/jaegertracing/jaeger/cmd/query/app"
"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
"github.com/jaegertracing/jaeger/plugin/metricstore/disabled"
"github.com/jaegertracing/jaeger/storage/spanstore"
spanstoremocks "github.com/jaegertracing/jaeger/storage/spanstore/mocks"
dependencyStoreMocks "github.com/jaegertracing/jaeger/storage_v2/depstore/mocks"
Expand Down Expand Up @@ -58,15 +57,13 @@ type testServer struct {
func newTestServer(t *testing.T) *testServer {
spanReader := &spanstoremocks.Reader{}
traceReader := v1adapter.NewTraceReader(spanReader)
metricsReader, err := disabled.NewMetricsReader()
require.NoError(t, err)

q := querysvc.NewQueryService(
traceReader,
&dependencyStoreMocks.Reader{},
querysvc.QueryServiceOptions{},
)
h := app.NewGRPCHandler(q, metricsReader, app.GRPCHandlerOptions{})
h := app.NewGRPCHandler(q, app.GRPCHandlerOptions{})

server := grpc.NewServer()
api_v2.RegisterQueryServiceServer(server, h)
Expand Down
1 change: 0 additions & 1 deletion cmd/jaeger/internal/extension/jaegerquery/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,6 @@ func TestServerStart(t *testing.T) {
ExpectedServices: []string{
"jaeger.api_v2.QueryService",
"jaeger.api_v3.QueryService",
"jaeger.api_v2.metrics.MetricsQueryService",
"grpc.health.v1.Health",
},
}.Execute(t)
Expand Down
161 changes: 10 additions & 151 deletions cmd/query/app/grpc_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ import (
"github.com/jaegertracing/jaeger/cmd/query/app/querysvc"
_ "github.com/jaegertracing/jaeger/pkg/gogocodec" // force gogo codec registration
"github.com/jaegertracing/jaeger/pkg/jtracer"
"github.com/jaegertracing/jaeger/plugin/metricstore/disabled"
"github.com/jaegertracing/jaeger/proto-gen/api_v2/metrics"
"github.com/jaegertracing/jaeger/storage/metricstore"
"github.com/jaegertracing/jaeger/storage/spanstore"
)

Expand All @@ -30,20 +27,16 @@ const (
)

var (
errGRPCMetricsQueryDisabled = status.Error(codes.Unimplemented, "metrics querying is currently disabled")
errNilRequest = status.Error(codes.InvalidArgument, "a nil argument is not allowed")
errUninitializedTraceID = status.Error(codes.InvalidArgument, "uninitialized TraceID is not allowed")
errMissingServiceNames = status.Error(codes.InvalidArgument, "please provide at least one service name")
errMissingQuantile = status.Error(codes.InvalidArgument, "please provide a quantile between (0, 1]")
errNilRequest = status.Error(codes.InvalidArgument, "a nil argument is not allowed")
errUninitializedTraceID = status.Error(codes.InvalidArgument, "uninitialized TraceID is not allowed")
)

// GRPCHandler implements the gRPC endpoint of the query service.
type GRPCHandler struct {
queryService *querysvc.QueryService
metricsQueryService querysvc.MetricsQueryService
logger *zap.Logger
tracer *jtracer.JTracer
nowFn func() time.Time
queryService *querysvc.QueryService
logger *zap.Logger
tracer *jtracer.JTracer
nowFn func() time.Time
}

// GRPCHandlerOptions contains optional members of GRPCHandler.
Expand All @@ -55,7 +48,6 @@ type GRPCHandlerOptions struct {

// NewGRPCHandler returns a GRPCHandler.
func NewGRPCHandler(queryService *querysvc.QueryService,
metricsQueryService querysvc.MetricsQueryService,
options GRPCHandlerOptions,
) *GRPCHandler {
if options.Logger == nil {
Expand All @@ -71,11 +63,10 @@ func NewGRPCHandler(queryService *querysvc.QueryService,
}

return &GRPCHandler{
queryService: queryService,
metricsQueryService: metricsQueryService,
logger: options.Logger,
tracer: options.Tracer,
nowFn: options.NowFn,
queryService: queryService,
logger: options.Logger,
tracer: options.Tracer,
nowFn: options.NowFn,
}
}

Expand Down Expand Up @@ -248,135 +239,3 @@ 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) {
bqp, err := g.newBaseQueryParameters(r)
if err := g.handleErr("failed to build parameters", err); err != nil {
return nil, err
}
// Check for cases where clients do not provide the Quantile, which defaults to the float64's zero value.
if r.Quantile == 0 {
return nil, errMissingQuantile
}
queryParams := metricstore.LatenciesQueryParameters{
BaseQueryParameters: bqp,
Quantile: r.Quantile,
}
m, err := g.metricsQueryService.GetLatencies(ctx, &queryParams)
if err := g.handleErr("failed to fetch latencies", err); err != nil {
return nil, err
}
return &metrics.GetMetricsResponse{Metrics: *m}, nil
}

// GetCallRates is the gRPC handler to fetch call rate metrics.
func (g *GRPCHandler) GetCallRates(ctx context.Context, r *metrics.GetCallRatesRequest) (*metrics.GetMetricsResponse, error) {
bqp, err := g.newBaseQueryParameters(r)
if err := g.handleErr("failed to build parameters", err); err != nil {
return nil, err
}
queryParams := metricstore.CallRateQueryParameters{
BaseQueryParameters: bqp,
}
m, err := g.metricsQueryService.GetCallRates(ctx, &queryParams)
if err := g.handleErr("failed to fetch call rates", err); err != nil {
return nil, err
}
return &metrics.GetMetricsResponse{Metrics: *m}, nil
}

// GetErrorRates is the gRPC handler to fetch error rate metrics.
func (g *GRPCHandler) GetErrorRates(ctx context.Context, r *metrics.GetErrorRatesRequest) (*metrics.GetMetricsResponse, error) {
bqp, err := g.newBaseQueryParameters(r)
if err := g.handleErr("failed to build parameters", err); err != nil {
return nil, err
}
queryParams := metricstore.ErrorRateQueryParameters{
BaseQueryParameters: bqp,
}
m, err := g.metricsQueryService.GetErrorRates(ctx, &queryParams)
if err := g.handleErr("failed to fetch error rates", err); err != nil {
return nil, err
}
return &metrics.GetMetricsResponse{Metrics: *m}, nil
}

// GetMinStepDuration is the gRPC handler to fetch the minimum step duration supported by the underlying metrics store.
func (g *GRPCHandler) GetMinStepDuration(ctx context.Context, _ *metrics.GetMinStepDurationRequest) (*metrics.GetMinStepDurationResponse, error) {
minStep, err := g.metricsQueryService.GetMinStepDuration(ctx, &metricstore.MinStepDurationQueryParameters{})
if err := g.handleErr("failed to fetch min step duration", err); err != nil {
return nil, err
}
return &metrics.GetMinStepDurationResponse{MinStep: minStep}, nil
}

func (g *GRPCHandler) handleErr(msg string, err error) error {
if err == nil {
return nil
}
g.logger.Error(msg, zap.Error(err))

// Avoid wrapping "expected" errors with an "Internal Server" error.
if errors.Is(err, disabled.ErrDisabled) {
return errGRPCMetricsQueryDisabled
}
if _, ok := status.FromError(err); ok {
return err
}

// Received an "unexpected" error.
return status.Errorf(codes.Internal, "%s: %v", msg, err)
}

func (g *GRPCHandler) newBaseQueryParameters(r any) (bqp metricstore.BaseQueryParameters, err error) {
if r == nil {
return bqp, errNilRequest
}
var baseRequest *metrics.MetricsQueryBaseRequest
switch v := r.(type) {
case *metrics.GetLatenciesRequest:
baseRequest = v.BaseRequest
case *metrics.GetCallRatesRequest:
baseRequest = v.BaseRequest
case *metrics.GetErrorRatesRequest:
baseRequest = v.BaseRequest
}
if baseRequest == nil || len(baseRequest.ServiceNames) == 0 {
return bqp, errMissingServiceNames
}

// Copy non-nullable params.
bqp.GroupByOperation = baseRequest.GroupByOperation
bqp.ServiceNames = baseRequest.ServiceNames

// Initialize nullable params with defaults.
defaultEndTime := g.nowFn()
bqp.EndTime = &defaultEndTime
bqp.Lookback = &defaultMetricsQueryLookbackDuration
bqp.RatePer = &defaultMetricsQueryRateDuration
bqp.SpanKinds = defaultMetricsSpanKinds
bqp.Step = &defaultMetricsQueryStepDuration

// ... and override defaults with any provided request params.
if baseRequest.EndTime != nil {
bqp.EndTime = baseRequest.EndTime
}
if baseRequest.Lookback != nil {
bqp.Lookback = baseRequest.Lookback
}
if baseRequest.Step != nil {
bqp.Step = baseRequest.Step
}
if baseRequest.RatePer != nil {
bqp.RatePer = baseRequest.RatePer
}
if len(baseRequest.SpanKinds) > 0 {
spanKinds := make([]string, len(baseRequest.SpanKinds))
for i, v := range baseRequest.SpanKinds {
spanKinds[i] = v.String()
}
bqp.SpanKinds = spanKinds
}
return bqp, nil
}
Loading

0 comments on commit 7f72d96

Please sign in to comment.