Repository: buchgr/bazel-remote (go)

Issue:
Configure http request duration buckets for lower latency caches
Some smaller bucket sizes should be added here to cover lower latency caches: https://github.com/buchgr/bazel-remote/blob/b42d315f3763f9d365c6fd4119e22e66ea252d20/main.go#L50

Here's some numbers from a cache deployment with 4 replicas of bazel-remote (all replicas restarted in the last 24h):

Taking a look at the bucket counters themselves:
```
sum(http_request_duration_seconds_bucket{service="bazel-remote", endpoint!="metrics"}) by (le)
```

```
{le="0.5"}	30528468 # 30528468
{le="1"}	30541059 # 12591
{le="2.5"}	30543574 # 2515
{le="5"}	30543596 # 22
{le="10"}	30543605 # 9
{le="20"}	30543605
{le="40"}	30543605
{le="80"}	30543605
{le="160"}	30543605
{le="320"}	30543605
{le="+Inf"}	30543605
```

Which shows that around 99.9% of request durations are 0.5 or lower (histogram buckets are cumulative counts).

Calculating the mean request duration:

```
sum(http_request_duration_seconds_sum{service="bazel-remote", endpoint!="metrics"})
  /
sum(http_request_duration_seconds_count{service="bazel-remote", endpoint!="metrics"})
```

Returns 0.00845, which is a couple of orders of magnitude lower than the lowest bucket duration.

---

We can do one of:

1. Add some extra smaller buckets at the lower end (increases the cardinality again)
2. Keep the number of buckets the same, but reduce the smallest few
3. Make the duration configurable

Making durations configurable is possibly the best option, since altering bucket sizes makes querying old data less useful.

fwiw the default prometheus go client range looks like it would be better for my deployment at least: https://github.com/prometheus/client_golang/blob/b7799362e0ac323f658fb8d52c2d6df001cf272c/prometheus/histogram.go#L63

Pull request description:
Add new endpoint_metrics_duration_buckets config option
This allows people to configure a more suitable histogram range for their deployment. If the option is not specified, the default remains the same as the current values.

Fixes #389.

Patch:
diff --git a/README.md b/README.md
index f319e51..17605c0 100644
--- a/README.md
+++ b/README.md
@@ -198,6 +198,9 @@ host: localhost
 # If set to true, enable metrics for each HTTP/gRPC endpoint.
 #enable_endpoint_metrics: false
 
+# Specify a custom list of histogram buckets for endpoint request duration metrics
+#endpoint_metrics_duration_buckets: [.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320]
+
 # At most one of the proxy backends can be selected:
 #
 # If this is 0, proxy backends won't upload blobs.
diff --git a/config/config.go b/config/config.go
index 655ffdc..2abd9b6 100644
--- a/config/config.go
+++ b/config/config.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"os"
+	"sort"
 	"time"
 
 	yaml "gopkg.in/yaml.v2"
@@ -58,11 +59,14 @@ type Config struct {
 	DisableGRPCACDepsCheck      bool                      `yaml:"disable_grpc_ac_deps_check"`
 	EnableACKeyInstanceMangling bool                      `yaml:"enable_ac_key_instance_mangling"`
 	EnableEndpointMetrics       bool                      `yaml:"enable_endpoint_metrics"`
+	MetricsDurationBuckets      []float64                 `yaml:"endpoint_metrics_duration_buckets"`
 	ExperimentalRemoteAssetAPI  bool                      `yaml:"experimental_remote_asset_api"`
 	HTTPReadTimeout             time.Duration             `yaml:"http_read_timeout"`
 	HTTPWriteTimeout            time.Duration             `yaml:"http_write_timeout"`
 }
 
+var defaultDurationBuckets = []float64{.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320}
+
 // New returns a validated Config with the specified values, and an error
 // if there were any problems with the validation.
 func New(dir string, maxSize int, host string, port int, grpcPort int,
@@ -104,6 +108,7 @@ func New(dir string, maxSize int, host string, port int, grpcPort int,
 		DisableGRPCACDepsCheck:      disableGRPCACDepsCheck,
 		EnableACKeyInstanceMangling: enableACKeyInstanceMangling,
 		EnableEndpointMetrics:       enableEndpointMetrics,
+		MetricsDurationBuckets:      defaultDurationBuckets,
 		ExperimentalRemoteAssetAPI:  experimentalRemoteAssetAPI,
 		HTTPReadTimeout:             httpReadTimeout,
 		HTTPWriteTimeout:            httpWriteTimeout,
@@ -136,8 +141,9 @@ func NewFromYamlFile(path string) (*Config, error) {
 
 func newFromYaml(data []byte) (*Config, error) {
 	c := Config{
-		NumUploaders:     100,
-		MaxQueuedUploads: 1000000,
+		NumUploaders:           100,
+		MaxQueuedUploads:       1000000,
+		MetricsDurationBuckets: defaultDurationBuckets,
 	}
 
 	err := yaml.Unmarshal(data, &c)
@@ -145,6 +151,10 @@ func newFromYaml(data []byte) (*Config, error) {
 		return nil, fmt.Errorf("Failed to parse YAML config: %v", err)
 	}
 
+	if c.MetricsDurationBuckets != nil {
+		sort.Float64s(c.MetricsDurationBuckets)
+	}
+
 	err = validateConfig(&c)
 	if err != nil {
 		return nil, err
@@ -211,5 +221,16 @@ func validateConfig(c *Config) error {
 		}
 	}
 
+	if c.MetricsDurationBuckets != nil {
+		duplicates := make(map[float64]bool)
+		for _, bucket := range c.MetricsDurationBuckets {
+			_, dupe := duplicates[bucket]
+			if dupe {
+				return errors.New("'endpoint_metrics_duration_buckets' must not contain duplicate buckets")
+			}
+			duplicates[bucket] = true
+		}
+	}
+
 	return nil
 }
diff --git a/main.go b/main.go
index 3e01952..a97b954 100644
--- a/main.go
+++ b/main.go
@@ -46,9 +46,6 @@ 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)
@@ -435,7 +432,7 @@ func main() {
 		if c.EnableEndpointMetrics {
 			metricsMdlw := middleware.New(middleware.Config{
 				Recorder: httpmetrics.NewRecorder(httpmetrics.Config{
-					DurationBuckets: durationBuckets,
+					DurationBuckets: c.MetricsDurationBuckets,
 				}),
 			})
 			mux.Handle("/metrics", middlewarestd.Handler("metrics", metricsMdlw, promhttp.Handler()))
@@ -464,7 +461,7 @@ func main() {
 				if c.EnableEndpointMetrics {
 					streamInterceptors = append(streamInterceptors, grpc_prometheus.StreamServerInterceptor)
 					unaryInterceptors = append(unaryInterceptors, grpc_prometheus.UnaryServerInterceptor)
-					grpc_prometheus.EnableHandlingTimeHistogram(grpc_prometheus.WithHistogramBuckets(durationBuckets))
+					grpc_prometheus.EnableHandlingTimeHistogram(grpc_prometheus.WithHistogramBuckets(c.MetricsDurationBuckets))
 				}
 
 				if tlsConfig != nil {


Test patch:
diff --git a/config/config_test.go b/config/config_test.go
index 07303f3..25ef631 100644
--- a/config/config_test.go
+++ b/config/config_test.go
@@ -48,6 +48,7 @@ http_write_timeout: 10s
 		HTTPWriteTimeout:            10 * time.Second,
 		NumUploaders:                100,
 		MaxQueuedUploads:            1000000,
+		MetricsDurationBuckets:      []float64{.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320},
 	}
 
 	if !reflect.DeepEqual(config, expectedConfig) {
@@ -82,8 +83,9 @@ gcs_proxy:
 			UseDefaultCredentials: false,
 			JSONCredentialsFile:   "/opt/creds.json",
 		},
-		NumUploaders:     100,
-		MaxQueuedUploads: 1000000,
+		NumUploaders:           100,
+		MaxQueuedUploads:       1000000,
+		MetricsDurationBuckets: []float64{.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320},
 	}
 
 	if !cmp.Equal(config, expectedConfig) {
@@ -114,8 +116,9 @@ http_proxy:
 		HTTPBackend: &HTTPBackendConfig{
 			BaseURL: "https://remote-cache.com:8080/cache",
 		},
-		NumUploaders:     100,
-		MaxQueuedUploads: 1000000,
+		NumUploaders:           100,
+		MaxQueuedUploads:       1000000,
+		MetricsDurationBuckets: []float64{.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320},
 	}
 
 	if !cmp.Equal(config, expectedConfig) {
@@ -186,8 +189,9 @@ s3_proxy:
 			SecretAccessKey: "EXAMPLE_SECRET_KEY",
 			KeyVersion:      2,
 		},
-		NumUploaders:     100,
-		MaxQueuedUploads: 1000000,
+		NumUploaders:           100,
+		MaxQueuedUploads:       1000000,
+		MetricsDurationBuckets: []float64{.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320},
 	}
 
 	if !cmp.Equal(config, expectedConfig) {
@@ -208,14 +212,15 @@ profile_port: 7070
 	}
 
 	expectedConfig := &Config{
-		Host:             "localhost",
-		Port:             1234,
-		Dir:              "/opt/cache-dir",
-		MaxSize:          42,
-		ProfilePort:      7070,
-		ProfileHost:      "",
-		NumUploaders:     100,
-		MaxQueuedUploads: 1000000,
+		Host:                   "localhost",
+		Port:                   1234,
+		Dir:                    "/opt/cache-dir",
+		MaxSize:                42,
+		ProfilePort:            7070,
+		ProfileHost:            "",
+		NumUploaders:           100,
+		MaxQueuedUploads:       1000000,
+		MetricsDurationBuckets: []float64{.5, 1, 2.5, 5, 10, 20, 40, 80, 160, 320},
 	}
 
 	if !cmp.Equal(config, expectedConfig) {
@@ -236,3 +241,47 @@ profile_host: 192.168.1.1`
 		t.Fatalf("Expected '%+v' but got '%+v'", expectedConfig, config)
 	}
 }
+
+func TestValidMetricsDurationBuckets(t *testing.T) {
+	yaml := `host: localhost
+port: 1234
+dir: /opt/cache-dir
+max_size: 42
+endpoint_metrics_duration_buckets: [.005, .1, 5]
+`
+	config, err := newFromYaml([]byte(yaml))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expectedConfig := &Config{
+		Host:                   "localhost",
+		Port:                   1234,
+		Dir:                    "/opt/cache-dir",
+		MaxSize:                42,
+		NumUploaders:           100,
+		MaxQueuedUploads:       1000000,
+		MetricsDurationBuckets: []float64{0.005, 0.1, 5},
+	}
+
+	if !cmp.Equal(config, expectedConfig) {
+		t.Fatalf("Expected '%+v' but got '%+v'", expectedConfig, config)
+	}
+}
+
+func TestMetricsDurationBucketsNoDupliates(t *testing.T) {
+	testConfig := &Config{
+		Host:                   "localhost",
+		Port:                   8080,
+		MaxSize:                42,
+		Dir:                    "/opt/cache-dir",
+		MetricsDurationBuckets: []float64{1, 2, 3, 3},
+	}
+	err := validateConfig(testConfig)
+	if err == nil {
+		t.Fatal("Expected an error because 'endpoint_metrics_duration_buckets' contained a duplicate")
+	}
+	if !strings.Contains(err.Error(), "'endpoint_metrics_duration_buckets'") {
+		t.Fatal("Expected the error message to mention the invalid 'endpoint_metrics_duration_buckets' key")
+	}
+}