-
Notifications
You must be signed in to change notification settings - Fork 106
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
Outdated version of the zap
logger, with multiple wrappers within Gorouter, used in Routing Release
#371
Comments
➕ If we touch this it would probably make sense to use
|
Submodule src/code.cloudfoundry.org/gorouter 2f0bfad7..1e6cea82: > test(proxy): Make gorouter_time less strict on slow responses (cloudfoundry#371) > test(proxy): Fix transfer encoding test stability issues (cloudfoundry#370)
We recently noticed that our current usage of zap is (periodically) flushing logs when writing a log. This results in syscalls that prevent the calling goroutine from doing its work. Most prominently we have some larger environments in which we see (almost) constant nats message buffering. Upon closer inspection we noticed that one of the contributors for messages which take a long time to process are More recent versions of zap offer a With our current version of zap this is not supported and as such we can't fix this issue. @dsabeti since you are assigned: how do you think we should proceed here? |
We are planning to start on this soon and plan to use |
No. I couldn't find CVEs for zap.
Issue
The version of the zap logger used in
routing-release
and thus Gorouter is pinned to a version from 2016. The API has since changed significantly in newer releases. Performance characteristics may have as well.Additionally, and potentially worse, there are two wrappers (lager + logger) that ultimately write into zap. The major issue with that is how the wrappers handle auxiliary data to be logged. The data is pre-processed without taking the active log level into consideration.
Each debug statement will actually pre-process and wrap data into
zap.Field
, just to never be logged in 99% of cases. There are means for lazy evaluation for such data, but they need to be handled explicitly, e.g. in such wrappers.Affected Versions
Probably all version from 2016 onwards.
Context
As stated above, the zap library is significantly outdated. Furthermore, the wrappers are inefficiently handling auxiliary data when it ultimately will not be logged.
Traffic Diagram
N/A
Steps to Reproduce
N/A
Expected result
We have an up to date version of zap to get functional, performance and security improvements.
We don't waste additional cycles on preparing data that will ultimately not be logged.
Current result
See above.
Possible Fix
Remove the version pin for zap.
Adapt, rewrite or remove the later + logger wrappers. Alternatively, one wrapper could remain if we don't want to tie ourselves to zap. A lot of the code is tied to zap however.
Very alternatively, we could use golang 1.21's
slog
feature. That also has a zap backend.Considering that the rift between 2016 zap and current zap is huge, a lot of the logging code would need to be rewritten anyway. If we tackle this larger undertaking, we might also do it the best way available at the current time. Opinions are welcome.
Additional Context
The text was updated successfully, but these errors were encountered: