Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Google Cloud Monitoring: Fix bucket bound for distributions #56565

Merged
merged 1 commit into from Oct 10, 2022

Conversation

andresmgot
Copy link
Contributor

@andresmgot andresmgot commented Oct 7, 2022

What this PR does / why we need it:

In a DISTRIBUTION response, there are two values that affect this PR: BucketCounts and ExplicitBuckets.Bounds. For the following values:

BucketCounts: [0, 1, 2, 3]
ExplicitBuckets.Bounds: [0, 10, 20]

It means that the bucket {-Inf:0} has 0 values, {0:10} has 1, {10:20} has 2 and {20:+Inf} has 3.

Since we are using the upper bound to generate the frame name, frame names should be: 0, 10, 20 and 20+.

Prior to this PR, values outside the last bound were not taken into account causing a panic error.

Which issue(s) this PR fixes:

Fixes #56181

Special notes for your reviewer:

@andresmgot andresmgot requested a review from a team as a code owner October 7, 2022 15:06
@andresmgot andresmgot requested review from aangelisc and yaelleC and removed request for a team October 7, 2022 15:06
Comment on lines +106 to +115
"8",
"0",
"0",
"0",
"0",
"0",
"0",
"0",
"0",
"0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this does not really represent reallity since in BucketCounts if the rest of values are 0, it's just ommited for performance reasons (that's why usually BucketCounts.length is smaller than the ExplicitBounds) but it's helpful to represent the issue in a unit test.

@andresmgot andresmgot added add to changelog backport v9.1.x Bot will automatically open backport PR backport v9.2.x Mark PR for automatic backport to v9.2.x labels Oct 7, 2022
@andresmgot andresmgot added this to the 9.1.8 milestone Oct 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2022

Backend code coverage report for PR #56565

Plugin Main PR Difference
cloudmonitoring 57.7% 57.8% .1%

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2022

Frontend code coverage report for PR #56565
No changes

@grafanabot
Copy link
Contributor

Copy link
Contributor

@aangelisc aangelisc left a comment

Choose a reason for hiding this comment

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

The code here looks fine to me. I'm not overly familiar on GCM so would benefit from a more in-depth explanation of the problem if possible 😊

@andresmgot
Copy link
Contributor Author

I'm not overly familiar on GCM so would benefit from a more in-depth explanation of the problem if possible

The problem is basically that the number of "buckets" (metric) is bigger than the number of the bucket "bounds" or limits. Prior to this PR, it was not considered values over the last limit but it's something that can happen. A couple of examples to try to explain it.

  1. Buckets without reaching the limit (less bucket counts than explicit buckets):
Explicit bucket bound:      0   10  20  30
                         <--|---|---|---|-->
Bucket count:             1   0   2   0   0  

For this case, ExplicitBuckets.Bounds is [0, 10, 20, 30] and BucketCounts is [1, 0, 2] (since it omits the last values if it's always 0). This means that there are 1 value in the bucket that goes from -Infinite to 0, 0 values in the bucket that goes from 0 to 10 and 2 in the bucket that goes from 10 to 20.

  1. Buckets with values over the limit (more bucket counts than explicit buckets):
Explicit bucket bound:      0   10  20  30
                         <--|---|---|---|-->
Bucket count:             1   0   2   0   1  

For this case, the ExplicitBuckets.Bounds is the same, but the BucketCounts is [1, 0, 2, 0, 1], meaning that there is 1 value in the bucket between 30 and +Infinite. This is what was not covered in the previous code (it assumed there were never values in that last bucket).

Hopefully that is more clear, but let me know if you have more questions, we can jump into a call :)

Comment on lines +599 to +604
if n < len(bucketOptions.ExplicitBuckets.Bounds) {
bucketBound = fmt.Sprintf("%g", bucketOptions.ExplicitBuckets.Bounds[n])
} else {
lastBound := bucketOptions.ExplicitBuckets.Bounds[len(bucketOptions.ExplicitBuckets.Bounds)-1]
bucketBound = fmt.Sprintf("%g+", lastBound)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify with ternary operator:

Suggested change
if n < len(bucketOptions.ExplicitBuckets.Bounds) {
bucketBound = fmt.Sprintf("%g", bucketOptions.ExplicitBuckets.Bounds[n])
} else {
lastBound := bucketOptions.ExplicitBuckets.Bounds[len(bucketOptions.ExplicitBuckets.Bounds)-1]
bucketBound = fmt.Sprintf("%g+", lastBound)
}
i := n < len(bucketOptions.ExplicitBuckets.Bounds) ? n : len(bucketOptions.ExplicitBuckets.Bounds)-1
bucketBound = fmt.Sprintf("%g+",bucketOptions.ExplicitBuckets.Bounds[i])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is no ternary operator in Go :) Also, the + of %g+ is only added for the case in which the index is outside bounds

@andresmgot andresmgot merged commit 65e56c9 into main Oct 10, 2022
@andresmgot andresmgot deleted the gcmExplicitDistribution branch October 10, 2022 14:08
grafanabot pushed a commit that referenced this pull request Oct 10, 2022
grafanabot pushed a commit that referenced this pull request Oct 10, 2022
andresmgot added a commit that referenced this pull request Oct 10, 2022
…56648)

(cherry picked from commit 65e56c9)

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
andresmgot added a commit that referenced this pull request Oct 10, 2022
…56649)

(cherry picked from commit 65e56c9)

Co-authored-by: Andres Martinez Gotor <andres.martinez@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Google Cloud Monitoring runtime error index out of range
4 participants