-
Notifications
You must be signed in to change notification settings - Fork 1k
Adding json support for log-format config #1791
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
cea0da3 to
d8edd52
Compare
|
Edit: Fixed. I think it was complaining about using the wrong representation of |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1791 +/- ##
============================================
+ Coverage 73.81% 73.90% +0.09%
============================================
Files 125 125
Lines 69347 69353 +6
============================================
+ Hits 51187 51256 +69
+ Misses 18160 18097 -63
🚀 New features to boost your workflow:
|
40b9e16 to
c4b19ab
Compare
|
Nice to see this has been started. We need to escape the strings. There's some code for that in cli_common.c: We could move it to another file like util.c, since it will no longer be only for the CLI. |
Makes sense. Would be much more comfortable if I could test this as we go. Should we start a unit test for basic functions? |
Yes, we can do that. Another option is to add an integration test where do something that causes something to be logged and then verify what's logged in the log file. Grep for We can use the command |
a6938e1 to
a67fd89
Compare
|
@zuiderkwast long time since last update -- apologies; life came in-between. I've done the changes you requested (refactor, add tests). Will make it ready for review. Finally – if there is places that lacks documentation, feel free to point me to them. Thanks for any feedback! |
a67fd89 to
4d10273
Compare
Introduce a json log format that can be invoked by passing `log-format json` in your config. Signed-off-by: Johan Bergström <bugs@bergstroem.nu>
Signed-off-by: Johan Bergström <bugs@bergstroem.nu>
4d10273 to
7a937d6
Compare
Signed-off-by: Johan Bergström <bugs@bergstroem.nu>
7a937d6 to
a44e57c
Compare
Signed-off-by: Johan Bergström <bugs@bergstroem.nu>
f710e74 to
20aa69e
Compare
zuiderkwast
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.
LGTM
@valkey-io/core-team When we decided to add the logfmt format in #1022 (comment), it appears we also already agreed to add the JSON log format. Please comment if you don't agree.
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Binbin <binloveplay1314@qq.com>
enjoy-binbin
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.
LGTM.
|
Thanks Johan! |
Introduce a json log format that can be invoked by passing `log-format json` in your config. We also refactor `escapeJsonString` to `util` since it is now a shared function. Closes: valkey-io#1006 --------- Signed-off-by: Johan Bergström <bugs@bergstroem.nu> Signed-off-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Introduce a json log format that can be invoked by passing
log-format jsonin your config.We also refactor
escapeJsonStringtoutilsince it is now a shared function.Closes: #1006