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

Mask uint64 with 63bit mask by default #8991

Merged
merged 5 commits into from
Nov 28, 2018
Merged

Conversation

urso
Copy link

@urso urso commented Nov 8, 2018

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.

@urso urso added in progress Pull request is currently in progress. discuss Issue needs further discussion. libbeat labels Nov 8, 2018
@urso urso added review and removed in progress Pull request is currently in progress. labels Nov 8, 2018
@jsoriano
Copy link
Member

jsoriano commented Nov 8, 2018

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 double, having this we would lose precission, but we could store bigger values. Also prometheus uses float64 for gauges.

Changes in gauges to float64 would need to be addressed as a breaking change in 7.0.

@urso
Copy link
Author

urso commented Nov 8, 2018

Masking might not always be the best solution, but there might be other reasons why values become this big. E.g. a metrics with 0xffffffffffffffff could be a flag and no actual value (e.g. infinite marker). It's up to the module to make the right decision like:

  • live with 63bit masking (for a many gauges having this large values should be somewhat uncommon)
  • just convert to int64, such that differences between subsequent values are still 'correct' (in case of derivative/differences being of interest, not actual values)
  • use double (accept loss of precision - and even float64 has upper limits)
  • create structured object with value and flags

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.

Copy link
Member

@jsoriano jsoriano left a 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.

Copy link
Member

@andrewkroh andrewkroh left a 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.

libbeat/common/event.go Show resolved Hide resolved
libbeat/common/event.go Show resolved Hide resolved
Copy link
Member

@ruflin ruflin left a 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

@urso urso mentioned this pull request Nov 28, 2018
6 tasks
@urso
Copy link
Author

urso commented Nov 28, 2018

Followup ticket: #9271

@urso urso merged commit 7303111 into elastic:master Nov 28, 2018
@ruflin
Copy link
Member

ruflin commented Nov 29, 2018

@jsoriano As we probably don't backport this, do you we need to do some additional fixes in 6.x?

@jsoriano
Copy link
Member

@ruflin we should review the failing cases, yes, but also even if it was backported :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs further discussion. libbeat review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants