Repository: buchgr/bazel-remote
Pull request #264 (2020-05-06 07:19:09)

Add flag for enabling metrics and use bigger duration buckets
The default histogram buckets in prometheus is from 0 to 10 seconds, so anything that takes >10 secs cannot be tracked. This commit uses duration buckets to include bigger values.

elianuber: > lgtm- thanks. Could you please squash this to a single commit?

done !

Patch:
diff --git a/README.md b/README.md
index 2269d3a..9c5a7c9 100644
--- a/README.md
+++ b/README.md
@@ -118,6 +118,7 @@ GLOBAL OPTIONS:
    --s3.region value             The AWS region. Required when using s3.iam_role_endpoint. [$BAZEL_REMOTE_S3_REGION]
    --disable_http_ac_validation  Whether to disable ActionResult validation for HTTP requests. (default: false, ie enable validation) [$BAZEL_REMOTE_DISABLE_HTTP_AC_VALIDATION]
    --disable_grpc_ac_deps_check  Whether to disable ActionResult dependency check for gRPC GetActionResult requests. (default: false, ie enable ActionCache dependency checks) [$BAZEL_REMOTE_DISABLE_GRPS_AC_DEPS_CHECK]
+   --enable_endpoint_metrics     Whether to enable metrics for each HTTP/gRPC endpoint. (default: false, ie disable metrics) [$BAZEL_REMOTE_ENABLE_ENDPOINT_METRICS]
    --help, -h                    show help (default: false)
 ```
 
@@ -157,6 +158,9 @@ host: localhost
 # to by ActionResult messages are in the cache.
 #disable_grpc_ac_deps_check: false
 
+# If set to true, enable metrics for each HTTP/gRPC endpoint.
+#enable_endpoint_metrics: false
+
 # At most one of the proxy backends can be selected:
 #
 #gcs_proxy:
diff --git a/cache/disk/disk.go b/cache/disk/disk.go
index 2d7eb8d..31fe6b4 100644
--- a/cache/disk/disk.go
+++ b/cache/disk/disk.go
@@ -539,9 +539,10 @@ func (c *Cache) Contains(kind cache.EntryKind, hash string) (bool, int64) {
 
 	var foundLocally bool
 	size := int64(-1)
+	key := cacheKey(kind, hash)
 
 	c.mu.Lock()
-	val, found := c.lru.Get(cacheKey(kind, hash))
+	val, found := c.lru.Get(key)
 	// Uncommitted (i.e. uploading items) should be reported as not ok
 	if found {
 		item := val.(*lruItem)
diff --git a/config/config.go b/config/config.go
index 452f16e..2ad6b3c 100644
--- a/config/config.go
+++ b/config/config.go
@@ -52,6 +52,7 @@ type Config struct {
 	IdleTimeout             time.Duration             `yaml:"idle_timeout"`
 	DisableHTTPACValidation bool                      `yaml:"disable_http_ac_validation"`
 	DisableGRPCACDepsCheck  bool                      `yaml:"disable_grpc_ac_deps_check"`
+	EnableEndpointMetrics   bool                      `yaml:"enable_endpoint_metrics"`
 }
 
 // New returns a validated Config with the specified values, and an error
@@ -60,7 +61,7 @@ func New(dir string, maxSize int, host string, port int, grpcPort int,
 	profileHost string, profilePort int, htpasswdFile string,
 	tlsCertFile string, tlsKeyFile string, idleTimeout time.Duration,
 	s3 *S3CloudStorageConfig, disableHTTPACValidation bool,
-	disableGRPCACDepsCheck bool) (*Config, error) {
+	disableGRPCACDepsCheck bool, enableEndpointMetrics bool) (*Config, error) {
 	c := Config{
 		Host:                    host,
 		Port:                    port,
@@ -78,6 +79,7 @@ func New(dir string, maxSize int, host string, port int, grpcPort int,
 		IdleTimeout:             idleTimeout,
 		DisableHTTPACValidation: disableHTTPACValidation,
 		DisableGRPCACDepsCheck:  disableGRPCACDepsCheck,
+		EnableEndpointMetrics:   enableEndpointMetrics,
 	}
 
 	err := validateConfig(&c)
diff --git a/main.go b/main.go
index 0086c2e..f080a4f 100644
--- a/main.go
+++ b/main.go
@@ -40,6 +40,9 @@ const (
 // is set through linker options.
 var gitCommit string
 
+// durationBuckets is the buckets used for Prometheus histograms in seconds.
+var durationBuckets = []float64{.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320}
+
 func main() {
 
 	log.SetFlags(logFlags)
@@ -197,6 +200,12 @@ func main() {
 			DefaultText: "false, ie enable ActionCache dependency checks",
 			EnvVars:     []string{"BAZEL_REMOTE_DISABLE_GRPS_AC_DEPS_CHECK"},
 		},
+		&cli.BoolFlag{
+			Name:        "enable_endpoint_metrics",
+			Usage:       "Whether to enable metrics for each HTTP/gRPC endpoint.",
+			DefaultText: "false, ie disable metrics",
+			EnvVars:     []string{"BAZEL_REMOTE_ENABLE_ENDPOINT_METRICS"},
+		},
 	}
 
 	app.Action = func(ctx *cli.Context) error {
@@ -234,6 +243,7 @@ func main() {
 				s3,
 				ctx.Bool("disable_http_ac_validation"),
 				ctx.Bool("disable_grpc_ac_deps_check"),
+				ctx.Bool("enable_endpoint_metrics"),
 			)
 		}
 
@@ -291,11 +301,6 @@ func main() {
 
 		validateAC := !c.DisableHTTPACValidation
 		h := server.NewHTTPCache(diskCache, accessLogger, errorLogger, validateAC, gitCommit)
-		metricsMdlw := httpmiddleware.New(httpmiddleware.Config{
-			Recorder: httpmetrics.NewRecorder(httpmetrics.Config{}),
-		})
-		mux.Handle("/metrics", metricsMdlw.Handler("metrics", promhttp.Handler()))
-		mux.Handle("/status", metricsMdlw.Handler("status", http.HandlerFunc(h.StatusPageHandler)))
 
 		var htpasswdSecrets auth.SecretProvider
 		cacheHandler := h.CacheHandler
@@ -310,9 +315,21 @@ func main() {
 			cacheHandler = wrapIdleHandler(cacheHandler, idleTimer, accessLogger, httpServer)
 		}
 
-		mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
-			metricsMdlw.Handler(r.Method, http.HandlerFunc(cacheHandler)).ServeHTTP(w, r)
-		})
+		if c.EnableEndpointMetrics {
+			metricsMdlw := httpmiddleware.New(httpmiddleware.Config{
+				Recorder: httpmetrics.NewRecorder(httpmetrics.Config{
+					DurationBuckets: durationBuckets,
+				}),
+			})
+			mux.Handle("/metrics", metricsMdlw.Handler("metrics", promhttp.Handler()))
+			mux.Handle("/status", metricsMdlw.Handler("status", http.HandlerFunc(h.StatusPageHandler)))
+			mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+				metricsMdlw.Handler(r.Method, http.HandlerFunc(cacheHandler)).ServeHTTP(w, r)
+			})
+		} else {
+			mux.HandleFunc("/status", h.StatusPageHandler)
+			mux.HandleFunc("/", cacheHandler)
+		}
 
 		if c.GRPCPort > 0 {
 
@@ -324,8 +341,14 @@ func main() {
 				addr := c.Host + ":" + strconv.Itoa(c.GRPCPort)
 
 				opts := []grpc.ServerOption{}
-				streamInterceptors := []grpc.StreamServerInterceptor{grpc_prometheus.StreamServerInterceptor}
-				unaryInterceptors := []grpc.UnaryServerInterceptor{grpc_prometheus.UnaryServerInterceptor}
+				streamInterceptors := []grpc.StreamServerInterceptor{}
+				unaryInterceptors := []grpc.UnaryServerInterceptor{}
+
+				if c.EnableEndpointMetrics {
+					streamInterceptors = append(streamInterceptors, grpc_prometheus.StreamServerInterceptor)
+					unaryInterceptors = append(unaryInterceptors, grpc_prometheus.UnaryServerInterceptor)
+					grpc_prometheus.EnableHandlingTimeHistogram(grpc_prometheus.WithHistogramBuckets(durationBuckets))
+				}
 
 				if len(c.TLSCertFile) > 0 && len(c.TLSKeyFile) > 0 {
 					creds, err2 := credentials.NewServerTLSFromFile(


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index b48f72e..c933bc3 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -18,6 +18,7 @@ htpasswd_file: /opt/.htpasswd
 tls_cert_file: /opt/tls.cert
 tls_key_file:  /opt/tls.key
 disable_http_ac_validation: true
+enable_endpoint_metrics: true
 `
 
 	config, err := newFromYaml([]byte(yaml))
@@ -35,6 +36,7 @@ disable_http_ac_validation: true
 		TLSCertFile:             "/opt/tls.cert",
 		TLSKeyFile:              "/opt/tls.key",
 		DisableHTTPACValidation: true,
+		EnableEndpointMetrics:   true,
 	}
 
 	if !reflect.DeepEqual(config, expectedConfig) {