Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: use docker/go-metrics utilities for prometheus, add "no_metrics" build-tag #1659

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
41 changes: 41 additions & 0 deletions server/metrics.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
//go:build !no_metrics
// +build !no_metrics

package server

import (
"net/http"

"github.com/docker/go-metrics"
"github.com/gorilla/mux"
)

// namespacePrefix is the namespace prefix used for prometheus metrics.
const namespacePrefix = "notary_server"

// 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 {
if _, registered := instrumented[handlerName]; registered {
// handler for this operation is already registered.
return handler
}
Comment on lines +23 to +26
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to add this to prevent the handlers.Changefeed being registered twice; it's used for two endpoints, bot using Changefeed as handlerName / operationName. Double check if it still works as expected with this.

instrumented[handlerName] = struct{}{}

// Preserve the old situation, which used ConstLabels: "operation: <operation>"
// for metrics, but ConstLabels in go-metrics are per-namespace, and use
// ConstLabels: "handler: <handlerName>" (we pass operationName as handlerName).
namespace := metrics.NewNamespace(namespacePrefix, "http", metrics.Labels{"operation": handlerName})
httpMetrics := namespace.NewDefaultHttpMetrics(handlerName)
Comment on lines +29 to +33
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New situation will have two labels; "old" (operation: <handlerName>) and "new" (handler: <handlerName>)

metrics.Register(namespace)
return metrics.InstrumentHandler(httpMetrics, handler)
}

// handleMetricsEndpoint registers the /metrics endpoint.
func handleMetricsEndpoint(r *mux.Router) {
r.Methods("GET").Path("/metrics").Handler(metrics.Handler())
}
14 changes: 14 additions & 0 deletions server/metrics_disabled.go
Original file line number Diff line number Diff line change
@@ -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{}) {}
25 changes: 25 additions & 0 deletions server/metrics_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//go:build !no_metrics
// +build !no_metrics

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)
}
13 changes: 2 additions & 11 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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))

Expand Down
11 changes: 0 additions & 11 deletions server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down