From 6d68aab88793b9bd08e61a30d40e05b1ac43271b Mon Sep 17 00:00:00 2001 From: Yichao Yang Date: Tue, 2 Aug 2022 10:30:11 -0700 Subject: [PATCH 1/2] Fix tally timer metric --- common/metrics/config.go | 44 +++++++++++++++++++++++-- common/metrics/tally_metrics_handler.go | 13 ++------ 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/common/metrics/config.go b/common/metrics/config.go index 37aadf83603..f9f87a8f814 100644 --- a/common/metrics/config.go +++ b/common/metrics/config.go @@ -118,16 +118,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 @@ -263,12 +267,13 @@ func NewScope(logger log.Logger, c *Config) tally.Scope { return newStatsdScope(logger, c) } if c.Prometheus != nil { - return newPrometheusScope(logger, convertPrometheusConfigToTally(c.Prometheus), &c.ClientConfig) + return newPrometheusScope(logger, convertPrometheusConfigToTally(&c.ClientConfig, c.Prometheus), &c.ClientConfig) } return tally.NoopScope } func convertPrometheusConfigToTally( + clientConfig *ClientConfig, config *PrometheusConfig, ) *prometheus.Configuration { defaultObjectives := make([]prometheus.SummaryObjective, len(config.DefaultSummaryObjectives)) @@ -282,11 +287,44 @@ func convertPrometheusConfigToTally( ListenNetwork: config.ListenNetwork, ListenAddress: config.ListenAddress, TimerType: "histogram", + DefaultHistogramBuckets: buildTallyTimerHistogramBuckets(clientConfig, config), DefaultSummaryObjectives: defaultObjectives, OnError: config.OnError, } } +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 + } + + 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) { buckets := maps.Clone(defaultPerUnitHistogramBoundaries) diff --git a/common/metrics/tally_metrics_handler.go b/common/metrics/tally_metrics_handler.go index 9dc909c60e8..ad0f152fd66 100644 --- a/common/metrics/tally_metrics_handler.go +++ b/common/metrics/tally_metrics_handler.go @@ -48,16 +48,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{ @@ -93,7 +84,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(tag, tmp.excludeTags)).Histogram(timer, tmp.perUnitBuckets[Milliseconds]).RecordDuration(d) + tmp.scope.Tagged(tagsToMap(tag, tmp.excludeTags)).Timer(timer).Record(d) }) } From a6db3da7678027c7e382b3dec1d8858e8bea9e3a Mon Sep 17 00:00:00 2001 From: Yichao Yang Date: Tue, 2 Aug 2022 11:30:50 -0700 Subject: [PATCH 2/2] fix tests --- common/metrics/tally_metrics_handler_test.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/common/metrics/tally_metrics_handler_test.go b/common/metrics/tally_metrics_handler_test.go index de1aa3e47e8..c42df95751a 100644 --- a/common/metrics/tally_metrics_handler_test.go +++ b/common/metrics/tally_metrics_handler_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{