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

exclude zero bucket from aggregation_data #1183

Merged
merged 3 commits into from
Nov 9, 2019
Merged

exclude zero bucket from aggregation_data #1183

merged 3 commits into from
Nov 9, 2019

Conversation

rghetia
Copy link
Contributor

@rghetia rghetia commented Nov 7, 2019 •
edited
Loading

fixes #1169

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

rghetia commented Nov 8, 2019

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

songy23
songy23 approved these changes Nov 8, 2019
View reviewed changes
Copy link
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.

stats/view/view_to_metric_test.go Outdated
m := stats.Int64("OutOfOrderWithZeroBuckets", "", "")
v := &View{
Measure: m,
Aggregation: Distribution(10, 0, 2),
Copy link
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
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.
Reviewers

@songy23 songy23 songy23 approved these changes

@rakyll rakyll Awaiting requested review from rakyll

@dinooliva dinooliva Awaiting requested review from dinooliva

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
3 participants
@rghetia @songy23 @googlebot