From 58d626f5b7f11dbed4971540e00a0d33de5d225d Mon Sep 17 00:00:00 2001 From: Jessie Date: Mon, 4 Nov 2024 17:28:43 -0500 Subject: [PATCH 1/4] Customize status bundle_loading_duration_ns Signed-off-by: Jessie Wu --- docs/content/configuration.md | 1 + docs/content/management-status.md | 3 + plugins/status/metrics.go | 154 +++++++++++++++++++++++------- plugins/status/plugin.go | 89 +++++++---------- plugins/status/plugin_test.go | 93 ++++++++++++++---- 5 files changed, 231 insertions(+), 109 deletions(-) diff --git a/docs/content/configuration.md b/docs/content/configuration.md index dc56b0427d..846ef06196 100644 --- a/docs/content/configuration.md +++ b/docs/content/configuration.md @@ -772,6 +772,7 @@ included in the actual bundle gzipped tarball. | `status.partition_name` | `string` | No | Path segment to include in status updates. | | `status.console` | `boolean` | No (default: `false`) | Log the status updates locally to the console. When enabled alongside a remote status update API the `service` must be configured, the default `service` selection will be disabled. | | `status.prometheus` | `boolean` | No (default: `false`) | Export the status (bundle and plugin) metrics to prometheus (see [the monitoring documentation](../monitoring/#prometheus)). When enabled alongside a remote status update API the `service` must be configured, the default `service` selection will be disabled. | +| `status.prometheus_config.bundle_loading_duration_ns.buckets` | `[]float64` | No, (Only use when status.prometheus true, default: [1000, 2000, 4000, 8000, 16_000, 32_000, 64_000, 128_000, 256_000, 512_000, 1_024_000, 2_048_000, 4_096_000, 8_192_000, 16_384_000, 32_768_000, 65_536_000, 131_072_000, 262_144_000, 524_288_000]) | Specifies the buckets for the `bundle_loading_duration_ns` metric. Each value is a float, it is expressed in nanoseconds. | | `status.plugin` | `string` | No | Use the named plugin for status updates. If this field exists, the other configuration fields are not required. | | `status.trigger` | `string` (default: `periodic`) | No | Controls how status updates are reported to the remote server. Allowed values are `periodic` and `manual` (`manual` triggers are only possible when using OPA as a Go package). | diff --git a/docs/content/management-status.md b/docs/content/management-status.md index 6cdfa0fb98..389a18866b 100644 --- a/docs/content/management-status.md +++ b/docs/content/management-status.md @@ -297,6 +297,9 @@ Example of minimal config to enable: ```yaml status: prometheus: true + prometheus_config: + bundle_loading_duration_ns: + buckets: [1, 1000, 10_000, 1e8] ``` When enabled the OPA instance's Prometheus endpoint exposes the metrics described on [the monitoring documentation](../monitoring/#status-metrics). diff --git a/plugins/status/metrics.go b/plugins/status/metrics.go index aa27e1cd36..58d13a76f0 100644 --- a/plugins/status/metrics.go +++ b/plugins/status/metrics.go @@ -1,82 +1,168 @@ package status import ( + "github.com/open-policy-agent/opa/logging" "github.com/open-policy-agent/opa/version" "github.com/prometheus/client_golang/prometheus" ) -var ( - opaInfo = prometheus.NewGauge( +var defaultBundleLoadStageBuckets = prometheus.ExponentialBuckets(1000, 2, 20) + +type PrometheusConfig struct { + BundleLoadDurationNanoseconds *BundleLoadDurationNanoseconds `json:"bundle_loading_duration_ns,omitempty"` +} + +func injectDefaultDurationBuckets(p *PrometheusConfig) *PrometheusConfig { + if p != nil && p.BundleLoadDurationNanoseconds != nil && p.BundleLoadDurationNanoseconds.Buckets != nil { + return p + } + + return &PrometheusConfig{ + BundleLoadDurationNanoseconds: &BundleLoadDurationNanoseconds{ + Buckets: defaultBundleLoadStageBuckets, + }, + } +} + +// collectors is a list of all collectors maintained by the status plugin. +// Note: when adding a new collector, make sure to also add it to this list, +// or it won't survive status plugin reconfigure events. +type collectors struct { + opaInfo prometheus.Gauge + pluginStatus *prometheus.GaugeVec + loaded *prometheus.CounterVec + failLoad *prometheus.CounterVec + lastRequest *prometheus.GaugeVec + lastSuccessfulActivation *prometheus.GaugeVec + lastSuccessfulDownload *prometheus.GaugeVec + lastSuccessfulRequest *prometheus.GaugeVec + bundleLoadDuration *prometheus.HistogramVec +} + +func newCollectors(prometheusConfig *PrometheusConfig) *collectors { + opaInfo := prometheus.NewGauge( prometheus.GaugeOpts{ Name: "opa_info", Help: "Information about the OPA environment.", ConstLabels: map[string]string{"version": version.Version}, }, ) - pluginStatus = prometheus.NewGaugeVec( + opaInfo.Set(1) // only publish once + + pluginStatus := prometheus.NewGaugeVec( prometheus.GaugeOpts{ Name: "plugin_status_gauge", - Help: "Gauge for the plugin by status."}, + Help: "Gauge for the plugin by status.", + }, []string{"name", "status"}, ) - loaded = prometheus.NewCounterVec( + loaded := prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "bundle_loaded_counter", - Help: "Counter for the bundle loaded."}, + Help: "Counter for the bundle loaded.", + }, []string{"name"}, ) - failLoad = prometheus.NewCounterVec( + failLoad := prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "bundle_failed_load_counter", - Help: "Counter for the failed bundle load."}, + Help: "Counter for the failed bundle load.", + }, []string{"name", "code", "message"}, ) - lastRequest = prometheus.NewGaugeVec( + lastRequest := prometheus.NewGaugeVec( prometheus.GaugeOpts{ Name: "last_bundle_request", - Help: "Gauge for the last bundle request."}, + Help: "Gauge for the last bundle request.", + }, []string{"name"}, ) - lastSuccessfulActivation = prometheus.NewGaugeVec( + lastSuccessfulActivation := prometheus.NewGaugeVec( prometheus.GaugeOpts{ Name: "last_success_bundle_activation", - Help: "Gauge for the last success bundle activation."}, + Help: "Gauge for the last success bundle activation.", + }, []string{"name", "active_revision"}, ) - lastSuccessfulDownload = prometheus.NewGaugeVec( + lastSuccessfulDownload := prometheus.NewGaugeVec( prometheus.GaugeOpts{ Name: "last_success_bundle_download", - Help: "Gauge for the last success bundle download."}, + Help: "Gauge for the last success bundle download.", + }, []string{"name"}, ) - lastSuccessfulRequest = prometheus.NewGaugeVec( + lastSuccessfulRequest := prometheus.NewGaugeVec( prometheus.GaugeOpts{ Name: "last_success_bundle_request", - Help: "Gauge for the last success bundle request."}, + Help: "Gauge for the last success bundle request.", + }, []string{"name"}, ) - bundleLoadDuration = prometheus.NewHistogramVec(prometheus.HistogramOpts{ + + bundleLoadDuration := newBundleLoadDurationCollector(prometheusConfig) + + return &collectors{ + opaInfo: opaInfo, + pluginStatus: pluginStatus, + loaded: loaded, + failLoad: failLoad, + lastRequest: lastRequest, + lastSuccessfulActivation: lastSuccessfulActivation, + lastSuccessfulDownload: lastSuccessfulDownload, + lastSuccessfulRequest: lastSuccessfulRequest, + bundleLoadDuration: bundleLoadDuration, + } +} + +func newBundleLoadDurationCollector(prometheusConfig *PrometheusConfig) *prometheus.HistogramVec { + return prometheus.NewHistogramVec(prometheus.HistogramOpts{ Name: "bundle_loading_duration_ns", Help: "Histogram for the bundle loading duration by stage.", - Buckets: prometheus.ExponentialBuckets(1000, 2, 20), + Buckets: prometheusConfig.BundleLoadDurationNanoseconds.Buckets, }, []string{"name", "stage"}) +} - // allCollectors is a list of all collectors maintained by the status plugin. - // Note: when adding a new collector, make sure to also add it to this list, - // or it won't survive status plugin reconfigure events. - allCollectors = []prometheus.Collector{ - opaInfo, - pluginStatus, - loaded, - failLoad, - lastRequest, - lastSuccessfulActivation, - lastSuccessfulDownload, - lastSuccessfulRequest, - bundleLoadDuration, +func (c *collectors) RegisterAll(register prometheus.Registerer, logger logging.Logger) { + if register == nil { + return } -) + for _, collector := range c.toList() { + if err := register.Register(collector); err != nil { + logger.Error("Status metric failed to register on prometheus :%v.", err) + } + } +} -func init() { - opaInfo.Set(1) +func (c *collectors) UnregisterAll(register prometheus.Registerer) { + if register == nil { + return + } + + for _, collector := range c.toList() { + register.Unregister(collector) + } +} + +func (c *collectors) ReregisterCustomizedCollectors(register prometheus.Registerer, config *PrometheusConfig, logger logging.Logger) { + logger.Info("Re-register bundleLoadDuration collector") + register.Unregister(c.bundleLoadDuration) + c.bundleLoadDuration = newBundleLoadDurationCollector(config) + if err := register.Register(c.bundleLoadDuration); err != nil { + logger.Error("Status metric failed to register bundleLoadDuration collector on prometheus :%v.", err) + } +} + +// helper function +func (c *collectors) toList() []prometheus.Collector { + return []prometheus.Collector{ + c.opaInfo, + c.pluginStatus, + c.loaded, + c.failLoad, + c.lastRequest, + c.lastSuccessfulActivation, + c.lastSuccessfulDownload, + c.lastSuccessfulRequest, + c.bundleLoadDuration, + } } diff --git a/plugins/status/plugin.go b/plugins/status/plugin.go index 8807524039..e418a1e3d8 100644 --- a/plugins/status/plugin.go +++ b/plugins/status/plugin.go @@ -12,8 +12,6 @@ import ( "net/http" "reflect" - prom "github.com/prometheus/client_golang/prometheus" - lstat "github.com/open-policy-agent/opa/plugins/logs/status" "github.com/open-policy-agent/opa/logging" @@ -62,16 +60,23 @@ type Plugin struct { metrics metrics.Metrics logger logging.Logger trigger chan trigger + collectors *collectors } // Config contains configuration for the plugin. type Config struct { - Plugin *string `json:"plugin"` - Service string `json:"service"` - PartitionName string `json:"partition_name,omitempty"` - ConsoleLogs bool `json:"console"` - Prometheus bool `json:"prometheus"` - Trigger *plugins.TriggerMode `json:"trigger,omitempty"` // trigger mode + Plugin *string `json:"plugin"` + Service string `json:"service"` + PartitionName string `json:"partition_name,omitempty"` + ConsoleLogs bool `json:"console"` + Prometheus bool `json:"prometheus"` + PrometheusConfig *PrometheusConfig `json:"prometheus_config,omitempty"` + Trigger *plugins.TriggerMode `json:"trigger,omitempty"` // trigger mode +} + +// BundleLoadDurationNanoseconds represents the configuration for the status.promtheus_config.bundle_loading_duration_ns settings +type BundleLoadDurationNanoseconds struct { + Buckets []float64 `json:"buckets,omitempty"` // the float64 array of buckets representing nanoseconds or multiple of a nanoseconds } type reconfigure struct { @@ -85,7 +90,6 @@ type trigger struct { } func (c *Config) validateAndInjectDefaults(services []string, pluginsList []string, trigger *plugins.TriggerMode) error { - if c.Plugin != nil { var found bool for _, other := range pluginsList { @@ -124,6 +128,8 @@ func (c *Config) validateAndInjectDefaults(services []string, pluginsList []stri } c.Trigger = t + c.PrometheusConfig = injectDefaultDurationBuckets(c.PrometheusConfig) + return nil } @@ -211,6 +217,7 @@ func New(parsedConfig *Config, manager *plugins.Manager) *Plugin { queryCh: make(chan chan *UpdateRequestV1), logger: manager.Logger().WithFields(map[string]interface{}{"plugin": Name}), trigger: make(chan trigger), + collectors: newCollectors(parsedConfig.PrometheusConfig), } p.manager.UpdatePluginStatus(Name, &plugins.Status{State: plugins.StateNotReady}) @@ -246,7 +253,7 @@ func (p *Plugin) Start(ctx context.Context) error { p.manager.RegisterPluginStatusListener(Name, p.UpdatePluginStatus) if p.config.Prometheus { - p.registerAll() + p.collectors.RegisterAll(p.manager.PrometheusRegister(), p.logger) } // Set the status plugin's status to OK now that everything is registered and @@ -256,32 +263,6 @@ func (p *Plugin) Start(ctx context.Context) error { return nil } -func (p *Plugin) register(r prom.Registerer, cs ...prom.Collector) { - for _, c := range cs { - if err := r.Register(c); err != nil { - p.logger.Error("Status metric failed to register on prometheus :%v.", err) - } - } -} - -func (p *Plugin) registerAll() { - if p.manager.PrometheusRegister() != nil { - p.register(p.manager.PrometheusRegister(), allCollectors...) - } -} - -func (p *Plugin) unregister(r prom.Registerer, cs ...prom.Collector) { - for _, c := range cs { - r.Unregister(c) - } -} - -func (p *Plugin) unregisterAll() { - if p.manager.PrometheusRegister() != nil { - p.unregister(p.manager.PrometheusRegister(), allCollectors...) - } -} - // Stop stops the plugin. func (p *Plugin) Stop(_ context.Context) { p.logger.Info("Stopping status reporter.") @@ -348,11 +329,9 @@ func (p *Plugin) Trigger(ctx context.Context) error { } func (p *Plugin) loop(ctx context.Context) { - ctx, cancel := context.WithCancel(ctx) for { - select { case statuses := <-p.pluginStatusCh: p.lastPluginStatuses = statuses @@ -429,7 +408,6 @@ func (p *Plugin) loop(ctx context.Context) { } func (p *Plugin) oneShot(ctx context.Context) error { - req := p.snapshot() if p.config.ConsoleLogs { @@ -440,7 +418,7 @@ func (p *Plugin) oneShot(ctx context.Context) error { } if p.config.Prometheus { - updatePrometheusMetrics(req) + p.updatePrometheusMetrics(req) } if p.config.Plugin != nil { @@ -455,7 +433,6 @@ func (p *Plugin) oneShot(ctx context.Context) error { resp, err := p.manager.Client(p.config.Service). WithJSON(req). Do(ctx, "POST", fmt.Sprintf("/status/%v", p.config.PartitionName)) - if err != nil { return fmt.Errorf("Status update failed: %w", err) } @@ -480,16 +457,17 @@ func (p *Plugin) reconfigure(config interface{}) { p.logger.Info("Status reporter configuration changed.") if newConfig.Prometheus && !p.config.Prometheus { - p.registerAll() + p.collectors.RegisterAll(p.manager.PrometheusRegister(), p.logger) } else if !newConfig.Prometheus && p.config.Prometheus { - p.unregisterAll() + p.collectors.UnregisterAll(p.manager.PrometheusRegister()) + } else if newConfig.Prometheus && p.config.Prometheus { + p.collectors.ReregisterCustomizedCollectors(p.manager.PrometheusRegister(), newConfig.PrometheusConfig, p.logger) } p.config = *newConfig } func (p *Plugin) snapshot() *UpdateRequestV1 { - s := &UpdateRequestV1{ Labels: p.manager.Labels(), Discovery: p.lastDiscoStatus, @@ -522,27 +500,28 @@ func (p *Plugin) logUpdate(update *UpdateRequestV1) error { return nil } -func updatePrometheusMetrics(u *UpdateRequestV1) { - pluginStatus.Reset() +func (p *Plugin) updatePrometheusMetrics(u *UpdateRequestV1) { + p.collectors.pluginStatus.Reset() for name, plugin := range u.Plugins { - pluginStatus.WithLabelValues(name, string(plugin.State)).Set(1) + p.collectors.pluginStatus.WithLabelValues(name, string(plugin.State)).Set(1) } - lastSuccessfulActivation.Reset() + p.collectors.lastSuccessfulActivation.Reset() for _, bundle := range u.Bundles { if bundle.Code == "" && !bundle.LastSuccessfulActivation.IsZero() { - loaded.WithLabelValues(bundle.Name).Inc() + p.collectors.loaded.WithLabelValues(bundle.Name).Inc() } else { - failLoad.WithLabelValues(bundle.Name, bundle.Code, bundle.Message).Inc() + p.collectors.failLoad.WithLabelValues(bundle.Name, bundle.Code, bundle.Message).Inc() } - lastSuccessfulActivation.WithLabelValues(bundle.Name, bundle.ActiveRevision).Set(float64(bundle.LastSuccessfulActivation.UnixNano())) - lastSuccessfulDownload.WithLabelValues(bundle.Name).Set(float64(bundle.LastSuccessfulDownload.UnixNano())) - lastSuccessfulRequest.WithLabelValues(bundle.Name).Set(float64(bundle.LastSuccessfulRequest.UnixNano())) - lastRequest.WithLabelValues(bundle.Name).Set(float64(bundle.LastRequest.UnixNano())) + p.collectors.lastSuccessfulActivation.WithLabelValues(bundle.Name, bundle.ActiveRevision).Set(float64(bundle.LastSuccessfulActivation.UnixNano())) + p.collectors.lastSuccessfulDownload.WithLabelValues(bundle.Name).Set(float64(bundle.LastSuccessfulDownload.UnixNano())) + p.collectors.lastSuccessfulRequest.WithLabelValues(bundle.Name).Set(float64(bundle.LastSuccessfulRequest.UnixNano())) + p.collectors.lastRequest.WithLabelValues(bundle.Name).Set(float64(bundle.LastRequest.UnixNano())) + if bundle.Metrics != nil { for stage, metric := range bundle.Metrics.All() { switch stage { case "timer_bundle_request_ns", "timer_rego_data_parse_ns", "timer_rego_module_parse_ns", "timer_rego_module_compile_ns", "timer_rego_load_bundles_ns": - bundleLoadDuration.WithLabelValues(bundle.Name, stage).Observe(float64(metric.(int64))) + p.collectors.bundleLoadDuration.WithLabelValues(bundle.Name, stage).Observe(float64(metric.(int64))) } } } diff --git a/plugins/status/plugin_test.go b/plugins/status/plugin_test.go index c5f167459c..f2ed314a65 100644 --- a/plugins/status/plugin_test.go +++ b/plugins/status/plugin_test.go @@ -11,6 +11,7 @@ import ( "net/http/httptest" "os" "reflect" + "slices" "strings" "testing" "time" @@ -36,6 +37,61 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +func TestConfigValueParse(t *testing.T) { + tests := []struct { + note string + input string + expectedNoConfig bool + expectedValue []float64 + }{ + { + note: "empty config", + input: `{}`, + expectedNoConfig: true, + }, + { + note: "empty prometheus config", + input: `{"prometheus": false}`, + expectedNoConfig: true, + }, + { + note: "no specific prom config, expected default config buckets", + input: `{"prometheus": true, "prometheus_config": {}}`, + expectedValue: defaultBundleLoadStageBuckets, + }, + { + note: "specified prom config, expected value same as config", + input: `{"prometheus": true, "prometheus_config": {"bundle_loading_duration_ns": {}}}`, + expectedValue: defaultBundleLoadStageBuckets, + }, + { + note: "specified prom config, expected value same as config", + input: `{"prometheus": true, "prometheus_config": {"bundle_loading_duration_ns": {"buckets": []}}}`, + expectedValue: []float64{}, + }, + { + note: "specified prom config, expected value same as config", + input: `{"prometheus": true, "prometheus_config": {"bundle_loading_duration_ns": {"buckets":[1, 1000, 1000_000, 1e8]}}}`, + expectedValue: []float64{1, 1000, 1000_000, 1e8}, + }, + } + + for _, test := range tests { + t.Run(test.note, func(t *testing.T) { + config, err := ParseConfig([]byte(test.input), []string{}, []string{"status"}) + if err != nil { + t.Errorf("expected no error: %v", err) + } + if test.expectedNoConfig && config != nil { + t.Errorf("expected parsed config is nil, got %v", config) + } + if !test.expectedNoConfig && !slices.Equal(config.PrometheusConfig.BundleLoadDurationNanoseconds.Buckets, test.expectedValue) { + t.Errorf("expected %v, got %v", test.expectedValue, config.PrometheusConfig.BundleLoadDurationNanoseconds.Buckets) + } + }) + } +} + func TestPluginPrometheus(t *testing.T) { fixture := newTestFixture(t, nil, func(c *Config) { c.Prometheus = true @@ -61,65 +117,65 @@ func TestPluginPrometheus(t *testing.T) { assertOpInformationGauge(t, registerMock) - if registerMock.Collectors[pluginStatus] != true { + if registerMock.Collectors[fixture.plugin.collectors.pluginStatus] != true { t.Fatalf("Plugin status metric was not registered on prometheus") } - if registerMock.Collectors[loaded] != true { + if registerMock.Collectors[fixture.plugin.collectors.loaded] != true { t.Fatalf("Loaded metric was not registered on prometheus") } - if registerMock.Collectors[failLoad] != true { + if registerMock.Collectors[fixture.plugin.collectors.failLoad] != true { t.Fatalf("FailLoad metric was not registered on prometheus") } - if registerMock.Collectors[lastRequest] != true { + if registerMock.Collectors[fixture.plugin.collectors.lastRequest] != true { t.Fatalf("Last request metric was not registered on prometheus") } - if registerMock.Collectors[lastSuccessfulActivation] != true { + if registerMock.Collectors[fixture.plugin.collectors.lastSuccessfulActivation] != true { t.Fatalf("Last Successful Activation metric was not registered on prometheus") } - if registerMock.Collectors[lastSuccessfulDownload] != true { + if registerMock.Collectors[fixture.plugin.collectors.lastSuccessfulDownload] != true { t.Fatalf("Last Successful Download metric was not registered on prometheus") } - if registerMock.Collectors[lastSuccessfulRequest] != true { + if registerMock.Collectors[fixture.plugin.collectors.lastSuccessfulRequest] != true { t.Fatalf("Last Successful Request metric was not registered on prometheus") } - if registerMock.Collectors[bundleLoadDuration] != true { + if registerMock.Collectors[fixture.plugin.collectors.bundleLoadDuration] != true { t.Fatalf("Bundle Load Duration metric was not registered on prometheus") } if len(registerMock.Collectors) != 9 { t.Fatalf("Number of collectors expected (%v), got %v", 9, len(registerMock.Collectors)) } - lastRequestMetricResult := time.UnixMilli(int64(testutil.ToFloat64(lastRequest) / 1e6)) + lastRequestMetricResult := time.UnixMilli(int64(testutil.ToFloat64(fixture.plugin.collectors.lastRequest) / 1e6)) if !lastRequestMetricResult.Equal(status.LastRequest) { t.Fatalf("Last request expected (%v), got %v", status.LastRequest.UTC(), lastRequestMetricResult.UTC()) } - lastSuccessfulRequestMetricResult := time.UnixMilli(int64(testutil.ToFloat64(lastSuccessfulRequest) / 1e6)) + lastSuccessfulRequestMetricResult := time.UnixMilli(int64(testutil.ToFloat64(fixture.plugin.collectors.lastSuccessfulRequest) / 1e6)) if !lastSuccessfulRequestMetricResult.Equal(status.LastSuccessfulRequest) { t.Fatalf("Last request expected (%v), got %v", status.LastSuccessfulRequest.UTC(), lastSuccessfulRequestMetricResult.UTC()) } - lastSuccessfulDownloadMetricResult := time.UnixMilli(int64(testutil.ToFloat64(lastSuccessfulDownload) / 1e6)) + lastSuccessfulDownloadMetricResult := time.UnixMilli(int64(testutil.ToFloat64(fixture.plugin.collectors.lastSuccessfulDownload) / 1e6)) if !lastSuccessfulDownloadMetricResult.Equal(status.LastSuccessfulDownload) { t.Fatalf("Last request expected (%v), got %v", status.LastSuccessfulDownload.UTC(), lastSuccessfulDownloadMetricResult.UTC()) } - lastSuccessfulActivationMetricResult := time.UnixMilli(int64(testutil.ToFloat64(lastSuccessfulActivation) / 1e6)) + lastSuccessfulActivationMetricResult := time.UnixMilli(int64(testutil.ToFloat64(fixture.plugin.collectors.lastSuccessfulActivation) / 1e6)) if !lastSuccessfulActivationMetricResult.Equal(status.LastSuccessfulActivation) { t.Fatalf("Last request expected (%v), got %v", status.LastSuccessfulActivation.UTC(), lastSuccessfulActivationMetricResult.UTC()) } - bundlesLoaded := testutil.CollectAndCount(loaded) + bundlesLoaded := testutil.CollectAndCount(fixture.plugin.collectors.loaded) if bundlesLoaded != 1 { t.Fatalf("Unexpected number of bundle loads (%v), got %v", 1, bundlesLoaded) } - bundlesFailedToLoad := testutil.CollectAndCount(failLoad) + bundlesFailedToLoad := testutil.CollectAndCount(fixture.plugin.collectors.failLoad) if bundlesFailedToLoad != 0 { t.Fatalf("Unexpected number of bundle fails load (%v), got %v", 0, bundlesFailedToLoad) } - pluginsStatus := testutil.CollectAndCount(pluginStatus) + pluginsStatus := testutil.CollectAndCount(fixture.plugin.collectors.pluginStatus) if pluginsStatus != 1 { t.Fatalf("Unexpected number of plugins (%v), got %v", 1, pluginsStatus) } @@ -228,12 +284,12 @@ func TestMetricsBundleWithoutRevision(t *testing.T) { fixture.plugin.BulkUpdateBundleStatus(map[string]*bundle.Status{"bundle": status}) <-fixture.server.ch - bundlesLoaded := testutil.CollectAndCount(loaded) + bundlesLoaded := testutil.CollectAndCount(fixture.plugin.collectors.loaded) if bundlesLoaded != 1 { t.Fatalf("Unexpected number of bundle loads (%v), got %v", 1, bundlesLoaded) } - bundlesFailedToLoad := testutil.CollectAndCount(failLoad) + bundlesFailedToLoad := testutil.CollectAndCount(fixture.plugin.collectors.failLoad) if bundlesFailedToLoad != 0 { t.Fatalf("Unexpected number of bundle fails load (%v), got %v", 0, bundlesFailedToLoad) } @@ -847,7 +903,6 @@ func TestParseConfigUseDefaultServiceNoConsole(t *testing.T) { }`) config, err := ParseConfig(loggerConfig, services, nil) - if err != nil { t.Errorf("Unexpected error: %s", err) } @@ -869,7 +924,6 @@ func TestParseConfigDefaultServiceWithConsole(t *testing.T) { }`) config, err := ParseConfig(loggerConfig, services, nil) - if err != nil { t.Errorf("Unexpected error: %s", err) } @@ -915,7 +969,6 @@ func TestParseConfigTriggerMode(t *testing.T) { for _, tc := range cases { t.Run(tc.note, func(t *testing.T) { - c, err := NewConfigBuilder().WithBytes(tc.config).WithServices([]string{"s0"}).WithTriggerMode(&tc.expected).Parse() if tc.wantErr { From fba2a44328199ea1034d620ac50ea4bfd2f4df51 Mon Sep 17 00:00:00 2001 From: Jessie Wu Date: Tue, 5 Nov 2024 11:01:05 -0500 Subject: [PATCH 2/4] fmt: Redo extra newline removing Signed-off-by: Jessie Wu --- plugins/status/plugin_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/status/plugin_test.go b/plugins/status/plugin_test.go index f2ed314a65..ba7a3f953b 100644 --- a/plugins/status/plugin_test.go +++ b/plugins/status/plugin_test.go @@ -903,6 +903,7 @@ func TestParseConfigUseDefaultServiceNoConsole(t *testing.T) { }`) config, err := ParseConfig(loggerConfig, services, nil) + if err != nil { t.Errorf("Unexpected error: %s", err) } @@ -924,6 +925,7 @@ func TestParseConfigDefaultServiceWithConsole(t *testing.T) { }`) config, err := ParseConfig(loggerConfig, services, nil) + if err != nil { t.Errorf("Unexpected error: %s", err) } From dfdb4118017da54262cf5356754dabcb4abe1689 Mon Sep 17 00:00:00 2001 From: Jessie Wu Date: Tue, 5 Nov 2024 11:10:08 -0500 Subject: [PATCH 3/4] Update test cases note Signed-off-by: Jessie Wu --- plugins/status/plugin_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/status/plugin_test.go b/plugins/status/plugin_test.go index ba7a3f953b..8d99bb93c3 100644 --- a/plugins/status/plugin_test.go +++ b/plugins/status/plugin_test.go @@ -55,22 +55,22 @@ func TestConfigValueParse(t *testing.T) { expectedNoConfig: true, }, { - note: "no specific prom config, expected default config buckets", + note: "no specific prometheus config, expected default config buckets", input: `{"prometheus": true, "prometheus_config": {}}`, expectedValue: defaultBundleLoadStageBuckets, }, { - note: "specified prom config, expected value same as config", + note: "specified prometheus config, expected value same as config", input: `{"prometheus": true, "prometheus_config": {"bundle_loading_duration_ns": {}}}`, expectedValue: defaultBundleLoadStageBuckets, }, { - note: "specified prom config, expected value same as config", + note: "specified prometheus config, expected value same as config", input: `{"prometheus": true, "prometheus_config": {"bundle_loading_duration_ns": {"buckets": []}}}`, expectedValue: []float64{}, }, { - note: "specified prom config, expected value same as config", + note: "specified prometheus config, expected value same as config", input: `{"prometheus": true, "prometheus_config": {"bundle_loading_duration_ns": {"buckets":[1, 1000, 1000_000, 1e8]}}}`, expectedValue: []float64{1, 1000, 1000_000, 1e8}, }, From 6c112e57b77c56a3e6730061b88de241a2f4082f Mon Sep 17 00:00:00 2001 From: Jessie Wu Date: Tue, 5 Nov 2024 11:51:04 -0500 Subject: [PATCH 4/4] Fix unit tests with PrometheusConfig value Signed-off-by: Jessie Wu --- plugins/status/plugin_test.go | 5 +++++ server/server_test.go | 20 ++++++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/plugins/status/plugin_test.go b/plugins/status/plugin_test.go index 8d99bb93c3..58d438de48 100644 --- a/plugins/status/plugin_test.go +++ b/plugins/status/plugin_test.go @@ -54,6 +54,11 @@ func TestConfigValueParse(t *testing.T) { input: `{"prometheus": false}`, expectedNoConfig: true, }, + { + note: "no specific prometheus config, expected default config buckets", + input: `{"prometheus": true}`, + expectedValue: defaultBundleLoadStageBuckets, + }, { note: "no specific prometheus config, expected default config buckets", input: `{"prometheus": true, "prometheus_config": {}}`, diff --git a/server/server_test.go b/server/server_test.go index 8e80f77117..af6c3b1484 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -59,6 +59,7 @@ import ( "github.com/open-policy-agent/opa/util" "github.com/open-policy-agent/opa/util/test" "github.com/open-policy-agent/opa/version" + prom "github.com/prometheus/client_golang/prometheus" ) type tr struct { @@ -3603,7 +3604,14 @@ func TestStatusV1(t *testing.T) { // Expect HTTP 200 after status plus is registered manual := plugins.TriggerManual - bs := pluginStatus.New(&pluginStatus.Config{Trigger: &manual}, f.server.manager) + bs := pluginStatus.New(&pluginStatus.Config{ + Trigger: &manual, + PrometheusConfig: &pluginStatus.PrometheusConfig{ + BundleLoadDurationNanoseconds: &pluginStatus.BundleLoadDurationNanoseconds{ + Buckets: prom.ExponentialBuckets(1000, 2, 20), + }, + }, + }, f.server.manager) err := bs.Start(context.Background()) if err != nil { t.Fatal(err) @@ -3726,7 +3734,15 @@ func TestStatusV1MetricsWithSystemAuthzPolicy(t *testing.T) { // Register Status plugin manual := plugins.TriggerManual - bs := pluginStatus.New(&pluginStatus.Config{Trigger: &manual, Prometheus: true}, f.server.manager).WithMetrics(prom) + bs := pluginStatus.New(&pluginStatus.Config{ + Trigger: &manual, + Prometheus: true, + PrometheusConfig: &pluginStatus.PrometheusConfig{ + BundleLoadDurationNanoseconds: &pluginStatus.BundleLoadDurationNanoseconds{ + Buckets: []float64{1, 1000, 10_000, 1e8}, + }, + }, + }, f.server.manager).WithMetrics(prom) err := bs.Start(context.Background()) if err != nil { t.Fatal(err)