ignore negative bucket bounds

[mayurkale22]

No description provided.

[songy23]

For backwards-compatibility, I think we need to continue supporting negative buckets in Stats APIs. In my opinion we should drop the negative buckets when converting MutableViewData to Metrics rather than drop them when doing the aggregation.

@bogdandrutu WDYT?

[codecov-io]

Codecov Report

Merging #1499 into master will increase coverage by 0.15%.
The diff coverage is 94.28%.

@@             Coverage Diff              @@
##             master    #1499      +/-   ##
============================================
+ Coverage     82.56%   82.71%   +0.15%     
- Complexity     1662     1674      +12     
============================================
  Files           251      251              
  Lines          7753     7781      +28     
  Branches        756      763       +7     
============================================
+ Hits           6401     6436      +35     
+ Misses         1125     1121       -4     
+ Partials        227      224       -3

Impacted Files	Coverage Δ	Complexity Δ
.../opencensus/implcore/stats/MutableAggregation.java	91.71% <ø> (+3.18%)	0 <0> (ø)	⬇️
...ain/java/io/opencensus/stats/BucketBoundaries.java	100% <100%> (ø)	11 <6> (+6)	⬆️
...a/io/opencensus/implcore/stats/MeasureMapImpl.java	100% <100%> (ø)	11 <7> (+4)	⬆️
...i/src/main/java/io/opencensus/stats/NoopStats.java	94.02% <77.77%> (-2.7%)	4 <1> (ø)
...ion/log4j2/OpenCensusTraceContextDataInjector.java	95% <0%> (ø)	9% <0%> (ø)	⬇️
.../opencensus/contrib/zpages/StatszZPageHandler.java	90.5% <0%> (+0.94%)	46% <0%> (+1%)	⬆️
...ava/io/opencensus/metrics/export/Distribution.java	97.61% <0%> (+2.38%)	7% <0%> (+1%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f07e4b...4ff5f41. Read the comment docs.

[bogdandrutu]

Negative values are currently not supported by any of the backends that we support and have histograms (Stackdriver and Prometheus). In Stackdriver for example sum cannot go down.

There are multiple things to consider:

1. If you drop negative from the MutableView, you should drop from count and sum as well.
2. To do a backwards compatible change we should probably have 2 count and 2 sum and always add a 0 in the bucket definition. One count/sum for >=0 and one count/sum for <0 and make sure that we always have negative buckets and positive buckets separated (one bucket cannot have negative and positive).
3. Probably we should consider a backwards incompatible change and drop negative values in the Record API, that will simplify a lot of things (I would like to hear what others think about this). @dinooliva @songy23 @mayurkale22?

[songy23]

Probably we should consider a backwards incompatible change and drop negative values in the Record API, that will simplify a lot of things (I would like to hear what others think about this).

Given the complexity of maintaining both the positive and negative aggregated values, I think dropping negative values makes more sense. An alternative is to throw an unchecked exception for negative values so that users will be noticed that negative values are no longer supported (instead of silently skipping them).

[mayurkale22]

Probably we should consider a backwards incompatible change and drop negative values in the Record API, that will simplify a lot of things (I would like to hear what others think about this).

I think 2nd option will make things really complicated and might create a pain in future in maintaining that. I am in favor of 3rd option, also throwing an exception will make things clear for users.

[bogdandrutu]

I think we should not directly throw an exception. We can start with logging an error for few versions then throw an exception. We should explain this to the current users very clearly.

[songy23]

We can start with logging an error for few versions then throw an exception. We should explain this to the current users very clearly.

Agree that logging is a good starting point. We should document this behavior in the Javadoc as well as in the CHANGELOG.

[songy23]

IIUC the idea is to not allow negative values in MeasureMap.record? Or at least, drop negative values in MutableDistribution.record.

With your current approach, sum and count can still accept negative values and can go unsync with the bucket counts.

[bogdandrutu]

I agree with @songy23 we should drop negative values in the Record (and drop all recorded values in that record batch).

[songy23]

LGTM