-
Couldn't load subscription status.
- Fork 21.5k
log: remove custom json and logfmt handlers #28638
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
c94437d to
58378e0
Compare
|
Triage for discussion |
|
triage discussion: check on twitter if any users of logfmt/json yell |
|
shameless plug: https://github.com/lmittmann/tint a pretty |
Taylor G. wrote back that it does break graylog collectors. Are we saving anything performance wise to make it necessary to do the change? The extra code is not really meaningfully large. My 2c currently is not to break it as there's no real gain in having t vs time. |
Unless it is significantly faster than what we use currently (also it needs field alignment), it's probably simpler to keep the current code vs introduce new dependencies. |
Btw, I meant to say (but forgot): I looked at that earlier, looks really neat. I'm probably going to use that in some project down the line, but I think we should maybe not use it for geth. |
Fair enough. Tho we should probably just drop the |
Potential follow-up to #28622
This PR removes our custom
jsonandlogfmtimplementations, to instead use the ones built intoslog. Ours does some transformations under the hood, mainly renamingtimetotandleveltolvl, and de-capitalizing the levels.It also does away with the custom key-value pair "filling". Instead of
key=<nil> LOG_ERROR="Normalized odd number of arguments by adding nil"slog simply saysBADKEY=keyThe one other thing worth mentioning is that this makes the json output spit out
*big.Inttypes as json-native numeric format, e.g111222333444555678999instead of"111222333444555678999". When trying to read that back intoany, the unmarshaller selectsfloat64, which cannot read it fully correct on the way back.I think it would be better to switch over to
slog, if we believe that slog is some kind of go-idiomatic future of logging. A few of our users may potentially have to switch over their parsing, but all in all it's probably not a great amount of work, and probably something that's better done sooner rather than later.