-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Mask uint64 with 63bit mask by default #8991
Conversation
I agree with this approach as a compromise solution. It is not perfect as it can generate incorrect gauges, but at least for the rest of cases it is better than current situation. Also we wouldn't be losing whole events just for some "incorrect" values. Rest of cases should be handled one by one on the modules. For example for most gauges we could also use Changes in gauges to float64 would need to be addressed as a breaking change in 7.0. |
Masking might not always be the best solution, but there might be other reasons why values become this big. E.g. a metrics with
Depending on use-case we could provide custom go types + fields.yml types helping with the implementation. But it's up to the module author to make the right decision. |
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.
Approving, but I'd like to hear more opinions before merging. This would also need a changelog entry.
And we'd also have to think if we want to backport it.
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 feels like a fairly big change so I’m leaning towards not backporting.
After merging this we should go through the various open issues that this will resolve and determine if this is the correct resolution. If not we can make one of the changes mentioned above to that particaular field.
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.
+1 on the suggestion from @andrewkroh
eadb644
to
3e93fb2
Compare
Followup ticket: #9271 |
@jsoriano As we probably don't backport this, do you we need to do some additional fixes in 6.x? |
@ruflin we should review the failing cases, yes, but also even if it was backported :) |
64bit unsigned values can not be stored in ES. This PR applies a 63bit mask to all values of type uint64.
We normally see uint64 in either counters, but sometimes with gauges. For pure counter values we normally use derivatives in order to show rates. As aggregations convert values to float before computing the derivate, it makes no differences if counters are converted to int64 or if we keep only 63bits. For gauges a values > 2^64 is often times some kind of special flag (e.g. infinite). If the 63bit representation is not good enough we push the responsibility back to the module to either: convert the value into a another type or create an object with additional information/flags.