Merge 236acab into 0c15a0b

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

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"`

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")

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,12 +84,19 @@ 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 {
 		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 +149,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 +176,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.

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)
 	})