diff --git a/internal/cmd/base/internal/metric/build_info.go b/internal/cmd/base/internal/metric/build_info.go index 552021f8c..b73dc3ddd 100644 --- a/internal/cmd/base/internal/metric/build_info.go +++ b/internal/cmd/base/internal/metric/build_info.go @@ -27,7 +27,7 @@ var buildInfoVec = prometheus.NewGaugeVec( []string{labelGoVersion, labelGitRevision, labelBoundaryVersion}, ) -func retrieveBuildInfoLabels() map[string]string { +func getBuildInfoLabels() map[string]string { verInfo := version.Get() return map[string]string{ @@ -45,6 +45,6 @@ func InitializeBuildInfo(r prometheus.Registerer) { } r.MustRegister(buildInfoVec) - l := prometheus.Labels(retrieveBuildInfoLabels()) + l := prometheus.Labels(getBuildInfoLabels()) buildInfoVec.With(l).Set(float64(1)) } diff --git a/internal/daemon/controller/internal/metric/api.go b/internal/daemon/controller/internal/metric/api.go index 987f5200f..8307c47b0 100644 --- a/internal/daemon/controller/internal/metric/api.go +++ b/internal/daemon/controller/internal/metric/api.go @@ -10,7 +10,7 @@ import ( "strings" "github.com/hashicorp/boundary/globals" - "github.com/hashicorp/boundary/internal/daemon/internal/metric" + "github.com/hashicorp/boundary/internal/daemon/metric" "github.com/hashicorp/boundary/internal/gen/controller/api/services" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" diff --git a/internal/daemon/controller/internal/metric/cluster.go b/internal/daemon/controller/internal/metric/cluster.go index 6449e2750..10790a70e 100644 --- a/internal/daemon/controller/internal/metric/cluster.go +++ b/internal/daemon/controller/internal/metric/cluster.go @@ -4,7 +4,7 @@ import ( "context" "github.com/hashicorp/boundary/globals" - metric "github.com/hashicorp/boundary/internal/daemon/internal/metric" + "github.com/hashicorp/boundary/internal/daemon/metric" "github.com/prometheus/client_golang/prometheus" "google.golang.org/grpc" "google.golang.org/grpc/codes" diff --git a/internal/daemon/internal/metric/initialize_metrics.go b/internal/daemon/metric/initialize_metrics.go similarity index 86% rename from internal/daemon/internal/metric/initialize_metrics.go rename to internal/daemon/metric/initialize_metrics.go index 0ed80d225..47668d2a1 100644 --- a/internal/daemon/internal/metric/initialize_metrics.go +++ b/internal/daemon/metric/initialize_metrics.go @@ -53,9 +53,21 @@ func rangeProtoFiles(m map[string][]string, fd protoreflect.FileDescriptor) bool return true } +// appendServicesAndMethods ranges through all registered files in a specified proto package +// and appends service and method names to the provided map m. +// This method is exported for testing purposes. +func appendServicesAndMethods(m map[string][]string, pkg protoreflect.FileDescriptor) { + protoregistry.GlobalFiles.RangeFilesByPackage( + pkg.Package(), + func(fd protoreflect.FileDescriptor) bool { return rangeProtoFiles(m, fd) }, + ) +} + // InitializeGrpcCollectorsFromPackage registers and zeroes a Prometheus -// histogram, populating all service and method labels by ranging through a -// given protobuf package. +// histogram, populating all service and method labels by ranging through +// the package containing the provided FileDescriptor. +// Note: inputting a protoreflect.FileDescriptor will populate all services and methods +// found in its package, not just methods associated with that specific FileDescriptor. func InitializeGrpcCollectorsFromPackage(r prometheus.Registerer, v prometheus.ObserverVec, pkg protoreflect.FileDescriptor, codes []codes.Code) { if r == nil { return @@ -63,10 +75,7 @@ func InitializeGrpcCollectorsFromPackage(r prometheus.Registerer, v prometheus.O r.MustRegister(v) serviceNamesToMethodNames := make(map[string][]string, 0) - protoregistry.GlobalFiles.RangeFilesByPackage( - pkg.Package(), - func(fd protoreflect.FileDescriptor) bool { return rangeProtoFiles(serviceNamesToMethodNames, fd) }, - ) + appendServicesAndMethods(serviceNamesToMethodNames, pkg) for serviceName, serviceMethods := range serviceNamesToMethodNames { for _, sm := range serviceMethods { diff --git a/internal/daemon/metric/initialize_metrics_test.go b/internal/daemon/metric/initialize_metrics_test.go new file mode 100644 index 000000000..a891f7fa3 --- /dev/null +++ b/internal/daemon/metric/initialize_metrics_test.go @@ -0,0 +1,32 @@ +package metric + +import ( + "testing" + + "github.com/hashicorp/boundary/internal/gen/testing/protooptions" + "github.com/stretchr/testify/assert" + "google.golang.org/protobuf/reflect/protoreflect" +) + +// As is, this test will currently fail if the services and methods in the protooptions +// package are changed. Please use temporary testcases to manually debug. +func Test_AppendServicesAndMethods(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + pkg protoreflect.FileDescriptor + expected map[string][]string + }{ + { + name: "basic", + pkg: protooptions.File_testing_options_v1_service_proto, + expected: map[string][]string{"testing.options.v1.TestService": {"TestMethod"}}, + }, + } + for _, tc := range cases { + m := make(map[string][]string, 0) + appendServicesAndMethods(m, tc.pkg) + assert.Equal(t, tc.expected, m) + } +} diff --git a/internal/daemon/internal/metric/instrument_handlers.go b/internal/daemon/metric/instrument_handlers.go similarity index 81% rename from internal/daemon/internal/metric/instrument_handlers.go rename to internal/daemon/metric/instrument_handlers.go index 37f5c8bca..ae47e9779 100644 --- a/internal/daemon/internal/metric/instrument_handlers.go +++ b/internal/daemon/metric/instrument_handlers.go @@ -5,6 +5,7 @@ package metric import ( "context" "strings" + "time" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/util" @@ -66,6 +67,34 @@ func (sh *statsHandler) HandleRPC(ctx context.Context, s stats.RPCStats) { } } +type requestRecorder struct { + reqLatency prometheus.ObserverVec + labels prometheus.Labels + + // measurements + start time.Time +} + +// NewGrpcRequestRecorder creates a requestRecorder struct which is used to measure gRPC client request latencies. +func NewGrpcRequestRecorder(fullMethodName string, reqLatency prometheus.ObserverVec) requestRecorder { + service, method := SplitMethodName(fullMethodName) + r := requestRecorder{ + reqLatency: reqLatency, + labels: prometheus.Labels{ + LabelGrpcMethod: method, + LabelGrpcService: service, + }, + start: time.Now(), + } + + return r +} + +func (r requestRecorder) Record(err error) { + r.labels[LabelGrpcCode] = StatusFromError(err).Code().String() + r.reqLatency.With(r.labels).Observe(time.Since(r.start).Seconds()) +} + // StatusFromError retrieves the *status.Status from the provided error. It'll // attempt to unwrap the *status.Error, which is something status.FromError // does not do. diff --git a/internal/daemon/internal/metric/instrument_handlers_test.go b/internal/daemon/metric/instrument_handlers_test.go similarity index 62% rename from internal/daemon/internal/metric/instrument_handlers_test.go rename to internal/daemon/metric/instrument_handlers_test.go index 327d5c145..b528dceb0 100644 --- a/internal/daemon/internal/metric/instrument_handlers_test.go +++ b/internal/daemon/metric/instrument_handlers_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/hashicorp/boundary/globals" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -14,6 +15,17 @@ import ( "google.golang.org/grpc/status" ) +var grpcRequestLatency prometheus.ObserverVec = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Namespace: globals.MetricNamespace, + Subsystem: "test_metric", + Name: "grpc_request_duration_seconds", + Help: "Test histogram.", + Buckets: prometheus.DefBuckets, + }, + ListGrpcLabels, +) + func TestNewStatsHandler(t *testing.T) { cases := []struct { name string @@ -140,3 +152,69 @@ func TestNewStatsHandler(t *testing.T) { }) } } + +func TestRecorder(t *testing.T) { + cases := []struct { + name string + methodName string + err error + wantedLabels prometheus.Labels + }{ + { + name: "basic", + methodName: "/some.service.path/method", + err: nil, + wantedLabels: map[string]string{ + LabelGrpcCode: "OK", + LabelGrpcMethod: "method", + LabelGrpcService: "some.service.path", + }, + }, + { + name: "unrecognized method path format", + methodName: "unrecognized", + err: nil, + wantedLabels: map[string]string{ + LabelGrpcCode: "OK", + LabelGrpcMethod: "unknown", + LabelGrpcService: "unknown", + }, + }, + { + name: "cancel error", + methodName: "/some.service.path/method", + err: status.Error(codes.Canceled, ""), + wantedLabels: map[string]string{ + LabelGrpcCode: "Canceled", + LabelGrpcMethod: "method", + LabelGrpcService: "some.service.path", + }, + }, + { + name: "permission error", + methodName: "/some.service.path/method", + err: status.Error(codes.PermissionDenied, ""), + wantedLabels: map[string]string{ + LabelGrpcCode: "PermissionDenied", + LabelGrpcMethod: "method", + LabelGrpcService: "some.service.path", + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ogReqLatency := grpcRequestLatency + defer func() { grpcRequestLatency = ogReqLatency }() + testableLatency := &TestableObserverVec{} + start := time.Now() + tested := NewGrpcRequestRecorder(tc.methodName, testableLatency) + tested.Record(tc.err) + + require.Len(t, testableLatency.Observations, 1) + assert.Greater(t, testableLatency.Observations[0].Observation, float64(0)) + assert.LessOrEqual(t, testableLatency.Observations[0].Observation, time.Since(start).Seconds()) + assert.Equal(t, testableLatency.Observations[0].Labels, tc.wantedLabels) + }) + } +} diff --git a/internal/daemon/internal/metric/testing.go b/internal/daemon/metric/testing.go similarity index 100% rename from internal/daemon/internal/metric/testing.go rename to internal/daemon/metric/testing.go diff --git a/internal/daemon/worker/internal/metric/cluster_client.go b/internal/daemon/worker/internal/metric/cluster_client.go index 19e720096..3413e6f18 100644 --- a/internal/daemon/worker/internal/metric/cluster_client.go +++ b/internal/daemon/worker/internal/metric/cluster_client.go @@ -2,10 +2,9 @@ package metric import ( "context" - "time" "github.com/hashicorp/boundary/globals" - metric "github.com/hashicorp/boundary/internal/daemon/internal/metric" + "github.com/hashicorp/boundary/internal/daemon/metric" "github.com/hashicorp/boundary/internal/gen/controller/servers/services" "github.com/prometheus/client_golang/prometheus" "google.golang.org/grpc" @@ -29,35 +28,6 @@ var grpcRequestLatency prometheus.ObserverVec = prometheus.NewHistogramVec( metric.ListGrpcLabels, ) -type requestRecorder struct { - reqLatency prometheus.ObserverVec - labels prometheus.Labels - - // measurements - start time.Time -} - -// NewRequestRecorder creates a requestRecorder struct which is used to measure gRPC client request latencies. -// For testing purposes, this method is exported. -func newRequestRecorder(fullMethodName string, reqLatency prometheus.ObserverVec) requestRecorder { - service, method := metric.SplitMethodName(fullMethodName) - r := requestRecorder{ - reqLatency: reqLatency, - labels: prometheus.Labels{ - metric.LabelGrpcMethod: method, - metric.LabelGrpcService: service, - }, - start: time.Now(), - } - - return r -} - -func (r requestRecorder) Record(err error) { - r.labels[metric.LabelGrpcCode] = metric.StatusFromError(err).Code().String() - r.reqLatency.With(r.labels).Observe(time.Since(r.start).Seconds()) -} - // The expected codes returned by the grpc client calls to cluster services. var expectedGrpcClientCodes = []codes.Code{ codes.OK, codes.Canceled, codes.Unknown, codes.InvalidArgument, codes.DeadlineExceeded, codes.NotFound, @@ -71,7 +41,7 @@ var expectedGrpcClientCodes = []codes.Code{ // between the cluster and its clients. func InstrumentClusterClient() grpc.UnaryClientInterceptor { return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { - r := newRequestRecorder(method, grpcRequestLatency) + r := metric.NewGrpcRequestRecorder(method, grpcRequestLatency) err := invoker(ctx, method, req, reply, cc, opts...) r.Record(err) return err diff --git a/internal/daemon/worker/internal/metric/cluster_client_test.go b/internal/daemon/worker/internal/metric/cluster_client_test.go index f3367f0ad..e6f35d44b 100644 --- a/internal/daemon/worker/internal/metric/cluster_client_test.go +++ b/internal/daemon/worker/internal/metric/cluster_client_test.go @@ -6,13 +6,11 @@ import ( "testing" "time" - metric "github.com/hashicorp/boundary/internal/daemon/internal/metric" + "github.com/hashicorp/boundary/internal/daemon/metric" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/wrapperspb" ) @@ -21,72 +19,6 @@ func TestInitializeClusterClientCollectors(t *testing.T) { require.NotPanics(t, func() { InitializeClusterClientCollectors(prometheus.NewRegistry()) }) } -func TestRecorder(t *testing.T) { - cases := []struct { - name string - methodName string - err error - wantedLabels prometheus.Labels - }{ - { - name: "basic", - methodName: "/some.service.path/method", - err: nil, - wantedLabels: map[string]string{ - metric.LabelGrpcCode: "OK", - metric.LabelGrpcMethod: "method", - metric.LabelGrpcService: "some.service.path", - }, - }, - { - name: "unrecognized method path format", - methodName: "unrecognized", - err: nil, - wantedLabels: map[string]string{ - metric.LabelGrpcCode: "OK", - metric.LabelGrpcMethod: "unknown", - metric.LabelGrpcService: "unknown", - }, - }, - { - name: "cancel error", - methodName: "/some.service.path/method", - err: status.Error(codes.Canceled, ""), - wantedLabels: map[string]string{ - metric.LabelGrpcCode: "Canceled", - metric.LabelGrpcMethod: "method", - metric.LabelGrpcService: "some.service.path", - }, - }, - { - name: "permission error", - methodName: "/some.service.path/method", - err: status.Error(codes.PermissionDenied, ""), - wantedLabels: map[string]string{ - metric.LabelGrpcCode: "PermissionDenied", - metric.LabelGrpcMethod: "method", - metric.LabelGrpcService: "some.service.path", - }, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ogReqLatency := grpcRequestLatency - defer func() { grpcRequestLatency = ogReqLatency }() - testableLatency := &metric.TestableObserverVec{} - start := time.Now() - tested := newRequestRecorder(tc.methodName, testableLatency) - tested.Record(tc.err) - - require.Len(t, testableLatency.Observations, 1) - assert.Greater(t, testableLatency.Observations[0].Observation, float64(0)) - assert.LessOrEqual(t, testableLatency.Observations[0].Observation, time.Since(start).Seconds()) - assert.Equal(t, testableLatency.Observations[0].Labels, tc.wantedLabels) - }) - } -} - func TestInstrumentClusterClient(t *testing.T) { ogReqLatency := grpcRequestLatency defer func() { grpcRequestLatency = ogReqLatency }() diff --git a/internal/daemon/worker/internal/metric/cluster_server.go b/internal/daemon/worker/internal/metric/cluster_server.go index 394354d53..904409c9c 100644 --- a/internal/daemon/worker/internal/metric/cluster_server.go +++ b/internal/daemon/worker/internal/metric/cluster_server.go @@ -4,7 +4,7 @@ import ( "context" "github.com/hashicorp/boundary/globals" - metric "github.com/hashicorp/boundary/internal/daemon/internal/metric" + "github.com/hashicorp/boundary/internal/daemon/metric" "github.com/prometheus/client_golang/prometheus" "google.golang.org/grpc" "google.golang.org/grpc/codes" diff --git a/internal/daemon/worker/internal/metric/proxy_http.go b/internal/daemon/worker/internal/metric/proxy_http.go index 1e35e1257..d02dbb671 100644 --- a/internal/daemon/worker/internal/metric/proxy_http.go +++ b/internal/daemon/worker/internal/metric/proxy_http.go @@ -8,7 +8,7 @@ import ( "path" "github.com/hashicorp/boundary/globals" - "github.com/hashicorp/boundary/internal/daemon/internal/metric" + "github.com/hashicorp/boundary/internal/daemon/metric" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" ) diff --git a/internal/daemon/worker/worker.go b/internal/daemon/worker/worker.go index e970edfef..9b000bcfa 100644 --- a/internal/daemon/worker/worker.go +++ b/internal/daemon/worker/worker.go @@ -32,6 +32,7 @@ import ( nodeefile "github.com/hashicorp/nodeenrollment/storage/file" "github.com/hashicorp/nodeenrollment/types" "github.com/mr-tron/base58" + "github.com/prometheus/client_golang/prometheus" ua "go.uber.org/atomic" "google.golang.org/grpc" "google.golang.org/grpc/resolver/manual" @@ -65,6 +66,10 @@ type downstreamers interface { // create its downstreamRouter var downstreamRouterFactory func() downstreamRouter +var initializeReverseGrpcClientCollectors = noopInitializePromCollectors + +func noopInitializePromCollectors(r prometheus.Registerer) {} + const ( authenticationStatusNeverAuthenticated uint32 = iota authenticationStatusFirstAuthentication @@ -135,6 +140,7 @@ func New(conf *Config) (*Worker, error) { metric.InitializeHttpCollectors(conf.PrometheusRegisterer) metric.InitializeWebsocketCollectors(conf.PrometheusRegisterer) metric.InitializeClusterClientCollectors(conf.PrometheusRegisterer) + initializeReverseGrpcClientCollectors(conf.PrometheusRegisterer) w := &Worker{ conf: conf,