From 5869b053904e065d86d9abb0616312863af8708c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 7 Nov 2022 13:30:18 +0100 Subject: [PATCH 1/3] server: move prometheus metrics to separate files Signed-off-by: Sebastiaan van Stijn --- server/metrics.go | 30 ++++++++++++++++++++++++++++++ server/metrics_test.go | 22 ++++++++++++++++++++++ server/server.go | 13 ++----------- 3 files changed, 54 insertions(+), 11 deletions(-) create mode 100644 server/metrics.go create mode 100644 server/metrics_test.go diff --git a/server/metrics.go b/server/metrics.go new file mode 100644 index 000000000..a307c715c --- /dev/null +++ b/server/metrics.go @@ -0,0 +1,30 @@ +package server + +import ( + "net/http" + + "github.com/docker/go-metrics" + "github.com/gorilla/mux" + "github.com/prometheus/client_golang/prometheus" +) + +// namespacePrefix is the namespace prefix used for prometheus metrics. +const namespacePrefix = "notary_server" + +func prometheusOpts(operation string) prometheus.SummaryOpts { + return prometheus.SummaryOpts{ + Namespace: namespacePrefix, + Subsystem: "http", + ConstLabels: prometheus.Labels{"operation": operation}, + } +} + +// instrumentedHandler instruments a server handler for monitoring with prometheus. +func instrumentedHandler(handlerName string, handler http.Handler) http.Handler { + return prometheus.InstrumentHandlerFuncWithOpts(prometheusOpts(handlerName), handler.ServeHTTP) //lint:ignore SA1019 TODO update prometheus API +} + +// handleMetricsEndpoint registers the /metrics endpoint. +func handleMetricsEndpoint(r *mux.Router) { + r.Methods("GET").Path("/metrics").Handler(metrics.Handler()) +} diff --git a/server/metrics_test.go b/server/metrics_test.go new file mode 100644 index 000000000..f40e607b6 --- /dev/null +++ b/server/metrics_test.go @@ -0,0 +1,22 @@ +package server + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + "github.com/theupdateframework/notary/tuf/signed" +) + +func TestMetricsEndpoint(t *testing.T) { + handler := RootHandler(context.Background(), nil, signed.NewEd25519(), + nil, nil, nil) + ts := httptest.NewServer(handler) + defer ts.Close() + + res, err := http.Get(ts.URL + "/metrics") + require.NoError(t, err) + require.Equal(t, http.StatusOK, res.StatusCode) +} diff --git a/server/server.go b/server/server.go index c57e452d1..ac0c6fa0b 100644 --- a/server/server.go +++ b/server/server.go @@ -11,7 +11,6 @@ import ( "github.com/docker/distribution/registry/api/errcode" "github.com/docker/distribution/registry/auth" "github.com/gorilla/mux" - "github.com/prometheus/client_golang/prometheus" "github.com/sirupsen/logrus" "github.com/theupdateframework/notary/server/errors" "github.com/theupdateframework/notary/server/handlers" @@ -25,14 +24,6 @@ func init() { data.SetDefaultExpiryTimes(data.NotaryDefaultExpiries) } -func prometheusOpts(operation string) prometheus.SummaryOpts { - return prometheus.SummaryOpts{ - Namespace: "notary_server", - Subsystem: "http", - ConstLabels: prometheus.Labels{"operation": operation}, - } -} - // Config tells Run how to configure a server type Config struct { Addr string @@ -124,7 +115,7 @@ func CreateHandler(operationName string, serverHandler utils.ContextHandler, err wrapped = utils.WrapWithCacheHandler(cacheControlConfig, wrapped) } wrapped = filterImagePrefixes(repoPrefixes, errorIfGUNInvalid, wrapped) - return prometheus.InstrumentHandlerWithOpts(prometheusOpts(operationName), wrapped) //lint:ignore SA1019 TODO update prometheus API + return instrumentedHandler(operationName, wrapped) } // RootHandler returns the handler that routes all the paths from / for the @@ -232,7 +223,7 @@ func RootHandler(ctx context.Context, ac auth.AccessController, trust signed.Cry repoPrefixes, )) r.Methods("GET").Path("/_notary_server/health").HandlerFunc(health.StatusHandler) - r.Methods("GET").Path("/metrics").Handler(prometheus.Handler()) //lint:ignore SA1019 TODO update prometheus API + handleMetricsEndpoint(r) r.Methods("GET", "POST", "PUT", "HEAD", "DELETE").Path("/{other:.*}").Handler( authWrapper(handlers.NotFoundHandler)) From ca2d64177041ecd31eb533879240a8e97c8c801e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 7 Nov 2022 13:33:46 +0100 Subject: [PATCH 2/3] server: add no_metrics build-tag to disable prometheus Signed-off-by: Sebastiaan van Stijn --- Makefile | 5 +++++ server/metrics.go | 3 +++ server/metrics_disabled.go | 14 ++++++++++++++ server/metrics_test.go | 3 +++ server/server_test.go | 11 ----------- 5 files changed, 25 insertions(+), 11 deletions(-) create mode 100644 server/metrics_disabled.go diff --git a/Makefile b/Makefile index 170ca54bb..bd7d78612 100644 --- a/Makefile +++ b/Makefile @@ -16,6 +16,11 @@ CTIMEVAR=-X $(NOTARY_PKG)/version.GitCommit=$(GITCOMMIT) -X $(NOTARY_PKG)/versio GO_LDFLAGS=-ldflags "-w $(CTIMEVAR)" GO_LDFLAGS_STATIC=-ldflags "-w $(CTIMEVAR) -extldflags -static" GOOSES = darwin linux windows + +# Available build-tags: +# +# - pkcs11 enables pkcs11 integration +# - no_metrics removes prometheus metrics and the /metrics endpoint NOTARY_BUILDTAGS ?= pkcs11 NOTARYDIR := /go/src/github.com/theupdateframework/notary diff --git a/server/metrics.go b/server/metrics.go index a307c715c..6ffc0cd7c 100644 --- a/server/metrics.go +++ b/server/metrics.go @@ -1,3 +1,6 @@ +//go:build !no_metrics +// +build !no_metrics + package server import ( diff --git a/server/metrics_disabled.go b/server/metrics_disabled.go new file mode 100644 index 000000000..d165d7074 --- /dev/null +++ b/server/metrics_disabled.go @@ -0,0 +1,14 @@ +//go:build no_metrics +// +build no_metrics + +package server + +import "net/http" + +// instrumentedHandler instruments a server handler for monitoring with prometheus. +func instrumentedHandler(_ string, handler http.Handler) http.Handler { + return handler +} + +// handleMetricsEndpoint registers the /metrics endpoint. +func handleMetricsEndpoint(r *interface{}) {} diff --git a/server/metrics_test.go b/server/metrics_test.go index f40e607b6..8fa045b8e 100644 --- a/server/metrics_test.go +++ b/server/metrics_test.go @@ -1,3 +1,6 @@ +//go:build !no_metrics +// +build !no_metrics + package server import ( diff --git a/server/server_test.go b/server/server_test.go index 336bbc926..d1b93acfa 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -140,17 +140,6 @@ func TestRepoPrefixDoesNotMatch(t *testing.T) { require.Equal(t, http.StatusNotFound, res.StatusCode) } -func TestMetricsEndpoint(t *testing.T) { - handler := RootHandler(context.Background(), nil, signed.NewEd25519(), - nil, nil, nil) - ts := httptest.NewServer(handler) - defer ts.Close() - - res, err := http.Get(ts.URL + "/metrics") - require.NoError(t, err) - require.Equal(t, http.StatusOK, res.StatusCode) -} - // GetKeys supports only the timestamp and snapshot key endpoints func TestGetKeysEndpoint(t *testing.T) { ctx := context.WithValue( From 07a34bab812cd7f5fb33f6a1578b56b36d340321 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 7 Nov 2022 13:37:29 +0100 Subject: [PATCH 3/3] server: use docker/go-metrics utilities for prometheus The old code was no longer compatible with current versions of prometheus. This switches the code to use docker/go-metrics, which is compatible with current versions of prometheus, and already in use in other code in the dependency tree. I tried to keep the metrics the same as before, but there may be some differences. Signed-off-by: Sebastiaan van Stijn --- go.mod | 4 ++-- server/metrics.go | 26 +++++++++++++++++--------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/go.mod b/go.mod index b5eed2686..9575046c2 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/docker/distribution v2.7.1+incompatible github.com/docker/go v1.5.1-1.0.20160303222718-d30aec9fd63c github.com/docker/go-connections v0.4.0 + github.com/docker/go-metrics v0.0.0-20180209012529-399ea8c73916 github.com/dvsekhvalnov/jose2go v0.0.0-20200901110807-248326c1351b github.com/go-sql-driver/mysql v1.5.0 github.com/golang/protobuf v1.5.2 @@ -17,7 +18,6 @@ require ( github.com/lib/pq v1.9.0 github.com/mattn/go-sqlite3 v1.14.0 github.com/miekg/pkcs11 v1.0.3 - github.com/prometheus/client_golang v0.9.0-pre1.0.20180209125602-c332b6f63c06 github.com/sirupsen/logrus v1.8.1 github.com/spf13/cobra v1.6.0 github.com/spf13/viper v0.0.0-20150530192845-be5ff3e4840c @@ -39,7 +39,6 @@ require ( github.com/bugsnag/panicwrap v0.0.0-20151223152923-e2c28503fcd0 // indirect github.com/davecgh/go-spew v1.1.1 // indirect github.com/denisenkom/go-mssqldb v0.0.0-20191128021309-1d7a30a10f73 // indirect - github.com/docker/go-metrics v0.0.0-20180209012529-399ea8c73916 // indirect github.com/docker/libtrust v0.0.0-20160708172513-aabc10ec26b7 // indirect github.com/gogo/protobuf v1.0.0 // indirect github.com/google/certificate-transparency-go v1.0.10-0.20180222191210-5ab67e519c93 // indirect @@ -58,6 +57,7 @@ require ( github.com/opentracing/opentracing-go v1.1.0 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/prometheus/client_golang v0.9.0-pre1.0.20180209125602-c332b6f63c06 // indirect github.com/prometheus/client_model v0.0.0-20190812154241-14fe0d1b01d4 // indirect github.com/prometheus/common v0.0.0-20180110214958-89604d197083 // indirect github.com/prometheus/procfs v0.0.0-20180125133057-cb4147076ac7 // indirect diff --git a/server/metrics.go b/server/metrics.go index 6ffc0cd7c..20253c91a 100644 --- a/server/metrics.go +++ b/server/metrics.go @@ -8,23 +8,31 @@ import ( "github.com/docker/go-metrics" "github.com/gorilla/mux" - "github.com/prometheus/client_golang/prometheus" ) // namespacePrefix is the namespace prefix used for prometheus metrics. const namespacePrefix = "notary_server" -func prometheusOpts(operation string) prometheus.SummaryOpts { - return prometheus.SummaryOpts{ - Namespace: namespacePrefix, - Subsystem: "http", - ConstLabels: prometheus.Labels{"operation": operation}, - } -} +// Server uses handlers.Changefeed for two separate routes. It's not allowed +// to register twice ("duplicate metrics collector registration attempted"), +// so checking if it's already instrumented, otherwise skip. +var instrumented = map[string]struct{}{} // instrumentedHandler instruments a server handler for monitoring with prometheus. func instrumentedHandler(handlerName string, handler http.Handler) http.Handler { - return prometheus.InstrumentHandlerFuncWithOpts(prometheusOpts(handlerName), handler.ServeHTTP) //lint:ignore SA1019 TODO update prometheus API + if _, registered := instrumented[handlerName]; registered { + // handler for this operation is already registered. + return handler + } + instrumented[handlerName] = struct{}{} + + // Preserve the old situation, which used ConstLabels: "operation: " + // for metrics, but ConstLabels in go-metrics are per-namespace, and use + // ConstLabels: "handler: " (we pass operationName as handlerName). + namespace := metrics.NewNamespace(namespacePrefix, "http", metrics.Labels{"operation": handlerName}) + httpMetrics := namespace.NewDefaultHttpMetrics(handlerName) + metrics.Register(namespace) + return metrics.InstrumentHandler(httpMetrics, handler) } // handleMetricsEndpoint registers the /metrics endpoint.