From 38c997eef4930d4c90c14e67c1db90f2ba6e841d Mon Sep 17 00:00:00 2001 From: aarnautu Date: Tue, 19 Sep 2023 23:04:32 +0300 Subject: [PATCH] This change adds support to configurable prometheus buckets http_request_duration has fixed, hardcoded number of buckets with no possibility to tweak them For cases when the most of the latencies are above 1ms, with only 4 available buckets there's no good insight on OPA's performance. This implementation: - adds the possibility for the buckets to be configurable in ```server.metrics.prom.http_request_duration_seconds.buckets``` key - it's not a breaking change, if the buckets are not present in the configuration, the metric is configured with the existing values as a fallback Signed-off-by: aarnautu --- config/config.go | 1 + config/config_test.go | 14 ++ docs/content/configuration.md | 13 +- internal/prometheus/prometheus.go | 19 +-- internal/prometheus/prometheus_go1.19_test.go | 2 +- internal/prometheus/prometheus_test.go | 2 +- plugins/server/metrics/config.go | 93 +++++++++++++ plugins/server/metrics/config_test.go | 129 ++++++++++++++++++ runtime/runtime.go | 29 +++- runtime/runtime_test.go | 36 +++++ server/server_test.go | 2 +- 11 files changed, 316 insertions(+), 24 deletions(-) create mode 100644 plugins/server/metrics/config.go create mode 100644 plugins/server/metrics/config_test.go diff --git a/config/config.go b/config/config.go index bb914d8376..2e3fa10b77 100644 --- a/config/config.go +++ b/config/config.go @@ -39,6 +39,7 @@ type Config struct { DistributedTracing json.RawMessage `json:"distributed_tracing,omitempty"` Server *struct { Encoding json.RawMessage `json:"encoding,omitempty"` + Metrics json.RawMessage `json:"metrics,omitempty"` } `json:"server,omitempty"` Storage *struct { Disk json.RawMessage `json:"disk,omitempty"` diff --git a/config/config_test.go b/config/config_test.go index 803758b292..3922749be7 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -202,6 +202,13 @@ func TestActiveConfig(t *testing.T) { "min_length": 1024, "compression_level": 1 } + }, + "metrics": { + "prom": { + "http_request_duration_seconds": { + "buckets": [0.1, 0.2] + } + } } }, "discovery": {"name": "config"}` @@ -263,6 +270,13 @@ func TestActiveConfig(t *testing.T) { "min_length": 1024, "compression_level": 1 } + }, + "metrics": { + "prom": { + "http_request_duration_seconds": { + "buckets": [0.1, 0.2] + } + } } }, "default_authorization_decision": "/system/authz/allow", diff --git a/docs/content/configuration.md b/docs/content/configuration.md index 12c602f219..baeaddc114 100644 --- a/docs/content/configuration.md +++ b/docs/content/configuration.md @@ -991,10 +991,13 @@ See [the docs on disk storage](../misc-disk/) for details about the settings. ### Server -The `server` configuration sets the gzip compression settings for `/v0/data`, `/v1/data` and `/v1/compile` HTTP `POST` endpoints +The `server` configuration sets: +- the gzip compression settings for `/v0/data`, `/v1/data` and `/v1/compile` HTTP `POST` endpoints The gzip compression settings are used when the client sends `Accept-Encoding: gzip` +- buckets for `http_request_duration_seconds` histogram -| Field | Type | Required | Description | -|------------------------------------------|-------|---------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `server.encoding.gzip.min_length` | `int` | No, (default: 1024) | Specifies the minimum length of the response to compress | -| `server.encoding.gzip.compression_level` | `int` | No, (default: 9) | Specifies the compression level. Accepted values: a value of either 0 (no compression), 1 (best speed, lowest compression) or 9 (slowest, best compression). See https://pkg.go.dev/compress/flate#pkg-constants | +| Field | Type | Required | Description | +|-------------------------------------------------------------|-------------|---------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `server.encoding.gzip.min_length` | `int` | No, (default: 1024) | Specifies the minimum length of the response to compress | +| `server.encoding.gzip.compression_level` | `int` | No, (default: 9) | Specifies the compression level. Accepted values: a value of either 0 (no compression), 1 (best speed, lowest compression) or 9 (slowest, best compression). See https://pkg.go.dev/compress/flate#pkg-constants | +| `server.metrics.prom.http_request_duration_seconds.buckets` | `[]float64` | No, (default: [1e-6, 5e-6, 1e-5, 5e-5, 1e-4, 5e-4, 1e-3, 0.01, 0.1, 1 ]) | Specifies the buckets for the `http_request_duration_seconds` metric. Each value is a float, it is expressed in seconds and subdivisions of it. E.g `1e-6` is 1 microsecond, `1e-3` 1 millisecond, `0.01` 10 milliseconds | diff --git a/internal/prometheus/prometheus.go b/internal/prometheus/prometheus.go index a9bac6ab22..5646a01d29 100644 --- a/internal/prometheus/prometheus.go +++ b/internal/prometheus/prometheus.go @@ -37,25 +37,14 @@ type Provider struct { type loggerFunc func(attrs map[string]interface{}, f string, a ...interface{}) // New returns a new Provider object. -func New(inner metrics.Metrics, logger loggerFunc) *Provider { +func New(inner metrics.Metrics, logger loggerFunc, httpRequestBuckets []float64) *Provider { registry := prometheus.NewRegistry() registry.MustRegister(collector()) durationHistogram := prometheus.NewHistogramVec( prometheus.HistogramOpts{ - Name: "http_request_duration_seconds", - Help: "A histogram of duration for requests.", - Buckets: []float64{ - 1e-6, // 1 microsecond - 5e-6, - 1e-5, - 5e-5, - 1e-4, - 5e-4, - 1e-3, // 1 millisecond - 0.01, - 0.1, - 1, // 1 second - }, + Name: "http_request_duration_seconds", + Help: "A histogram of duration for requests.", + Buckets: httpRequestBuckets, }, []string{"code", "handler", "method"}, ) diff --git a/internal/prometheus/prometheus_go1.19_test.go b/internal/prometheus/prometheus_go1.19_test.go index ffed0ca14f..4b253d8ada 100644 --- a/internal/prometheus/prometheus_go1.19_test.go +++ b/internal/prometheus/prometheus_go1.19_test.go @@ -25,7 +25,7 @@ func TestJSONSerialization(t *testing.T) { } }(logging.NewNoOpLogger()) - prom := New(inner, logger) + prom := New(inner, logger, []float64{1e-6, 5e-6, 1e-5, 5e-5, 1e-4, 5e-4, 1e-3, 0.01, 0.1, 1}) m := prom.All() bs, err := json.Marshal(m) diff --git a/internal/prometheus/prometheus_test.go b/internal/prometheus/prometheus_test.go index bc9d0a934a..7f2c73f441 100644 --- a/internal/prometheus/prometheus_test.go +++ b/internal/prometheus/prometheus_test.go @@ -25,7 +25,7 @@ func TestJSONSerialization(t *testing.T) { } }(logging.NewNoOpLogger()) - prom := New(inner, logger) + prom := New(inner, logger, []float64{1e-6, 5e-6, 1e-5, 5e-5, 1e-4, 5e-4, 1e-3, 0.01, 0.1, 1}) m := prom.All() bs, err := json.Marshal(m) diff --git a/plugins/server/metrics/config.go b/plugins/server/metrics/config.go new file mode 100644 index 0000000000..e13bb8798e --- /dev/null +++ b/plugins/server/metrics/config.go @@ -0,0 +1,93 @@ +package metrics + +import ( + "github.com/open-policy-agent/opa/util" +) + +var defaultHTTPRequestBuckets = []float64{ + 1e-6, // 1 microsecond + 5e-6, + 1e-5, + 5e-5, + 1e-4, + 5e-4, + 1e-3, // 1 millisecond + 0.01, + 0.1, + 1, // 1 second +} + +// Config represents the configuration for the Server.Metrics settings +type Config struct { + Prom *Prom `json:"prom,omitempty"` +} + +// Prom represents the configuration for the Server.Metrics.Prom settings +type Prom struct { + HTTPRequestDurationSeconds *HTTPRequestDurationSeconds `json:"http_request_duration_seconds,omitempty"` +} + +// HTTPRequestDurationSeconds represents the configuration for the Server.Metrics.Prom.HTTPRequestDurationSeconds settings +type HTTPRequestDurationSeconds struct { + Buckets []float64 `json:"buckets,omitempty"` // the float64 array of buckets representing seconds or division of a second +} + +// ConfigBuilder assists in the construction of the plugin configuration. +type ConfigBuilder struct { + raw []byte +} + +// NewConfigBuilder returns a new ConfigBuilder to build and parse the server config +func NewConfigBuilder() *ConfigBuilder { + return &ConfigBuilder{} +} + +// WithBytes sets the raw server config +func (b *ConfigBuilder) WithBytes(config []byte) *ConfigBuilder { + b.raw = config + return b +} + +// Parse returns a valid Config object with defaults injected. +func (b *ConfigBuilder) Parse() (*Config, error) { + if b.raw == nil { + defaultConfig := &Config{ + Prom: &Prom{ + HTTPRequestDurationSeconds: &HTTPRequestDurationSeconds{ + Buckets: defaultHTTPRequestBuckets, + }, + }, + } + return defaultConfig, nil + } + + var result Config + + if err := util.Unmarshal(b.raw, &result); err != nil { + return nil, err + } + + return &result, result.validateAndInjectDefaults() +} + +func (c *Config) validateAndInjectDefaults() error { + if c.Prom == nil { + c.Prom = &Prom{ + HTTPRequestDurationSeconds: &HTTPRequestDurationSeconds{ + Buckets: defaultHTTPRequestBuckets, + }, + } + } + + if c.Prom.HTTPRequestDurationSeconds == nil { + c.Prom.HTTPRequestDurationSeconds = &HTTPRequestDurationSeconds{ + Buckets: defaultHTTPRequestBuckets, + } + } + + if c.Prom.HTTPRequestDurationSeconds.Buckets == nil { + c.Prom.HTTPRequestDurationSeconds.Buckets = defaultHTTPRequestBuckets + } + + return nil +} diff --git a/plugins/server/metrics/config_test.go b/plugins/server/metrics/config_test.go new file mode 100644 index 0000000000..c776d22adc --- /dev/null +++ b/plugins/server/metrics/config_test.go @@ -0,0 +1,129 @@ +package metrics + +import ( + "fmt" + "testing" +) + +func TestConfigValidation(t *testing.T) { + tests := []struct { + input string + wantErr bool + }{ + { + input: `{}`, + wantErr: false, + }, + { + input: `{"prom": {}}`, + wantErr: false, + }, + { + input: `{"prom": {"http_request_duration_seconds": {}}}`, + wantErr: false, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": []}}}`, + wantErr: false, + }, + + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": ["not-a-array"]}}}`, + wantErr: true, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": [1]}}}`, + wantErr: false, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": "1"}}}`, + wantErr: true, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": [0.001, "1", "2"]}}}`, + wantErr: true, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": ["one", "two", "three"]}}}`, + wantErr: true, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": ["0.1", "0.2", "0.3", "4"]}}}`, + wantErr: true, + }, + { + input: `{"prom": {"random_key": 0}}`, + wantErr: false, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"random_key": 0}}}`, + wantErr: false, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("TestConfigValidation_case_%d", i), func(t *testing.T) { + _, err := NewConfigBuilder().WithBytes([]byte(test.input)).Parse() + if err != nil && !test.wantErr { + t.Fail() + } + if err == nil && test.wantErr { + t.Fail() + } + }) + } +} + +func TestConfigValue(t *testing.T) { + tests := []struct { + input string + expectedValue []float64 + }{ + { + input: `{}`, + expectedValue: defaultHTTPRequestBuckets, + }, + { + input: `{"prom": {}}`, + expectedValue: defaultHTTPRequestBuckets, + }, + { + input: `{"prom": {"http_request_duration_seconds": {}}}`, + expectedValue: defaultHTTPRequestBuckets, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets": []}}}`, + expectedValue: []float64{}, + }, + { + input: `{"prom": {"http_request_duration_seconds": {"buckets":[0.1, 0.2, 0.3, 4]}}}`, + expectedValue: []float64{0.1, 0.2, 0.3, 4}, + }, + } + + for i, test := range tests { + t.Run(fmt.Sprintf("TestConfigValue_case_%d", i), func(t *testing.T) { + config, err := NewConfigBuilder().WithBytes([]byte(test.input)).Parse() + if err != nil { + t.Fail() + } + if !valuesAreEqual(config.Prom.HTTPRequestDurationSeconds.Buckets, test.expectedValue) { + t.Fail() + } + }) + } +} + +func valuesAreEqual(a []float64, b []float64) bool { + if len(a) != len(b) { + return false + } + + for i, v := range a { + if v != b[i] { + return false + } + } + + return true +} diff --git a/runtime/runtime.go b/runtime/runtime.go index a30816cff7..1935834e1e 100644 --- a/runtime/runtime.go +++ b/runtime/runtime.go @@ -32,6 +32,7 @@ import ( "go.uber.org/automaxprocs/maxprocs" "github.com/open-policy-agent/opa/bundle" + opa_config "github.com/open-policy-agent/opa/config" "github.com/open-policy-agent/opa/internal/config" internal_tracing "github.com/open-policy-agent/opa/internal/distributedtracing" internal_logging "github.com/open-policy-agent/opa/internal/logging" @@ -46,6 +47,7 @@ import ( "github.com/open-policy-agent/opa/plugins" "github.com/open-policy-agent/opa/plugins/discovery" "github.com/open-policy-agent/opa/plugins/logs" + metrics_config "github.com/open-policy-agent/opa/plugins/server/metrics" "github.com/open-policy-agent/opa/repl" "github.com/open-policy-agent/opa/server" "github.com/open-policy-agent/opa/storage" @@ -347,7 +349,11 @@ func NewRuntime(ctx context.Context, params Params) (*Runtime, error) { params.Router = mux.NewRouter() } - metrics := prometheus.New(metrics.New(), errorLogger(logger)) + metricsConfig, parseConfigErr := extractMetricsConfig(config, params) + if parseConfigErr != nil { + return nil, parseConfigErr + } + metrics := prometheus.New(metrics.New(), errorLogger(logger), metricsConfig.Prom.HTTPRequestDurationSeconds.Buckets) var store storage.Store if params.DiskStorage == nil { @@ -427,6 +433,27 @@ func NewRuntime(ctx context.Context, params Params) (*Runtime, error) { return rt, nil } +// extractMetricsConfig returns the configuration for server metrics and parsing errors if any +func extractMetricsConfig(config []byte, params Params) (*metrics_config.Config, error) { + var opaParsedConfig, opaParsedConfigErr = opa_config.ParseConfig(config, params.ID) + if opaParsedConfigErr != nil { + return nil, opaParsedConfigErr + } + + var serverMetricsData []byte + if opaParsedConfig.Server != nil { + serverMetricsData = opaParsedConfig.Server.Metrics + } + + var configBuilder = metrics_config.NewConfigBuilder() + var metricsParsedConfig, metricsParsedConfigErr = configBuilder.WithBytes(serverMetricsData).Parse() + if metricsParsedConfigErr != nil { + return nil, fmt.Errorf("server metrics configuration parse error: %w", metricsParsedConfigErr) + } + + return metricsParsedConfig, nil +} + // StartServer starts the runtime in server mode. This function will block the // calling goroutine and will exit the program on error. func (rt *Runtime) StartServer(ctx context.Context) { diff --git a/runtime/runtime_test.go b/runtime/runtime_test.go index 207d429176..41f8e60f35 100644 --- a/runtime/runtime_test.go +++ b/runtime/runtime_test.go @@ -603,3 +603,39 @@ func TestAddrWarningMessage(t *testing.T) { }) } } + +func TestRuntimeWithExplicitMetricConfiguration(t *testing.T) { + fs := map[string]string{ + "/config.yaml": `{"server": {"metrics": {"prom": {"http_request_duration_seconds": {"buckets": [0.1, 0.2, 0.3]}}}}}`, + } + + test.WithTempFS(fs, func(testDirRoot string) { + params := NewParams() + params.ConfigFile = filepath.Join(testDirRoot, "/config.yaml") + + _, err := NewRuntime(context.Background(), params) + if err != nil { + t.Fatalf(err.Error()) + } + }) +} + +func TestRuntimeWithExplicitBadMetricConfiguration(t *testing.T) { + fs := map[string]string{ + "/config.yaml": `{"server": {"metrics": {"prom": {"http_request_duration_seconds": {"buckets": "would-not-work"}}}}}`, + } + + test.WithTempFS(fs, func(testDirRoot string) { + params := NewParams() + params.ConfigFile = filepath.Join(testDirRoot, "/config.yaml") + + _, err := NewRuntime(context.Background(), params) + if err == nil { + t.Fatalf("Expected error to be thrown on malformed metrics config") + } + + if !strings.HasPrefix(err.Error(), "server metrics configuration parse error") { + t.Fatalf("Expected specific error to be thrown on malformed metrics config") + } + }) +} diff --git a/server/server_test.go b/server/server_test.go index 461a1b9ed4..d99e3c9676 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -3415,7 +3415,7 @@ func TestStatusV1MetricsWithSystemAuthzPolicy(t *testing.T) { } }(logging.NewNoOpLogger()) - prom := prometheus.New(inner, logger) + prom := prometheus.New(inner, logger, []float64{1e-6, 5e-6, 1e-5, 5e-5, 1e-4, 5e-4, 1e-3, 0.01, 0.1, 1}) serverOpts := []func(s *Server){func(s *Server) { s.WithAuthorization(AuthorizationBasic) }, func(s *Server) { s.WithMetrics(prom) }} f := newFixtureWithStore(t, store, serverOpts...)