From 1afc3ac933498c6724925deae02155c2d7c5beae Mon Sep 17 00:00:00 2001 From: Oleg Zaytsev Date: Thu, 9 Jan 2020 14:53:32 +0100 Subject: [PATCH 1/2] Summaries: require objectives to be explicit Just like https://github.com/cabify/gotoprom/pull/27 but for Summaries, same reasons apply, TL;DR: explicit is better than implicit. Also fixed summary builder not failing when objectives were malformed. --- CHANGELOG.md | 5 +++++ README.md | 2 +- metrics_test.go | 4 ++-- prometheusvanilla/builders.go | 25 +++++++++++++++++-------- prometheusvanilla/builders_test.go | 23 +++++++++++++++++------ 5 files changed, 42 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fe4d6a..40220e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added - Support for empty buckets tag, which will generate nil buckets for the prometheus Histogram and use default prometheus buckets +- Support for empty objectives tag, which will generate nil objectives for the prometheus Summary and use an empty objectives map after all. ### Changed - *Breaking*: `prometheus.Histogram` is now used to build histograms, instead of `prometheus.Observer`, which means that previous code building `prometheus.Observer` won't compile anymore. ### Removed - *Breaking*: default buckets on histograms. All histogram should explicitly specify their buckets now or they will fail to build. +- *Breaking*: default objectives on summaries. All summaries should explicitly specify their objectives now or they will fail to build. + +### Fixed +- Summary building was not failing with malformed objectives ## [0.3.0] - 2019-10-10 ### Added diff --git a/README.md b/README.md index a6766bf..b5d74c8 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ var metrics struct { SomeHistogram func() prometheus.Histogram `name:"some_histogram" help:"Some histogram with default prometheus buckets" buckets:""` SomeHistogramWithSpecificBuckets func() prometheus.Histogram `name:"some_histogram_with_buckets" help:"Some histogram with custom buckets" buckets:".01,.05,.1"` SomeGauge func() prometheus.Gauge `name:"some_gauge" help:"Some gauge"` - SomeSummaryWithSpecificMaxAge func() prometheus.Summary `name:"some_summary_with_specific_max_age" help:"Some summary with custom max age" max_age:"20m"` + SomeSummaryWithSpecificMaxAge func() prometheus.Summary `name:"some_summary_with_specific_max_age" help:"Some summary with custom max age" max_age:"20m" objectives:"0.50,0.95,0.99"` Requests struct { Total func(requestLabels) prometheus.Count `name:"total" help:"Total amount of requests served"` diff --git a/metrics_test.go b/metrics_test.go index bb9663d..779c3fd 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -24,7 +24,7 @@ func Test_InitHappyCase(t *testing.T) { HTTPRequestTime func(labels) prometheus.Histogram `name:"http_request_count" help:"Time taken to serve a HTTP request" buckets:"0.001,0.005,0.01,0.05,0.1,0.5,1,5,10"` DuvelsEmptied func(labels) prometheus.Counter `name:"duvels_emptied" help:"Delirium floor sweep count"` RubberDuckInTherapy func(labels) prometheus.Gauge `name:"rubber_ducks_in_therapy" help:"Number of rubber ducks who need help after some intense coding"` - BrokenDeploysAccomplished func(labels) prometheus.Summary `name:"broken_deploys_accomplished" help:"Number of deploys that broke production"` + BrokenDeploysAccomplished func(labels) prometheus.Summary `name:"broken_deploys_accomplished" help:"Number of deploys that broke production" objectives:"0.5,0.9,0.99"` NoLabels func() prometheus.Counter `name:"no_labels" help:"Metric without labels"` } @@ -284,7 +284,7 @@ func Test_SummaryWithSpecifiedMaxAge(t *testing.T) { summaryHelp := "Uses default value for max age" var metrics struct { - Summary func() prometheus.Summary `name:"without_max_age" help:"Uses default value for max age" max_age:"1s"` + Summary func() prometheus.Summary `name:"without_max_age" help:"Uses default value for max age" max_age:"1s" objectives:".5,.9,.99"` } err := gotoprom.Init(&metrics, "test") diff --git a/prometheusvanilla/builders.go b/prometheusvanilla/builders.go index 83e7122..9a27c38 100644 --- a/prometheusvanilla/builders.go +++ b/prometheusvanilla/builders.go @@ -87,6 +87,10 @@ func BuildSummary(name, help, namespace string, labelNames []string, tag reflect return nil, nil, fmt.Errorf("build summary %q: %s", name, err) } objectives, err := objectivesFromTag(tag) + if err != nil { + return nil, nil, fmt.Errorf("build summary %q: %s", name, err) + } + sum := prometheus.NewSummaryVec( prometheus.SummaryOpts{ Name: name, @@ -139,15 +143,25 @@ func maxAgeFromTag(tag reflect.StructTag) (time.Duration, error) { return maxAgeDuration, nil } +// objectivesFromTag will return the objectives from the tag provided +// if there's no objectives tag, it will return an error +// if objectives is an empty string, it will return a nil value instead of an initialized empty map +// this is intended to initialize prometheus metric with default values, as prometheus will +// check for the value to be nil instead of checking for its len to be 0 (like it does for buckets) func objectivesFromTag(tag reflect.StructTag) (map[float64]float64, error) { quantileString, ok := tag.Lookup("objectives") if !ok { - return DefaultObjectives(), nil + return nil, fmt.Errorf("objectives not specified") + } + + if len(quantileString) == 0 { + return nil, nil } + quantileSlice := strings.Split(quantileString, ",") objectives := make(map[float64]float64, len(quantileSlice)) - for _, quantile := range quantileSlice { - q, err := strconv.ParseFloat(quantile, 64) + for i := range quantileSlice { + q, err := strconv.ParseFloat(quantileSlice[i], 64) if err != nil { return nil, fmt.Errorf("invalid objective specified: %s", err) } @@ -156,11 +170,6 @@ func objectivesFromTag(tag reflect.StructTag) (map[float64]float64, error) { return objectives, nil } -// DefaultObjectives provides a list of objectives you can use when you don't know what to use yet. -func DefaultObjectives() map[float64]float64 { - return map[float64]float64{0.5: 0.05, 0.9: 0.01, 0.99: 0.001} -} - // absError will calculate the absolute error for a given objective up to 3 decimal places. // The variance is calculated based on the given quantile. // Values in lower quantiles have a higher probability of being similar, so we can apply greater variances. diff --git a/prometheusvanilla/builders_test.go b/prometheusvanilla/builders_test.go index 881db6f..aa10678 100644 --- a/prometheusvanilla/builders_test.go +++ b/prometheusvanilla/builders_test.go @@ -27,7 +27,8 @@ var ( maxAgeTag reflect.StructTag = `name:"some_name" help:"some help for the metric" max_age:"1h"` objectivesTag reflect.StructTag = `name:"some_name" help:"some help for the metric" max_age:"1h" objectives:"0.55,0.95,0.98"` - objectivesMalformedTag reflect.StructTag = `name:"some_name" help:"some help for the metric" max_age:"1h" objectives:"notFloat"` + emptyObjectivesTag reflect.StructTag = `name:"some_name" help:"some help for the metric" max_age:"1h" objectives:""` + malformedObjectivesTag reflect.StructTag = `name:"some_name" help:"some help for the metric" max_age:"1h" objectives:"notFloat"` expectedBuckets = []float64{0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10} expectedMaxAge = time.Hour @@ -64,7 +65,7 @@ func TestBuilders(t *testing.T) { }) t.Run("Test building a summary", func(t *testing.T) { - f, c, err := BuildSummary(name, help, nameSpace, keys, "") + f, c, err := BuildSummary(name, help, nameSpace, keys, `objectives:""`) assert.NoError(t, err) assert.Implements(t, (*prometheus.Collector)(nil), c) assert.Implements(t, (*prometheus.Summary)(nil), f(labels)) @@ -74,6 +75,16 @@ func TestBuilders(t *testing.T) { _, _, err := BuildSummary(name, help, nameSpace, keys, `max_age:"one year" objectives:"0.1,0.25"`) assert.Error(t, err) }) + + t.Run("Test building a summary without objectives", func(t *testing.T) { + _, _, err := BuildSummary(name, help, nameSpace, keys, "") + assert.Error(t, err) + }) + + t.Run("Test building a summary with malformed objectives", func(t *testing.T) { + _, _, err := BuildSummary(name, help, nameSpace, keys, `objectives:"."`) + assert.Error(t, err) + }) } func TestBuckets(t *testing.T) { @@ -119,13 +130,13 @@ func TestObjectives(t *testing.T) { assert.NoError(t, err) assert.Equal(t, expectedObjectives, obj) }) - t.Run("Test returning default objective values when none are specified", func(t *testing.T) { - obj, err := objectivesFromTag(defaultTag) + t.Run("Test parsing empty from tag", func(t *testing.T) { + obj, err := objectivesFromTag(emptyObjectivesTag) assert.NoError(t, err) - assert.Equal(t, DefaultObjectives(), obj) + assert.Equal(t, map[float64]float64(nil), obj) }) t.Run("Test returning default objective values when none are specified", func(t *testing.T) { - obj, err := objectivesFromTag(objectivesMalformedTag) + obj, err := objectivesFromTag(malformedObjectivesTag) assert.Error(t, err) assert.Nil(t, obj) }) From 236acabe4cbff3c1da8215ea5b6c2bcf8c86127a Mon Sep 17 00:00:00 2001 From: Oleg Zaytsev Date: Thu, 9 Jan 2020 15:21:48 +0100 Subject: [PATCH 2/2] Document Histogram&Summary builders tags behavior --- prometheusvanilla/builders.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/prometheusvanilla/builders.go b/prometheusvanilla/builders.go index 9a27c38..390e639 100644 --- a/prometheusvanilla/builders.go +++ b/prometheusvanilla/builders.go @@ -58,6 +58,9 @@ func BuildGauge(name, help, namespace string, labelNames []string, tag reflect.S // BuildHistogram builds a prometheus.Histogram // The function it returns returns a prometheus.Histogram type as an interface{} +// It requires the buckets tag to be provided +// If the buckets tag is explicitly empty, then the Histogram will be built with default prometheus buckets +// which is prometheus.DefBuckets at the time this comment is written. func BuildHistogram(name, help, namespace string, labelNames []string, tag reflect.StructTag) (func(prometheus.Labels) interface{}, prometheus.Collector, error) { buckets, err := bucketsFromTag(tag) if err != nil { @@ -81,6 +84,9 @@ func BuildHistogram(name, help, namespace string, labelNames []string, tag refle // BuildSummary builds a prometheus.Summary // The function it returns returns a prometheus.Summary type as an interface{} +// It requires the objectives tag to be provided, and optionally the max_age tag +// If the objectives tag is explicitly empty, then the Summary will be built with default prometheus objectives +// which is no objectives at the time this comment is written. func BuildSummary(name, help, namespace string, labelNames []string, tag reflect.StructTag) (func(prometheus.Labels) interface{}, prometheus.Collector, error) { maxAge, err := maxAgeFromTag(tag) if err != nil {