-
Notifications
You must be signed in to change notification settings - Fork 268
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
Fix logging behavior around setting goproc limits #2018
Conversation
7b1d4a4
to
14a0ee6
Compare
pkg/cmd/limits/limits.go
Outdated
// SetLimitsRunE wraps the RunFunc with setup logic for memory limits and maxprocs | ||
// limits for the go process. Implementing it here means that we can use already- | ||
// established loggers and wrap only those commands that require it. | ||
func SetLimitsRunE() cobrautil.CobraRunFunc { |
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.
This seems useful enough to put into cobrautil
so it can be used across other projects.
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.
I would vote to move this into cobrautil
.
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.
Nice
Fixes #2010
Description
In the implementation of #1921, we added memory and proc limits at the top level, along with logging from those libraries. Because it sits above the level of any of the commands, it didn't respect log levels. This fixes it by moving it into a
PreRunE
wrapper that sits below the level of log setup, which also means that it will be correctly formatted when running from the command line, and also only apply to commands that need it (we probably don't need it onversion
, for example).Notes
@ecordell made the point that this would probably be useful in Materialize as well; if it's desired, we can push this up into
cobrautil
and pull it into both projects that way.Questions
Changes
RunE
functionRunE
function in theDefaultPreRunE
functionTesting
See that
spicedb version
no longer reports changes from the limit libraries. See that if you set--log-level debug
, you see output related to these libs nicely-formatted early in the output ofspicedb serve
and friends.