Skip to content
This repository was archived by the owner on Jul 31, 2023. It is now read-only.

exclude zero bucket from aggregation_data#1183

Merged
rghetia merged 3 commits intocensus-instrumentation:masterfrom
rghetia:distribution
Nov 9, 2019
Merged

exclude zero bucket from aggregation_data#1183
rghetia merged 3 commits intocensus-instrumentation:masterfrom
rghetia:distribution

Conversation

@rghetia
Copy link
Copy Markdown
Contributor

@rghetia rghetia commented Nov 7, 2019

fixes #1169

@rghetia rghetia requested review from a team and rakyll as code owners November 7, 2019 05:58
@rghetia rghetia requested review from dinooliva and songy23 November 7, 2019 06:17
@rghetia
Copy link
Copy Markdown
Contributor Author

rghetia commented Nov 8, 2019

@songy23 @dinooliva can either one of you please look at this? Thanks.

Copy link
Copy Markdown
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, otherwise LGTM.

m := stats.Int64("OutOfOrderWithZeroBuckets", "", "")
v := &View{
Measure: m,
Aggregation: Distribution(10, 0, 2),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test with bucket bounds like [0, 5, 10]?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@rghetia rghetia merged commit aad2c52 into census-instrumentation:master Nov 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Including 0 bound when creating distribution leads to mismatch in bucket count between view and rows of actual data.

3 participants