From 084f0af45890ac542efb278fdcb11eb19f78d664 Mon Sep 17 00:00:00 2001
From: Yang Song <songy23@users.noreply.github.com>
Date: Tue, 12 Mar 2019 23:17:56 -0700
Subject: [PATCH] Exporter/Prometheus: Simplify histogram creation. (#1061)

* Exporter/Prometheus: Simplify histogram creation.

* Add a test on unordered bucket bounds.

* Fix review comments.
---
 exporter/prometheus/prometheus.go      | 21 +-----
 exporter/prometheus/prometheus_test.go | 89 +++++++++++++++++++++++++-
 2 files changed, 90 insertions(+), 20 deletions(-)

diff --git a/exporter/prometheus/prometheus.go b/exporter/prometheus/prometheus.go
index 968409816..203bd38ad 100644
--- a/exporter/prometheus/prometheus.go
+++ b/exporter/prometheus/prometheus.go
@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"log"
 	"net/http"
-	"sort"
 	"sync"
 
 	"go.opencensus.io/internal"
@@ -213,25 +212,9 @@ func (c *collector) toMetric(desc *prometheus.Desc, v *view.View, row *view.Row)
 	case *view.DistributionData:
 		points := make(map[float64]uint64)
 		// Histograms are cumulative in Prometheus.
-		// 1. Sort buckets in ascending order but, retain
-		// their indices for reverse lookup later on.
-		// TODO: If there is a guarantee that distribution elements
-		// are always sorted, then skip the sorting.
-		indicesMap := make(map[float64]int)
-		buckets := make([]float64, 0, len(v.Aggregation.Buckets))
-		for i, b := range v.Aggregation.Buckets {
-			if _, ok := indicesMap[b]; !ok {
-				indicesMap[b] = i
-				buckets = append(buckets, b)
-			}
-		}
-		sort.Float64s(buckets)
-
-		// 2. Now that the buckets are sorted by magnitude
-		// we can create cumulative indicesmap them back by reverse index
+		// Get cumulative bucket counts.
 		cumCount := uint64(0)
-		for _, b := range buckets {
-			i := indicesMap[b]
+		for i, b := range v.Aggregation.Buckets {
 			cumCount += uint64(data.CountPerBucket[i])
 			points[b] = cumCount
 		}
diff --git a/exporter/prometheus/prometheus_test.go b/exporter/prometheus/prometheus_test.go
index 67b05f020..4042e68b3 100644
--- a/exporter/prometheus/prometheus_test.go
+++ b/exporter/prometheus/prometheus_test.go
@@ -288,7 +288,94 @@ func TestCumulativenessFromHistograms(t *testing.T) {
 	// We want the results that look like this:
 	// 1:   [0.25]      		| 1 + prev(i) = 1 + 0 = 1
 	// 5:   [1.45]			| 1 + prev(i) = 1 + 1 = 2
-	// 10:	[]			| 1 + prev(i) = 1 + 2 = 3
+	// 10:	[7.69]			| 1 + prev(i) = 1 + 2 = 3
+	// 20:  [12]			| 1 + prev(i) = 1 + 3 = 4
+	// 50:  []			| 0 + prev(i) = 0 + 4 = 4
+	// 100: []			| 0 + prev(i) = 0 + 4 = 4
+	// 250: [187.12, 199.9, 245.67]	| 3 + prev(i) = 3 + 4 = 7
+	wantLines := []string{
+		`cash_register_bucket{le="1"} 1`,
+		`cash_register_bucket{le="5"} 2`,
+		`cash_register_bucket{le="10"} 3`,
+		`cash_register_bucket{le="20"} 4`,
+		`cash_register_bucket{le="50"} 4`,
+		`cash_register_bucket{le="100"} 4`,
+		`cash_register_bucket{le="250"} 7`,
+		`cash_register_bucket{le="+Inf"} 7`,
+		`cash_register_sum 654.0799999999999`, // Summation of the input values
+		`cash_register_count 7`,
+	}
+
+	ctx := context.Background()
+	ms := make([]stats.Measurement, 0, len(values))
+	for _, value := range values {
+		mx := m.M(value)
+		ms = append(ms, mx)
+	}
+	stats.Record(ctx, ms...)
+
+	// Give the recorder ample time to process recording
+	<-time.After(10 * reportPeriod)
+
+	cst := httptest.NewServer(exporter)
+	defer cst.Close()
+	res, err := http.Get(cst.URL)
+	if err != nil {
+		t.Fatalf("http.Get error: %v", err)
+	}
+	blob, err := ioutil.ReadAll(res.Body)
+	if err != nil {
+		t.Fatalf("Read body error: %v", err)
+	}
+	str := strings.Trim(string(blob), "\n")
+	lines := strings.Split(str, "\n")
+	nonComments := make([]string, 0, len(lines))
+	for _, line := range lines {
+		if !strings.Contains(line, "#") {
+			nonComments = append(nonComments, line)
+		}
+	}
+
+	got := strings.Join(nonComments, "\n")
+	want := strings.Join(wantLines, "\n")
+	if got != want {
+		t.Fatalf("\ngot:\n%s\n\nwant:\n%s\n", got, want)
+	}
+}
+
+func TestHistogramUnorderedBucketBounds(t *testing.T) {
+	exporter, err := NewExporter(Options{})
+	if err != nil {
+		t.Fatalf("failed to create prometheus exporter: %v", err)
+	}
+	view.RegisterExporter(exporter)
+	reportPeriod := time.Millisecond
+	view.SetReportingPeriod(reportPeriod)
+
+	m := stats.Float64("tests/bills", "payments by denomination", stats.UnitDimensionless)
+	v := &view.View{
+		Name:        "cash/register",
+		Description: "this is a test",
+		Measure:     m,
+
+		// Intentionally used unordered and duplicated elements in the distribution
+		// to ensure unordered bucket bounds are handled.
+		Aggregation: view.Distribution(10, 5, 1, 1, 50, 5, 20, 100, 250),
+	}
+
+	if err := view.Register(v); err != nil {
+		t.Fatalf("Register error: %v", err)
+	}
+	defer view.Unregister(v)
+
+	// Give the reporter ample time to process registration
+	<-time.After(10 * reportPeriod)
+
+	values := []float64{0.25, 245.67, 12, 1.45, 199.9, 7.69, 187.12}
+	// We want the results that look like this:
+	// 1:   [0.25]      		| 1 + prev(i) = 1 + 0 = 1
+	// 5:   [1.45]			| 1 + prev(i) = 1 + 1 = 2
+	// 10:	[7.69]			| 1 + prev(i) = 1 + 2 = 3
 	// 20:  [12]			| 1 + prev(i) = 1 + 3 = 4
 	// 50:  []			| 0 + prev(i) = 0 + 4 = 4
 	// 100: []			| 0 + prev(i) = 0 + 4 = 4