add zero bucket if not present. #89
Conversation
| @@ -458,6 +459,27 @@ func newTypedValue(vd *view.View, r *view.Row) *monitoringpb.TypedValue { | |||
| return nil | |||
| } | |||
|
|
|||
| func shouldInsertZeroBound(bounds ...float64) bool { | |||
| if len(bounds) > 0 && bounds[0] != 0.0 { | |||
There was a problem hiding this comment.
Should we check if the first bound is negative?
There was a problem hiding this comment.
I think no need to check for negative or zero bucket bounds, as we drop these bounds at top level API. I am not sure how it's done in Go, you may want to refer Java's implementation -> https://github.com/census-instrumentation/opencensus-java/blob/17e327f21921f981951608727602203bbc82238a/api/src/main/java/io/opencensus/stats/BucketBoundaries.java#L64-L90 and https://github.com/census-instrumentation/opencensus-java/blob/master/exporters/stats/stackdriver/src/main/java/io/opencensus/exporter/stats/stackdriver/StackdriverExportUtils.java#L153-L156
There was a problem hiding this comment.
@songy23 negative value is checked here
@mayurkale22 the reason for checking 0 value is that you could have different version (older version) of core library which would send 0 bound bucket. So you need to insert it conditionally.
|
/cc @mayurkale22 |
metrics.go
Outdated
| // The first bucket bound should be 0.0 because the Metrics first bucket is | ||
| // [0, first_bound) but Stackdriver monitoring bucket bounds begin with -infinity | ||
| // (first bucket is (-infinity, 0)) | ||
| Bounds: addZeroBound(insertZeroBound, bexp.Explicit.Bounds...), |
There was a problem hiding this comment.
Optional nit: for better readability consider
if insertZeroBound {
addZeroBound(bexp.Explicit.Bounds...)
}There was a problem hiding this comment.
it would be something like this.
insertZeroBound := false
if bopts := dv.BucketOptions; bopts != nil && bopts.Type != nil {
bexp, ok := bopts.Type.(*metricspb.DistributionValue_BucketOptions_Explicit_)
if ok && bexp != nil && bexp.Explicit != nil {
// The first bucket bound should be 0.0 because the Metrics first bucket is
// [0, first_bound) but Stackdriver monitoring bucket bounds begin with -infinity
// (first bucket is (-infinity, 0))
insertZeroBound = shouldInsertZeroBound(bexp.Explicit.Bounds...)
var bounds []float64
if insertZeroBound {
bounds = addZeroBound(bexp.Explicit.Bounds...)
} else {
bounds = bexp.Explicit.Bounds
}
mv.DistributionValue.BucketOptions = &distributionpb.Distribution_BucketOptions{
Options: &distributionpb.Distribution_BucketOptions_ExplicitBuckets{
ExplicitBuckets: &distributionpb.Distribution_BucketOptions_Explicit{
Bounds: bounds,
},
},
}
}
}
var buckets []int64
if insertZeroBound {
buckets = addZeroBucketCount(bucketCounts(dv.Buckets)...)
} else {
buckets = bucketCounts(dv.Buckets)
}
mv.DistributionValue.BucketCounts = bucketsMay be I can change the method name to addZeroBoundOnCondition
There was a problem hiding this comment.
@songy23 are you ok with renaming the method or you want me to make the change as shown above?
There was a problem hiding this comment.
Renaming the method sounds good.
metrics.go
Outdated
| }, | ||
| }, | ||
| } | ||
| } | ||
| } | ||
| mv.DistributionValue.BucketCounts = addZeroBucketCount(insertZeroBound, bucketCounts(dv.Buckets)...) |
|
fixes #87 |
No description provided.