-
Notifications
You must be signed in to change notification settings - Fork 1.8k
pack: add 'epoch_ms' format #4131
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
Conversation
|
thanks for this contribution. Can you make the default name |
69d0fef to
8b449b0
Compare
|
Thank you for your suggestion. I think that's a better idea. |
8b449b0 to
d80e644
Compare
PettitWesley
left a comment
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.
@jeongki-kim If rebase this with the latest (another format was added), I think we can merge it. Thanks for this contribution and sorry for the time it has taken to review.
d80e644 to
4311417
Compare
|
Hi @PettitWesley and @edsiper Since @jeongki-kim already fixed his PR to work with The latter is trivial and can be easily cherry picked in case you guys want to go ahead and merge this one directly. |
|
@jeongki-kim Thank You 😃 |
|
@edsiper This is ready. Also, Did the requirement on the number of maintainer approvals change? I thought after approving this I could merge it myself... anyway please merge it now.. |
|
@marcosdiez Thank you for your help! I think this PR is ready for review now. |
No changes I'm aware of @PettitWesley, it looked like we had a push after this comment as well so can't check now as needed an approval to run the tests again. |
|
@edsiper please merge |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
this should be merged! (message sent to remove stale PR) |
|
@PettitWesley the requirement is for codeowner approval, not just any maintainer (although that helps). You're not down as a codeowner for the files affected so I think this is why your approval does not allow merge: https://github.com/fluent/fluent-bit/blob/master/CODEOWNERS |
Signed-off-by: Jeongki Kim <[email protected]>
e2a540f to
9c7469e
Compare
|
Will this PR eventually be merged anytime soon? |
|
Do we waiting for something here? |
|
@edsiper Could you please take a look into this? |
| break; | ||
| case FLB_PACK_JSON_DATE_EPOCH_MS: | ||
| msgpack_pack_uint64(&tmp_pck, | ||
| (long long unsigned)(tms.tm.tv_sec) * 1000 + |
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 create the function flb_time_to_millisec in flb_time.c and move that conversion there so it can be reused by others in the future? Also, please use uint64_t like flb_time_to_nanosec.
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.
@jeongki-kim Could you make these changes, please?
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.
Thank you for your review. Had I found this earlier, I'd have followed your suggestion.
|
Addressed by #6114 |
This PR adds
epoch_msformat which represents UNIX epoch in milliseconds.Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
Documentation
Example configuration snippet
Debug log output
Valgrind output
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.