Repository: prometheus/statsd_exporter
Pull request #290 (2020-01-27 15:26:39)

allow setting granularity for summary metrics
The Go client for prometheus aggregates summary metrics over 10
minutes by default, in 5 buckets.  This is not always the behaviour we
want.

Allow tweaking those settings in `statsd_exporter`, so we can
aggregate summary metrics over more or less time, with more or fewer
buckets, and set the cap for the bucket as well.

/cc @matthiasr 

matthiasr: Can you explain more why you need to tweak this?

In general, I would rather encourage using histograms than add more features for summaries. With histograms, these decisions can be made at query time, with more options for aggregation too.
tgummerer: Sure.  This is for a metric for the response size of a request, where we sometimes get a very high response size for a few requests, and then a smaller one for others.  Sometimes there can be enough requests with a large enough size that they dominate the graph for the whole 10 minutes that the max age is set to by default, which is not what we want.

The summary metrics instead of histograms is necessary because the values don't fit into histogram buckets nicely because there is quite a big spread from the smallest to the biggest values.
matthiasr: You can add more/larger histogram buckets … but I see your point.

I'm okay with adding this, but I would like to make it clearer in the mapper config that these settings are only relevant to summaries. What do you think about moving them into a `summary_config` sub-structure? I would also like to use more descriptive keys in the YAML form.

What happens if they are not set?

Can you please add tests that include these settings? Just that they get parsed from the mapper correctly, even in mixed configurations.
tgummerer: > What do you think about moving them into a `summary_config` sub-structure? I would also like to use more descriptive keys in the YAML form.

I'm fine with moving them into a `summary_config` sub-structure, but didn't do that for now because the other settings such as quantiles that also only apply to summaries are not in a sub-structure either, so it would feel a bit odd having the new settings there.  The `buckets` setting is similar for histograms.

But if moving them into a `summary_config` substructure is your preference here I don't mind.  If we are introducing that should we allow the quantiles to be set inside that substructure as well?

> What happens if they are not set?

If they are not set, they will be assigned the default settings [here](https://github.com/prometheus/client_golang/blob/b2924667e31e7b08481e75f5ece00259ed429bf0/prometheus/summary.go#L194-L204), which matches what happens currently.

> Can you please add tests that include these settings? Just that they get parsed from the mapper correctly, even in mixed configurations.

Sure, I'll try to add some tests.
matthiasr: ah good point about the existing quantiles setting … now I'm thinking about consistency and moving those into sub structures too … the options I see are

1. keep everything flat
    1. with the current names
    2. add `summary_` prefix to the new settings
    3. `summary_` and `histogram_` prefix to new and existing settings (breaking change?)
2. move settings into substructure (breaking change?)
    1. only for summaries, keep "buckets" flat
    2. for summaries and quantiles

I am willing to break compatibility, but I don't want to do that repeatedly, so I would either keep compatibility (1.i,1.ii) or go all the way and restructure these settings all at once (2.ii). In the latter case, we could also use the presence of summary/histogram settings to decide which one the user prefers. And if we're breaking that we may as well make histograms the default …

As a user, how would you want the mapping to look?
How difficult would it be to maintain compatibility with older configs (translate into the new struct automatically, emit a warning)?
How much work are you willing to put into this?
tgummerer: Thanks for writing this down.  I can't think of any other options either.

> As a user, how would you want the mapping to look?

I think I would prefer either option `1.i` (though I'm having a hard time naming the settings tbh), and rely on the `timer_type` to tell me what the options mean, or go straight to option `2.ii`.  If we stick to option `1.i`, we could emit warnings if options that don't match the `timer_type` are given to make things safer?

> How difficult would it be to maintain compatibility with older configs (translate into the new struct automatically, emit a warning)?

Not entirely sure, but I don't think it would be too much work.  I'm happy to give that a try if that's what we decide we want to go for.  I think compatibility here is somewhat important, possibly more than whether histograms or summary is the default type, as I can't see a good transition that doesn't involve updating the config and the binary at the same type for this change (but I think it can be done for histogram vs summary switchovers).

> How much work are you willing to put into this?

Some hours I think.  I would like to do the right thing here, that we and the users are going to be happy about.
matthiasr: is updating the config and binary at the same time a problem? I was working under the assumption that that's the easy way 😅 
tgummerer: I think it might be tricky in some cases, especially if there are multiple teams involved that might have their own metrics.  I think for our usecase it might actually be possible, though not trivial.  Not sure how others manage the config/binary though.
tgummerer: I implemented option `2.ii` now, making it backwards compatible as well, with a deprecation warning.  I started off implementing option `1.i`, but then I tried implementing warnings for using the wrong options for the wrong `timer_type`, and it seemed to get hard to explain to users.  So I think separate hierarchies might actually be best.

The code could still use some cleanup, and I didn't exactly make this a nice series of commits.  Also the docs still need to be updated for the new hierarchies.  I'm happy to do all that, but would first like to get some feedback on this approach.  If you're happy with this option I'll clean it up a bit, including the docs.
matthiasr: Thank you! I will try to go through this in detail either today or tomorrow.

On Wed, Jan 29, 2020 at 5:41 PM Thomas Gummerer <notifications@github.com>
wrote:

> I implemented option 2.ii now, making it backwards compatible as well,
> with a deprecation warning. I started off implementing option 1.i, but
> then I tried implementing warnings for using the wrong options for the
> wrong timer_type, and it seemed to get hard to explain to users. So I
> think separate hierarchies might actually be best.
>
> The code could still use some cleanup, and I didn't exactly make this a
> nice series of commits. Also the docs still need to be updated for the new
> hierarchies. I'm happy to do all that, but would first like to get some
> feedback on this approach. If you're happy with this option I'll clean it
> up a bit, including the docs.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/prometheus/statsd_exporter/pull/290?email_source=notifications&email_token=AABAEBU3D355FVZRJDALNW3RAG5TDA5CNFSM4KMDFVT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKIC54Q#issuecomment-579874546>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABAEBRDJHHFZD2E4MHTHETRAG5TDANCNFSM4KMDFVTQ>
> .
>

tgummerer: @matthiasr friendly ping? I updated the docs as well now, and I'm happy to turn this into a nicer series of commits if you prefer that.
matthiasr: I'm sorry, work was a bit much this week and I didn't have enough focus to review this. I didn't forget about it!
tgummerer: No worries, totally understand that.  I just wanted to make sure it's not forgotten, but it's no problem if you need to hold of on taking a look at this for a bit longer :sparkling_heart: 

Patch:
diff --git a/README.md b/README.md
index b992e8e..830b57c 100644
--- a/README.md
+++ b/README.md
@@ -236,7 +236,8 @@ mappings:
 
 By default, statsd timers are represented as a Prometheus summary with
 quantiles. You may optionally configure the [quantiles and acceptable
-error](https://prometheus.io/docs/practices/histograms/#quantiles):
+error](https://prometheus.io/docs/practices/histograms/#quantiles), as
+well as adjusting how the summary metric is aggregated:
 
 ```yaml
 mappings:
@@ -247,19 +248,29 @@ mappings:
     provider: "$2"
     outcome: "$3"
     job: "${1}_server"
-  quantiles:
-    - quantile: 0.99
-      error: 0.001
-    - quantile: 0.95
-      error: 0.01
-    - quantile: 0.9
-      error: 0.05
-    - quantile: 0.5
-      error: 0.005
+  summary_options:
+    quantiles:
+      - quantile: 0.99
+        error: 0.001
+      - quantile: 0.95
+        error: 0.01
+      - quantile: 0.9
+        error: 0.05
+      - quantile: 0.5
+        error: 0.005
+    max_summary_age: 30s
+    summary_age_buckets: 3
+    stream_buffer_size: 1000
 ```
 
 The default quantiles are 0.99, 0.9, and 0.5.
 
+The default summary age is 10 minutes, the default number of buckets
+is 5 and the default buffer size is 500.  See also the
+[`golang_client` docs](https://godoc.org/github.com/prometheus/client_golang/prometheus#SummaryOpts).
+The `max_summary_age` corresponds to `SummaryOptions.MaxAge`, `summary_age_buckets`
+to `SummaryOptions.AgeBuckets` and `stream_buffer_size` to `SummaryOptions.BufCap`.
+
 In the configuration, one may also set the timer type to "histogram". The
 default is "summary" as in the plain text configuration format.  For example,
 to set the timer type for a single metric:
@@ -268,7 +279,8 @@ to set the timer type for a single metric:
 mappings:
 - match: "test.timing.*.*.*"
   timer_type: histogram
-  buckets: [ 0.01, 0.025, 0.05, 0.1 ]
+  histogram_options:
+    buckets: [ 0.01, 0.025, 0.05, 0.1 ]
   name: "my_timer"
   labels:
     provider: "$2"
diff --git a/pkg/mapper/mapper.go b/pkg/mapper/mapper.go
index 6496310..61814ad 100644
--- a/pkg/mapper/mapper.go
+++ b/pkg/mapper/mapper.go
@@ -57,21 +57,34 @@ type MetricMapper struct {
 }
 
 type MetricMapping struct {
-	Match           string `yaml:"match"`
-	Name            string `yaml:"name"`
-	nameFormatter   *fsm.TemplateFormatter
-	regex           *regexp.Regexp
-	Labels          prometheus.Labels `yaml:"labels"`
-	labelKeys       []string
-	labelFormatters []*fsm.TemplateFormatter
-	TimerType       TimerType         `yaml:"timer_type"`
-	Buckets         []float64         `yaml:"buckets"`
-	Quantiles       []metricObjective `yaml:"quantiles"`
-	MatchType       MatchType         `yaml:"match_type"`
-	HelpText        string            `yaml:"help"`
-	Action          ActionType        `yaml:"action"`
-	MatchMetricType MetricType        `yaml:"match_metric_type"`
-	Ttl             time.Duration     `yaml:"ttl"`
+	Match            string `yaml:"match"`
+	Name             string `yaml:"name"`
+	nameFormatter    *fsm.TemplateFormatter
+	regex            *regexp.Regexp
+	Labels           prometheus.Labels `yaml:"labels"`
+	labelKeys        []string
+	labelFormatters  []*fsm.TemplateFormatter
+	TimerType        TimerType         `yaml:"timer_type"`
+	LegacyBuckets    []float64         `yaml:"buckets"`
+	LegacyQuantiles  []metricObjective `yaml:"quantiles"`
+	MatchType        MatchType         `yaml:"match_type"`
+	HelpText         string            `yaml:"help"`
+	Action           ActionType        `yaml:"action"`
+	MatchMetricType  MetricType        `yaml:"match_metric_type"`
+	Ttl              time.Duration     `yaml:"ttl"`
+	SummaryOptions   *SummaryOptions   `yaml:"summary_options"`
+	HistogramOptions *HistogramOptions `yaml:"histogram_options"`
+}
+
+type SummaryOptions struct {
+	Quantiles  []metricObjective `yaml:"quantiles"`
+	MaxAge     time.Duration     `yaml:"max_age"`
+	AgeBuckets uint32            `yaml:"age_buckets"`
+	BufCap     uint32            `yaml:"buf_cap"`
+}
+
+type HistogramOptions struct {
+	Buckets []float64 `yaml:"buckets"`
 }
 
 type metricObjective struct {
@@ -172,12 +185,56 @@ func (m *MetricMapper) InitFromYAMLString(fileContents string, cacheSize int) er
 			currentMapping.TimerType = n.Defaults.TimerType
 		}
 
-		if currentMapping.Buckets == nil || len(currentMapping.Buckets) == 0 {
-			currentMapping.Buckets = n.Defaults.Buckets
+		if currentMapping.LegacyQuantiles != nil &&
+			(currentMapping.SummaryOptions == nil || currentMapping.SummaryOptions.Quantiles != nil) {
+			log.Warn("using the top level quantiles is deprecated.  Please use quantiles in the summary_options hierarchy")
+		}
+
+		if currentMapping.LegacyBuckets != nil &&
+			(currentMapping.HistogramOptions == nil || currentMapping.HistogramOptions.Buckets != nil) {
+			log.Warn("using the top level buckets is deprecated.  Please use buckets in the histogram_options hierarchy")
+		}
+
+		if currentMapping.SummaryOptions != nil &&
+			currentMapping.LegacyQuantiles != nil &&
+			currentMapping.SummaryOptions.Quantiles != nil {
+			return fmt.Errorf("cannot use quantiles in both the top level and summary options at the same time in %s", currentMapping.Match)
+		}
+
+		if currentMapping.HistogramOptions != nil &&
+			currentMapping.LegacyBuckets != nil &&
+			currentMapping.HistogramOptions.Buckets != nil {
+			return fmt.Errorf("cannot use buckets in both the top level and histogram options at the same time in %s", currentMapping.Match)
 		}
 
-		if currentMapping.Quantiles == nil || len(currentMapping.Quantiles) == 0 {
-			currentMapping.Quantiles = n.Defaults.Quantiles
+		if currentMapping.TimerType == TimerTypeHistogram {
+			if currentMapping.SummaryOptions != nil {
+				return fmt.Errorf("cannot use histogram timer and summary options at the same time")
+			}
+			if currentMapping.HistogramOptions == nil {
+				currentMapping.HistogramOptions = &HistogramOptions{}
+			}
+			if currentMapping.LegacyBuckets != nil && len(currentMapping.LegacyBuckets) != 0 {
+				currentMapping.HistogramOptions.Buckets = currentMapping.LegacyBuckets
+			}
+			if currentMapping.HistogramOptions.Buckets == nil || len(currentMapping.HistogramOptions.Buckets) == 0 {
+				currentMapping.HistogramOptions.Buckets = n.Defaults.Buckets
+			}
+		}
+
+		if currentMapping.TimerType == TimerTypeSummary {
+			if currentMapping.HistogramOptions != nil {
+				return fmt.Errorf("cannot use summary timer and histogram options at the same time")
+			}
+			if currentMapping.SummaryOptions == nil {
+				currentMapping.SummaryOptions = &SummaryOptions{}
+			}
+			if currentMapping.LegacyQuantiles != nil && len(currentMapping.LegacyQuantiles) != 0 {
+				currentMapping.SummaryOptions.Quantiles = currentMapping.LegacyQuantiles
+			}
+			if currentMapping.SummaryOptions.Quantiles == nil || len(currentMapping.SummaryOptions.Quantiles) == 0 {
+				currentMapping.SummaryOptions.Quantiles = n.Defaults.Quantiles
+			}
 		}
 
 		if currentMapping.Ttl == 0 && n.Defaults.Ttl > 0 {
diff --git a/registry.go b/registry.go
index df16dee..d29dbce 100644
--- a/registry.go
+++ b/registry.go
@@ -278,8 +278,8 @@ func (r *registry) getHistogram(metricName string, labels prometheus.Labels, hel
 	if vh == nil {
 		metricsCount.WithLabelValues("histogram").Inc()
 		buckets := r.mapper.Defaults.Buckets
-		if mapping.Buckets != nil && len(mapping.Buckets) > 0 {
-			buckets = mapping.Buckets
+		if mapping.HistogramOptions != nil && len(mapping.HistogramOptions.Buckets) > 0 {
+			buckets = mapping.HistogramOptions.Buckets
 		}
 		histogramVec = prometheus.NewHistogramVec(prometheus.HistogramOpts{
 			Name:    metricName,
@@ -325,8 +325,12 @@ func (r *registry) getSummary(metricName string, labels prometheus.Labels, help
 	if vh == nil {
 		metricsCount.WithLabelValues("summary").Inc()
 		quantiles := r.mapper.Defaults.Quantiles
-		if mapping != nil && mapping.Quantiles != nil && len(mapping.Quantiles) > 0 {
-			quantiles = mapping.Quantiles
+		if mapping != nil && mapping.SummaryOptions != nil && len(mapping.SummaryOptions.Quantiles) > 0 {
+			quantiles = mapping.SummaryOptions.Quantiles
+		}
+		summaryOptions := mapper.SummaryOptions{}
+		if mapping != nil && mapping.SummaryOptions != nil {
+			summaryOptions = *mapping.SummaryOptions
 		}
 		objectives := make(map[float64]float64)
 		for _, q := range quantiles {
@@ -340,6 +344,9 @@ func (r *registry) getSummary(metricName string, labels prometheus.Labels, help
 			Name:       metricName,
 			Help:       help,
 			Objectives: objectives,
+			MaxAge:     summaryOptions.MaxAge,
+			AgeBuckets: summaryOptions.AgeBuckets,
+			BufCap:     summaryOptions.BufCap,
 		}, labelNames)
 
 		if err := prometheus.Register(uncheckedCollector{summaryVec}); err != nil {


Test patch:
diff --git a/pkg/mapper/mapper_test.go b/pkg/mapper/mapper_test.go
index 3eb0d1c..dc8a45d 100644
--- a/pkg/mapper/mapper_test.go
+++ b/pkg/mapper/mapper_test.go
@@ -26,6 +26,9 @@ type mappings []struct {
 	notPresent   bool
 	ttl          time.Duration
 	metricType   MetricType
+	maxAge       time.Duration
+	ageBuckets   uint32
+	bufCap       uint32
 }
 
 func TestMetricMapperYAML(t *testing.T) {
@@ -496,6 +499,84 @@ mappings:
     `,
 			configBad: true,
 		},
+		// new style quantiles
+		{
+			config: `---
+mappings:
+- match: test.*.*
+  timer_type: summary
+  name: "foo"
+  labels: {}
+  summary_options:
+    quantiles:
+      - quantile: 0.42
+        error: 0.04
+      - quantile: 0.7
+        error: 0.002
+  `,
+			mappings: mappings{
+				{
+					statsdMetric: "test.*.*",
+					name:         "foo",
+					labels:       map[string]string{},
+					quantiles: []metricObjective{
+						{Quantile: 0.42, Error: 0.04},
+						{Quantile: 0.7, Error: 0.002},
+					},
+				},
+			},
+		},
+		// Config with summary configuration.
+		{
+			config: `---
+mappings:
+- match: test.*.*
+  timer_type: summary
+  name: "foo"
+  labels: {}
+  summary_options:
+    quantiles:
+      - quantile: 0.42
+        error: 0.04
+      - quantile: 0.7
+        error: 0.002
+    max_age: 5m
+    age_buckets: 2
+    buf_cap: 1000
+  `,
+			mappings: mappings{
+				{
+					statsdMetric: "test.*.*",
+					name:         "foo",
+					labels:       map[string]string{},
+					quantiles: []metricObjective{
+						{Quantile: 0.42, Error: 0.04},
+						{Quantile: 0.7, Error: 0.002},
+					},
+					maxAge:     5 * time.Minute,
+					ageBuckets: 2,
+					bufCap:     1000,
+				},
+			},
+		},
+		// duplicate quantiles are bad
+		{
+			config: `---
+mappings:
+- match: test.*.*
+  timer_type: summary
+  name: "foo"
+  labels: {}
+  quantiles:
+    - quantile: 0.42
+      error: 0.04
+  summary_options:
+    quantiles:
+      - quantile: 0.42
+        error: 0.04
+  `,
+			configBad: true,
+		},
 		// Config with good metric type.
 		{
 			config: `---
@@ -777,18 +858,27 @@ mappings:
 			}
 
 			if len(mapping.quantiles) != 0 {
-				if len(mapping.quantiles) != len(m.Quantiles) {
-					t.Fatalf("%d.%q: Expected %d quantiles, got %d", i, metric, len(mapping.quantiles), len(m.Quantiles))
+				if len(mapping.quantiles) != len(m.SummaryOptions.Quantiles) {
+					t.Fatalf("%d.%q: Expected %d quantiles, got %d", i, metric, len(mapping.quantiles), len(m.SummaryOptions.Quantiles))
 				}
 				for i, quantile := range mapping.quantiles {
-					if quantile.Quantile != m.Quantiles[i].Quantile {
-						t.Fatalf("%d.%q: Expected quantile %v, got %v", i, metric, m.Quantiles[i].Quantile, quantile.Quantile)
+					if quantile.Quantile != m.SummaryOptions.Quantiles[i].Quantile {
+						t.Fatalf("%d.%q: Expected quantile %v, got %v", i, metric, m.SummaryOptions.Quantiles[i].Quantile, quantile.Quantile)
 					}
-					if quantile.Error != m.Quantiles[i].Error {
-						t.Fatalf("%d.%q: Expected Error margin %v, got %v", i, metric, m.Quantiles[i].Error, quantile.Error)
+					if quantile.Error != m.SummaryOptions.Quantiles[i].Error {
+						t.Fatalf("%d.%q: Expected Error margin %v, got %v", i, metric, m.SummaryOptions.Quantiles[i].Error, quantile.Error)
 					}
 				}
 			}
+			if mapping.maxAge != 0 && mapping.maxAge != m.SummaryOptions.MaxAge {
+				t.Fatalf("%d.%q: Expected max age %v, got %v", i, metric, mapping.maxAge, m.SummaryOptions.MaxAge)
+			}
+			if mapping.ageBuckets != 0 && mapping.ageBuckets != m.SummaryOptions.AgeBuckets {
+				t.Fatalf("%d.%q: Expected max age %v, got %v", i, metric, mapping.ageBuckets, m.SummaryOptions.AgeBuckets)
+			}
+			if mapping.bufCap != 0 && mapping.bufCap != m.SummaryOptions.BufCap {
+				t.Fatalf("%d.%q: Expected max age %v, got %v", i, metric, mapping.bufCap, m.SummaryOptions.BufCap)
+			}
 		}
 	}
 }