-
Notifications
You must be signed in to change notification settings - Fork 669
Don't log DNS queries by default #1368
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
Local server names/addresses are often considered confidential and should not by default be logged. Their loglevel has been lowered to TRACE. The host agent code now switches to TRACE level logging when the instance is started via `limactl --debug start ...`. Signed-off-by: Jan Dubois <[email protected]>
@Nino-K Can you take a look if this makes sense to you? |
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
@@ -122,5 +122,10 @@ func initLogrus(stderr io.Writer) { | |||
logrus.SetOutput(stderr) | |||
// JSON logs are parsed in pkg/hostagent/events.Watcher() | |||
logrus.SetFormatter(new(logrus.JSONFormatter)) | |||
logrus.SetLevel(logrus.DebugLevel) | |||
// HostAgent logging is one level more verbose than the start command itself |
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.
Isn't this a bug?
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.
Idk, I thought it was intentionally set to always log at DEBUG because the output goes to ha.stderr.log
and doesn't pollute the terminal output: e88241e.
Which made sense to me: I don't normally want the output of limactl --debug start
in the terminal, but I only look at ha.stderr.log
when something is wrong, so always getting DEBUG output there is useful, so you don't have to restart your VM with --debug
to get full logs.
So assuming this was intentional, I extended it to include TRACE level output when you run with --debug
, as there is no way to specify the logging level for HA separately from the main logging level.
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.
IIRC that was just my silly workaround to avoid bother implementing --log-level
propagation 🤦
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.
IIRC that was just my silly workaround to avoid bother implementing --log-level propagation
Log-level propagation is implemented (and I tested that it works when you remove the explicit override to DEBUG in the HA code):
Lines 127 to 129 in 5b9df0e
if logrus.GetLevel() >= logrus.DebugLevel { | |
args = append(args, "--debug") | |
} |
I continue to think that always having debug logging active in ha.stderr.log
is useful, even though it feels inconsistent. I just don't want to include DNS lookups in it by default.
So on a practical level this PR works for me. If we want to regularize the settings, I think we need a separate log-level for HA that should default to DEBUG. But this is just adding complexity for little benefit.
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.
Log-level propagation is implemented
I just checked, and the propagation was already implemented by the time the override was added. So it still feels intentional to me. 😄
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.
Propagation of custom log level is not implemented
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 don't know what you mean by that. We only have INFO and DEBUG log levels in limactl
, and that level is propagated to the HA, but then overridden to always be DEBUG.
Anyways, I'm lost now at what you want to do.
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.
Sorry, you are right, I've forgotten my own coding design 😞
Local server names/addresses are often considered confidential and should not by default be logged. Their loglevel has been lowered to TRACE.
The host agent code now switches to TRACE level logging when the instance is started via
limactl --debug start ...
.Fixes #1367