Review comment fixes

Signed-off-by: Tyler Reid <[email protected]>

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Changelog
 
 ## master / unreleased
+* [FEATURE] Ruler: Add new `-ruler.query-stats-enabled` which when enabled will report the `cortex_ruler_query_seconds_total` metric that tracks the sum of the wall time of executing queries in the ruler in seconds. #4317
 
 ## 1.10.0-rc.0 / 2021-06-28
 
@@ -26,7 +27,6 @@
   * Ensure that a ring store is configured using `-alertmanager.sharding-ring.store`, and set the flags relevant to the chosen store type.
   * Enable the feature using `-alertmanager.sharding-enabled`.
   * Note the prior addition of a new configuration option `-alertmanager.persist-interval`. This sets the interval between persisting the current alertmanager state (notification log and silences) to object storage. See the [configuration file reference](https://cortexmetrics.io/docs/configuration/configuration-file/#alertmanager_config) for more information.
-* [FEATURE] Ruler: Add new `-ruler.enable-query-stats` which when enabled will report the `cortex_ruler_query_seconds_total` metric that tracks the sum of the wall time of executing queries in the ruler in seconds. #4317
 * [ENHANCEMENT] Alertmanager: Cleanup persisted state objects from remote storage when a tenant configuration is deleted. #4167
 * [ENHANCEMENT] Storage: Added the ability to disable Open Census within GCS client (e.g `-gcs.enable-opencensus=false`). #4219
 * [ENHANCEMENT] Etcd: Added username and password to etcd config. #4205

pkg/ruler/compat.go

@@ -147,11 +147,10 @@ func MetricsQueryFunc(qf rules.QueryFunc, queries, failedQueries prometheus.Coun
 	return func(ctx context.Context, qs string, t time.Time) (promql.Vector, error) {
 		queries.Inc()
 
-		var startTime time.Time
 		// If we've been passed a counter vec we want to record the wall time spent executing this request.
 		if queryTime != nil {
-			startTime = time.Now()
-			defer func() { queryTime.WithLabelValues(userID).Add(time.Since(startTime).Seconds()) }()
+			timer := prometheus.NewTimer(nil)
+			defer func() { queryTime.WithLabelValues(userID).Add(timer.ObserveDuration().Seconds()) }()
 		}
 
 		result, err := qf(ctx, qs, t)
@@ -207,10 +206,10 @@ func DefaultTenantManagerFactory(cfg Config, p Pusher, q storage.Queryable, engi
 		Help: "Number of failed queries by ruler.",
 	})
 	var rulerQuerySeconds *prometheus.CounterVec
-	if cfg.RulerEnableQueryStats {
+	if cfg.EnableQueryStats {
 		rulerQuerySeconds = promauto.With(reg).NewCounterVec(prometheus.CounterOpts{
 			Name: "cortex_ruler_query_seconds_total",
-			Help: "Total amount of wall clock time spend processing queries by the ruler.",
+			Help: "Total amount of wall clock time spent processing queries by the ruler.",
 		}, []string{"user"})
 	}
 

pkg/ruler/ruler.go

@@ -116,7 +116,7 @@ type Config struct {
 
 	RingCheckPeriod time.Duration `yaml:"-"`
 
-	RulerEnableQueryStats bool `yaml:"enable_query_stats"`
+	EnableQueryStats bool `yaml:"query_stats_enabled"`
 }
 
 // Validate config and returns error on failure
@@ -175,7 +175,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.Var(&cfg.EnabledTenants, "ruler.enabled-tenants", "Comma separated list of tenants whose rules this ruler can evaluate. If specified, only these tenants will be handled by ruler, otherwise this ruler can process rules from all tenants. Subject to sharding.")
 	f.Var(&cfg.DisabledTenants, "ruler.disabled-tenants", "Comma separated list of tenants whose rules this ruler cannot evaluate. If specified, a ruler that would normally pick the specified tenant(s) for processing will ignore them instead. Subject to sharding.")
 
-	f.BoolVar(&cfg.RulerEnableQueryStats, "ruler.enable-query-stats", false, "Report the wall time for ruler queries to complete as a metric.")
+	f.BoolVar(&cfg.EnableQueryStats, "ruler.query-stats-enabled", false, "Report the wall time for ruler queries to complete as a metric.")
 
 	cfg.RingCheckPeriod = 5 * time.Second
 }

pkg/ruler/ruler_test.go

@@ -60,7 +60,7 @@ func defaultRulerConfig(store rulestore.RuleStore) (Config, func()) {
 	cfg.Ring.ListenPort = 0
 	cfg.Ring.InstanceAddr = "localhost"
 	cfg.Ring.InstanceID = "localhost"
-	cfg.RulerEnableQueryStats = false
+	cfg.EnableQueryStats = false
 
 	// Create a cleanup function that will be called at the end of the test
 	cleanup := func() {