Repository: census-instrumentation/opencensus-go (go)

Issue:
Including 0 bound when creating distribution leads to mismatch in bucket count between view and rows of actual data.
Please answer these questions before submitting a bug report.

### What version of OpenCensus are you using?

0.23.0


### What version of Go are you using?

1.13

### What did you do?
Created a custom exporter that handles views of type Distribution; when creating the distribution I included 0 as a bound.

### What did you expect to see?

The number of buckets in the view and the number of buckets in the row of actual data match up

### What did you see instead?

The view and the row data do not match up:
`len(vd.View.Aggregation.Buckets)` = 10
and `len(vd.Rows[0].Data.CountPerBucket)` = 12

### Additional context
Wall of text for you!

My team is working on a custom exporter for metrics.  I'm encountering an issue specifically with `Views` of type `Distribution`.  What I am encountering is that if I create a view of type Distribution where the bounds includes 0, when that view gets registered the 0 bound is silently dropped.  However, when recording the data, that bucket is NOT dropped, leading to a mis-match in the view vs row that arrives in the `view.Data` that is passed to my `ExportView` function.

I'll post all of the details of my exploration below, but the main questions I have are: 

1.  is this a bug? or 
2.  if this is expected behavior, can it be clearly documented that you should NOT use 0 bounds for Distributions?  
  -  The example [here](https://opencensus.io/quickstart/go/metrics/) shows Distributions including a 0 bound: 

```
Aggregation: view.Distribution(0, 25, 50, 75, 100, 200, 400, 600, 800, 1000, 2000, 4000, 6000),
```


Here are the detailed steps of what I am finding.

My main program is creating Distribution View:
```
var (
	values = stats.Int64("balls", "measure of balls", "balls")

	distributionView = &view.View{
		Measure:     values,
		Name:        "distribution",
		Description: "a distribution of the values",
		Aggregation: view.Distribution(0, 10, 20, 30, 40, 50, 60, 70, 80, 90, 100),
	}
)
```

When `view.Distribution(...)` is called, it is ending up in the code here: https://github.com/census-instrumentation/opencensus-go/blob/master/stats/view/aggregation.go#L106
```
	return &Aggregation{
		Type:    AggTypeDistribution,
		Buckets: bounds,
		newData: func() AggregationData {
			return newDistributionData(bounds)
		},
	}
```
When that `newData` function is assinged, the value of `len(bounds)` is 11, as one would expect.

Next in my main program, I register the view:
```
	if err := view.Register(distributionView); err != nil {
		panic(err)
	}
```

In this step, when `view.Register(...)` is called, a `registerViewReq` is created.  When it is handled, the view has `canonicalize()` called on it, which eventually gets to this line: https://github.com/census-instrumentation/opencensus-go/blob/master/stats/view/view.go#L104
```
	// drop 0 bucket silently.
	v.Aggregation.Buckets = dropZeroBounds(v.Aggregation.Buckets...)
```
After this code executes, the length of v.Aggregation.Buckets is now 10, instead of 11.


Okay, now everything is registered and ready to go; next we record our metric:
```
	stats.Record(context.Background(), values.M(rand.Int63n(100)))
```

Following this down the rabbit hole, we end up in `collector.go`, specifically here: https://github.com/census-instrumentation/opencensus-go/blob/master/stats/view/collector.go#L38
```
	aggregator, ok := c.signatures[s]
	if !ok {
		aggregator = c.a.newData()
		c.signatures[s] = aggregator
	}
```
The first time through, `c.a.newData()` gets called, which returns the function that was defined up above - when `len(bounds)` was equal to 11.

Stepping into `newDistributionData()` https://github.com/census-instrumentation/opencensus-go/blob/master/stats/view/aggregation_data.go#L132
```
func newDistributionData(bounds []float64) *DistributionData {
	bucketCount := len(bounds) + 1
	return &DistributionData{
	...
```
Here, the bucket count gets set to 12 (11+1)

Now, finally, we make it to our ExportView function that we wrote:
```
func (e *Exporter) ExportView(vd *view.Data) {
	for _, row := range vd.Rows {
		e.processRow(vd, row)
	}
}
```
At this point, 
- `len(vd.View.Aggregation.Buckets)` = 10 (11 - 1 because it dropped the 0 bounds)
- `len(row.Data.CountPerBucket)` = 12 (11 + 1 as defined in `newDistributionData`)

Note: as the two things I am comparing are both referring to the "buckets", I would expect them to be equal. I can see how, since `vd.View.Aggregation.Buckets` is actually referring to the _bounds_, not the buckets, that in that case it would be expected that it be one less than the number of buckets in `row.Data.CountPerBucket`, as described [here](https://godoc.org/go.opencensus.io/stats/view#DistributionData):

> For a distribution with N bounds, the associated DistributionData will have N+1 buckets.

But in that case, I feel like that should be referred to as `vd.View.Aggregation.Bounds` instead of `Buckets`. 

At any rate, even taking into account that this is comparing bounds to buckets, we still have a mismatch - it should be either 11 and 12, if the 0 bound is not dropped for either, or 10 and 11, if the 0 bound is dropped for both.

Pull request description:
exclude zero bucket from aggregation_data
fixes #1169 

Patch:
diff --git a/go.mod b/go.mod
index 139157c..c867df5 100644
--- a/go.mod
+++ b/go.mod
@@ -8,7 +8,6 @@ require (
 	golang.org/x/net v0.0.0-20190620200207-3b0461eec859
 	golang.org/x/sys v0.0.0-20190502145724-3ef323f4f1fd // indirect
 	golang.org/x/text v0.3.2 // indirect
-	google.golang.org/appengine v1.4.0 // indirect
 	google.golang.org/genproto v0.0.0-20190425155659-357c62f0e4bb // indirect
 	google.golang.org/grpc v1.20.1
 )
diff --git a/stats/view/aggregation.go b/stats/view/aggregation.go
index 8bd2531..9d70937 100644
--- a/stats/view/aggregation.go
+++ b/stats/view/aggregation.go
@@ -99,13 +99,14 @@ func Sum() *Aggregation {
 // If len(bounds) is 1 then there is no finite buckets, and that single
 // element is the common boundary of the overflow and underflow buckets.
 func Distribution(bounds ...float64) *Aggregation {
-	return &Aggregation{
+	agg := &Aggregation{
 		Type:    AggTypeDistribution,
 		Buckets: bounds,
-		newData: func() AggregationData {
-			return newDistributionData(bounds)
-		},
 	}
+	agg.newData = func() AggregationData {
+		return newDistributionData(agg)
+	}
+	return agg
 }
 
 // LastValue only reports the last value recorded using this
diff --git a/stats/view/aggregation_data.go b/stats/view/aggregation_data.go
index d500e67..f331d45 100644
--- a/stats/view/aggregation_data.go
+++ b/stats/view/aggregation_data.go
@@ -128,12 +128,12 @@ type DistributionData struct {
 	bounds             []float64 // histogram distribution of the values
 }
 
-func newDistributionData(bounds []float64) *DistributionData {
-	bucketCount := len(bounds) + 1
+func newDistributionData(agg *Aggregation) *DistributionData {
+	bucketCount := len(agg.Buckets) + 1
 	return &DistributionData{
 		CountPerBucket:     make([]int64, bucketCount),
 		ExemplarsPerBucket: make([]*metricdata.Exemplar, bucketCount),
-		bounds:             bounds,
+		bounds:             agg.Buckets,
 		Min:                math.MaxFloat64,
 		Max:                math.SmallestNonzeroFloat64,
 	}


Test patch:
diff --git a/stats/view/aggregation_data_test.go b/stats/view/aggregation_data_test.go
index a7e0567..7d09a8f 100644
--- a/stats/view/aggregation_data_test.go
+++ b/stats/view/aggregation_data_test.go
@@ -26,7 +26,10 @@ import (
 )
 
 func TestDataClone(t *testing.T) {
-	dist := newDistributionData([]float64{1, 2, 3, 4})
+	agg := &Aggregation{
+		Buckets: []float64{1, 2, 3, 4},
+	}
+	dist := newDistributionData(agg)
 	dist.Count = 7
 	dist.Max = 11
 	dist.Min = 1
@@ -66,7 +69,10 @@ func TestDataClone(t *testing.T) {
 }
 
 func TestDistributionData_addSample(t *testing.T) {
-	dd := newDistributionData([]float64{1, 2})
+	agg := &Aggregation{
+		Buckets: []float64{1, 2},
+	}
+	dd := newDistributionData(agg)
 	attachments1 := map[string]interface{}{"key1": "value1"}
 	t1 := time.Now()
 	dd.addSample(0.5, attachments1, t1)
diff --git a/stats/view/view_to_metric_test.go b/stats/view/view_to_metric_test.go
index 6c82fb9..18c8771 100644
--- a/stats/view/view_to_metric_test.go
+++ b/stats/view/view_to_metric_test.go
@@ -24,6 +24,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"go.opencensus.io/metric/metricdata"
+	"go.opencensus.io/metric/metricexport"
 	"go.opencensus.io/stats"
 	"go.opencensus.io/tag"
 )
@@ -516,6 +517,139 @@ func TestUnitConversionForAggCount(t *testing.T) {
 	}
 }
 
+type mockExp struct {
+	metrics []*metricdata.Metric
+}
+
+func (me *mockExp) ExportMetrics(ctx context.Context, metrics []*metricdata.Metric) error {
+	me.metrics = append(me.metrics, metrics...)
+	return nil
+}
+
+var _ metricexport.Exporter = (*mockExp)(nil)
+
+func TestViewToMetric_OutOfOrderWithZeroBuckets(t *testing.T) {
+	m := stats.Int64("OutOfOrderWithZeroBuckets", "", "")
+	now := time.Now()
+	tts := []struct {
+		v *View
+		m *metricdata.Metric
+	}{
+		{
+			v: &View{
+				Name:        m.Name() + "_order1",
+				Measure:     m,
+				Aggregation: Distribution(10, 0, 2),
+			},
+			m: &metricdata.Metric{
+				Descriptor: metricdata.Descriptor{
+					Name:      "OutOfOrderWithZeroBuckets_order1",
+					Unit:      metricdata.UnitDimensionless,
+					Type:      metricdata.TypeCumulativeDistribution,
+					LabelKeys: []metricdata.LabelKey{},
+				},
+				TimeSeries: []*metricdata.TimeSeries{
+					{Points: []metricdata.Point{
+						{Value: &metricdata.Distribution{
+							Count:                 3,
+							Sum:                   9.0,
+							SumOfSquaredDeviation: 8,
+							BucketOptions: &metricdata.BucketOptions{
+								Bounds: []float64{2, 10},
+							},
+							Buckets: []metricdata.Bucket{
+								{Count: 1, Exemplar: nil},
+								{Count: 2, Exemplar: nil},
+								{Count: 0, Exemplar: nil},
+							},
+						},
+							Time: now,
+						},
+					},
+						StartTime:   now,
+						LabelValues: []metricdata.LabelValue{},
+					},
+				},
+			},
+		},
+		{
+			v: &View{
+				Name:        m.Name() + "_order2",
+				Measure:     m,
+				Aggregation: Distribution(0, 5, 10),
+			},
+			m: &metricdata.Metric{
+				Descriptor: metricdata.Descriptor{
+					Name:      "OutOfOrderWithZeroBuckets_order2",
+					Unit:      metricdata.UnitDimensionless,
+					Type:      metricdata.TypeCumulativeDistribution,
+					LabelKeys: []metricdata.LabelKey{},
+				},
+				TimeSeries: []*metricdata.TimeSeries{
+					{Points: []metricdata.Point{
+						{Value: &metricdata.Distribution{
+							Count:                 3,
+							Sum:                   9.0,
+							SumOfSquaredDeviation: 8,
+							BucketOptions: &metricdata.BucketOptions{
+								Bounds: []float64{5, 10},
+							},
+							Buckets: []metricdata.Bucket{
+								{Count: 2, Exemplar: nil},
+								{Count: 1, Exemplar: nil},
+								{Count: 0, Exemplar: nil},
+							},
+						},
+							Time: now,
+						},
+					},
+						StartTime:   now,
+						LabelValues: []metricdata.LabelValue{},
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tts {
+		err := Register(tt.v)
+		if err != nil {
+			t.Fatalf("error registering view %v, err: %v", tt.v, err)
+		}
+
+	}
+
+	stats.Record(context.Background(), m.M(5), m.M(1), m.M(3))
+	time.Sleep(1 * time.Second)
+
+	me := &mockExp{}
+	reader := metricexport.NewReader()
+	reader.ReadAndExport(me)
+
+	var got *metricdata.Metric
+	lookup := func(vname string, metrics []*metricdata.Metric) *metricdata.Metric {
+		for _, m := range metrics {
+			if m.Descriptor.Name == vname {
+				return m
+			}
+		}
+		return nil
+	}
+
+	for _, tt := range tts {
+		got = lookup(tt.v.Name, me.metrics)
+		if got == nil {
+			t.Fatalf("metric %s not found in %v\n", tt.v.Name, me.metrics)
+		}
+		got.TimeSeries[0].Points[0].Time = now
+		got.TimeSeries[0].StartTime = now
+
+		want := tt.m
+		if diff := cmp.Diff(got, want); diff != "" {
+			t.Errorf("buckets differ -got +want: %s \n Serialized got %v\n, Serialized want %v\n", diff, serializeAsJSON(got), serializeAsJSON(want))
+		}
+	}
+}
+
 func serializeAsJSON(v interface{}) string {
 	blob, _ := json.MarshalIndent(v, "", "  ")
 	return string(blob)