-
Notifications
You must be signed in to change notification settings - Fork 89
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
histogram: get sample count after distinguishing classic and native #176
Conversation
`GetSampleCount` returns 0 for float histograms. Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
However, even classic histograms can be float histograms (which is a relatively new addition to the protobuf spec, so prom2json wasn't aware of this so far).
See my comment how to fix count, but we also have to fix makeBuckets
and let it try GetCumulativeCountFloat
first.
@@ -112,7 +112,6 @@ func makeHistogram(m *dto.Metric) Histogram { | |||
hist := Histogram{ | |||
Labels: makeLabels(m), | |||
TimestampMs: makeTimestamp(m), | |||
Count: fmt.Sprint(dtoH.GetSampleCount()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even classic histograms can be float histograms, so we need to treat this differently for both classic and native histograms.
So what we have to do is to first check dtoH.GetSampleCountFloat()
. If that's > 0, take it. Otherwise, take dtoH.GetSampleCount()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh I didn't know that. I guess we need the same in pushgateway too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, although I think it will never really happen that somebody pushes a classic float histogram to the PGW. But yes, in principle, we want that in the PGW, too.
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you.
GetSampleCount
returns 0 for float histograms.