-
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
Timestamp fractions pattern #15911
Timestamp fractions pattern #15911
Conversation
return strconv.AppendInt(bs, int64(val), 10) | ||
} | ||
|
||
func appendFractPadded(bs []byte, val, digits, fractSz int) []byte { |
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.
Could you please add more comments or a unit test to this function? I am having a hard time wrapping my head around 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.
Ok, I think I got it now. But still, a few small test cases would be nice.
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.
Added a comment. The function is indirectly tested via the public API tests. I'd prefer to keep blackbox testing based on the public API only.
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.
Overall LGTM. It would be nice to have a bit more unit tests for lower lever functions to make understanding the code quicker to others and the future you. :)
{mkTime(1, 2, 3, 123*time.Microsecond), "ffffff", "000123"}, | ||
{mkTime(1, 2, 3, 123*time.Microsecond), "fffffff", "000123"}, | ||
{mkTime(1, 2, 3, 123*time.Microsecond), "ffffffff", "000123"}, | ||
{mkTime(1, 2, 3, 123*time.Microsecond), "fffffffff", "000123"}, |
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.
Just curious: Why are the trailing zeros removed? As a user, I would expect to see as many digits as I configured in the format string.
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.
We have multiple formatters for fractions of a second: S
, n
, and f
:
S
(fraction of second): print as many digits as set in format stringn
(nanoseconds): always print 9 digits independent of the number ofn
in the format stringf
(precision aware fraction of second): if timestamp precision is millisecond we print 3, if microsecond we print 6, and if nanosecond we print 9 digits.
By using f
in Beats we would keep the original timestamp precision in case we parse a timestamp from a JSON file.
* Add support for `f` in date patterns * Use `f` pattern in timestamp encoder * update check * Add godoc * typo
What does this PR do?
Introduce
f
(fractional) to date formatter supporting encoding milliseconds, microseconds and nanoseconds.Update timestamp encoder to use the
f
formatter.Note: this PR is part of #15871 and some tests will still fail. Tests failing on unexpected timestamp format will be fixed in follow up PR.
Why is it important?
The
f
pattern removes trailing zeroes, such that a multiple of 3 fractional digits are displayed. Timestamps with millisecond precisions will be encoded with microsecond precision only, while nanosecond precision is still possible.Checklist
Author's Checklist
How to test this PR locally
Related issues