Exporter/Prometheus: Simplify histogram creation.

[songy23]

And resolve a TODO.

With #979 and #1037, bucket bounds are guaranteed to be positive and strictly increasing.

[odeke-em]

@songy23 thanks for this change! However, I'd rather keep it as it was; merging this PR will take us back to a problem that was actually reported in May 2018. This is a bug we saw in the wild in #649 and fixed with PR #661
and Prometheus mentions this expectation at https://godoc.org/github.com/prometheus/client_golang/prometheus#NewConstHistogram.

[odeke-em]

If a user defines v := view.Distribution(0, 10, 9, 20, 40) what does OpenCensus-Go do? Currently nothing, which wouldn't mitigate the problem at core. We should at that call site panic but we don't. Those changes in #979 and #1037 enforce no negative bounds but really this code is about monotonic arrangement and sorting which is totally different from the assumption of this PR, thus my objection to not merge.

[songy23]

AFAIS #649 is a different issue where we didn't use cumulative counts for histogram in Prometheus. This PR doesn't change that part.

For the case when bucket bounds are out of order, we should fail fast when creating the distribution aggregation. This is the case in Java: https://github.com/census-instrumentation/opencensus-java/blob/master/api/src/main/java/io/opencensus/stats/BucketBoundaries.java#L52-L58. I'll add a PR in Go that also checks this.

[songy23]

If a user defines v := view.Distribution(0, 10, 9, 20, 40) what does OpenCensus-Go do?

This should return an error. This is already specified in the proto and is also a requirement for GA. /cc @mayurkale22

[rghetia]

it sorts the buckets here

[songy23]

it sorts the buckets here

I see, then we can always guarantee the bucket bounds are ordered. I think merging this PR won't cause any problem then.

[rghetia]

it sorts the buckets here

I see, then we can always guarantee the bucket bounds are ordered. I think merging this PR won't cause any problem then.

I don't think it should create any problem.

[odeke-em]

Alright, cool then. But let's add a test to ensure that it doesn't trip things out. I ask because the PR #979 that adding sorting (4 months ago) didn't add a test for that check hence the assumptions that it won't break.

[bogdandrutu]

+1 for having a test

[songy23]

Added a test in 1f91333, PTAL.

[odeke-em]

There is a problem here, 7.69 has to be in 10 bucket. Or is that just a bad copy+paste?

[songy23]

I copied and pasted it from

opencensus-go/exporter/prometheus/prometheus_test.go

Lines 288 to 295 in ed9f155

	// 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
	// 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

. Looks like the previous comments are wrong. Updated.

[odeke-em]

I think they got stale during the removal of 0 as the first bucket bound, but now this is cool. Thanks for fixing them!

[odeke-em]

Also make sure that we duplicate a bucket boundary in there as the previous code also deduplicated boundaries :)

[songy23]

SG, done.