Fix tally timer metric (#3173)

common/metrics/config.go

@@ -120,16 +120,20 @@ type (
 		TimerType string `yaml:"timerType"`
 
 		// Deprecated. Please use PerUnitHistogramBoundaries in ClientConfig.
-		// DefaultHistogramBoundaries defines the default histogram bucket boundaries.
+		// DefaultHistogramBoundaries defines the default histogram bucket boundaries for tally timer metrics.
 		DefaultHistogramBoundaries []float64 `yaml:"defaultHistogramBoundaries"`
 
 		// Deprecated. Please use PerUnitHistogramBoundaries in ClientConfig.
 		// DefaultHistogramBuckets if specified will set the default histogram
-		// buckets to be used by the reporter.
+		// buckets to be used by the reporter for tally timer metrics.
+		// The unit for value specified is Second.
+		// If specified, will override DefaultSummaryObjectives and PerUnitHistogramBoundaries["milliseconds"].
 		DefaultHistogramBuckets []HistogramObjective `yaml:"defaultHistogramBuckets"`
 
 		// Deprecated. DefaultSummaryObjectives if specified will set the default summary
 		// objectives to be used by the reporter.
+		// The unit for value specified is Second.
+		// If specified, will override PerUnitHistogramBoundaries["milliseconds"].
 		DefaultSummaryObjectives []SummaryObjective `yaml:"defaultSummaryObjectives"`
 
 		// Deprecated. OnError specifies what to do when an error either with listening
@@ -297,15 +301,24 @@ func NewScope(logger log.Logger, c *Config) tally.Scope {
 
 		return newPrometheusScope(
 			logger,
-			convertPrometheusConfigToTally(c.Prometheus),
+			convertPrometheusConfigToTally(&c.ClientConfig, c.Prometheus),
 			sanitizeOptions,
 			&c.ClientConfig,
 		)
 	}
 	return tally.NoopScope
 }
 
+func convertSanitizeOptionsToTally(config *PrometheusConfig) (tally.SanitizeOptions, error) {
+	if config.SanitizeOptions == nil {
+		return defaultTallySanitizeOptions, nil
+	}
+
+	return config.SanitizeOptions.toTally()
+}
+
 func convertPrometheusConfigToTally(
+	clientConfig *ClientConfig,
 	config *PrometheusConfig,
 ) *prometheus.Configuration {
 	defaultObjectives := make([]prometheus.SummaryObjective, len(config.DefaultSummaryObjectives))
@@ -319,17 +332,42 @@ func convertPrometheusConfigToTally(
 		ListenNetwork:            config.ListenNetwork,
 		ListenAddress:            config.ListenAddress,
 		TimerType:                "histogram",
+		DefaultHistogramBuckets:  buildTallyTimerHistogramBuckets(clientConfig, config),
 		DefaultSummaryObjectives: defaultObjectives,
 		OnError:                  config.OnError,
 	}
 }
 
-func convertSanitizeOptionsToTally(config *PrometheusConfig) (tally.SanitizeOptions, error) {
-	if config.SanitizeOptions == nil {
-		return defaultTallySanitizeOptions, nil
+func buildTallyTimerHistogramBuckets(
+	clientConfig *ClientConfig,
+	config *PrometheusConfig,
+) []prometheus.HistogramObjective {
+	if len(config.DefaultHistogramBuckets) > 0 {
+		result := make([]prometheus.HistogramObjective, len(config.DefaultHistogramBuckets))
+		for i, item := range config.DefaultHistogramBuckets {
+			result[i].Upper = item.Upper
+		}
+		return result
 	}
 
-	return config.SanitizeOptions.toTally()
+	if len(config.DefaultHistogramBoundaries) > 0 {
+		result := make([]prometheus.HistogramObjective, 0, len(config.DefaultHistogramBoundaries))
+		for _, value := range config.DefaultHistogramBoundaries {
+			result = append(result, prometheus.HistogramObjective{
+				Upper: value,
+			})
+		}
+		return result
+	}
+
+	boundaries := clientConfig.PerUnitHistogramBoundaries[Milliseconds]
+	result := make([]prometheus.HistogramObjective, 0, len(boundaries))
+	for _, boundary := range boundaries {
+		result = append(result, prometheus.HistogramObjective{
+			Upper: boundary / float64(time.Second/time.Millisecond), // convert milliseconds to seconds
+		})
+	}
+	return result
 }
 
 func setDefaultPerUnitHistogramBoundaries(clientConfig *ClientConfig) {

common/metrics/tally_metric_provider.go

@@ -49,16 +49,7 @@ func NewTallyMetricsHandler(cfg ClientConfig, scope tally.Scope) *tallyMetricsHa
 	perUnitBuckets := make(map[MetricUnit]tally.Buckets)
 
 	for unit, boundariesList := range cfg.PerUnitHistogramBoundaries {
-		if unit != Milliseconds {
-			perUnitBuckets[MetricUnit(unit)] = tally.ValueBuckets(boundariesList)
-			continue
-		}
-
-		durations := make([]time.Duration, 0, len(boundariesList))
-		for _, duration := range boundariesList {
-			durations = append(durations, time.Duration(duration)*time.Millisecond)
-		}
-		perUnitBuckets[MetricUnit(unit)] = tally.DurationBuckets(durations)
+		perUnitBuckets[MetricUnit(unit)] = tally.ValueBuckets(boundariesList)
 	}
 
 	return &tallyMetricsHandler{
@@ -95,7 +86,7 @@ func (tmp *tallyMetricsHandler) Gauge(gauge string) GaugeMetric {
 // Timer obtains a timer for the given name and MetricOptions.
 func (tmp *tallyMetricsHandler) Timer(timer string) TimerMetric {
 	return TimerMetricFunc(func(d time.Duration, tag ...Tag) {
-		tmp.scope.Tagged(tagsToMap(tmp.tags, tag, tmp.excludeTags)).Histogram(timer, tmp.perUnitBuckets[Milliseconds]).RecordDuration(d)
+		tmp.scope.Tagged(tagsToMap(tmp.tags, tag, tmp.excludeTags)).Timer(timer).Record(d)
 	})
 }
 

common/metrics/tally_metric_provider_test.go

@@ -50,7 +50,7 @@ func TestTallyScope(t *testing.T) {
 	recordTallyMetrics(mp)
 
 	snap := scope.Snapshot()
-	counters, gauges, timers, histograms := snap.Counters(), snap.Gauges(), snap.Histograms(), snap.Histograms()
+	counters, gauges, timers, histograms := snap.Counters(), snap.Gauges(), snap.Timers(), snap.Histograms()
 
 	assert.EqualValues(t, 8, counters["test.hits+"].Value())
 	assert.EqualValues(t, map[string]string{}, counters["test.hits+"].Tags())
@@ -66,14 +66,10 @@ func TestTallyScope(t *testing.T) {
 		"location": "Mare Imbrium",
 	}, gauges["test.temp+location=Mare Imbrium"].Tags())
 
-	assert.EqualValues(t, map[time.Duration]int64{
-		10 * time.Millisecond:    0,
-		500 * time.Millisecond:   0,
-		1000 * time.Millisecond:  0,
-		5000 * time.Millisecond:  1,
-		10000 * time.Millisecond: 1,
-		math.MaxInt64:            0,
-	}, timers["test.latency+"].Durations())
+	assert.EqualValues(t, []time.Duration{
+		1248 * time.Millisecond,
+		5255 * time.Millisecond,
+	}, timers["test.latency+"].Values())
 	assert.EqualValues(t, map[string]string{}, timers["test.latency+"].Tags())
 
 	assert.EqualValues(t, map[float64]int64{