-
Notifications
You must be signed in to change notification settings - Fork 464
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
Fix IsFloatHistogram #4706
Fix IsFloatHistogram #4706
Conversation
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.
To me this change makes sense (helpful tests!), but I'd like to hear from someone with more experience with this code.
BTW, shouldn't there be a changelog entry?
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.
As @aknuds1 says, please add changelog entry. Mind the required order in entries as well plz!
Count: 0, | ||
ZeroThreshold: 0.001, | ||
Schema: 2, | ||
PositiveSpans: []histogram.Span{ | ||
{Offset: 0, Length: 1}, | ||
{Offset: 3, Length: 1}, | ||
{Offset: 2, Length: 2}, | ||
}, | ||
PositiveBuckets: []int64{0, 0, 0, 0}, |
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.
This is an invalid histogram. We should not be supporting this, but rather fixing the source of such histograms. This fix can possibly create more subtle and hard-to-debug bugs. TSDB won't ingest these samples even if given. (in a way, you found out that the count was 0 because of the code assuming it as a float, so it was a good thing IMO that surfaced a bug in histogram generation)
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.
Ok my bad, I did not see the contents of the bucket, it is indeed valid
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.
LGTM
What this PR does
Fix bug mentioned in slack: float histograms are sometimes wrongly interpreted as integer histograms (you can test the 1st commit to see the failing new test, and the 2nd and 3rd commits make that test pass)
Which issue(s) this PR fixes or relates to
N/A
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]