Skip to content
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

Propagate all logging fields to the tty logger #2233

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

jandubois
Copy link
Member

@jandubois jandubois commented Mar 6, 2024

The fields are serialized in JSON by the hostagent and then printed by the limactl start command to the console, so need to be explicitly added back to the logger entry.

This PR is in response to #2228 (comment).

Note that nested structures (like events.Event) will be flattened by the standard logger, e.g.

       err := errors.New("oops")
       ev := events.Event{
               Status: events.Status{
                       Running: true,
                       Errors:  []string{"Error one", "Error two"},
               },
       }
       logrus.WithError(err).WithField("events", ev).Warn("Log message with error")

will result in

WARN[0020] [hostagent] Log message with error            error=oops events="map[status:map[errors:[Error one Error two] running:true] time:0001-01-01T00:00:00Z]"
CleanShot 2024-03-06 at 00 26 57@2x

The change to qemu_driver.go is needed to avoid the ugly <nil> error in

INFO[0004] [hostagent] QEMU has exited                   error="<nil>"

@balajiv113 Your inotify PR has additional logging entries with duplicate err which should be changed if this PR is accepted/merged:

pkg/hostagent/hostagent.go
604:				logrus.WithError(err).Warn("failed to start inotify", err)

pkg/hostagent/inotify.go
52:				logrus.WithError(err).Warn("failed to send inotify", err)

@jandubois jandubois force-pushed the hostagent-logger branch 4 times, most recently from a834728 to 4643592 Compare March 6, 2024 18:18
@jandubois
Copy link
Member Author

I made one more change to replace the switch lv block with this logic, which feels clearer to me:

if !logger.IsLevelEnabled(lv) {
	return
}
// …
if lv <= logrus.FatalLevel {
	entry = entry.WithField("level", lv)
	lv = logrus.ErrorLevel
}
entry.Log(lv, header+j.Msg)

@jandubois
Copy link
Member Author

@AkihiroSuda Speaking of comments, I think this line could use some explanation:

if !j.Time.IsZero() && !begin.IsZero() && begin.After(j.Time.Add(epsilon)) {
	return
}

It seems to be added by 21b7cb0 to control logging for limactl stop iiuc, and seems to ignore log messages that are more than one second before the begin value.

It is not clear to me why this is needed; I assume if there is still a backlog of unprocessed log messages from before the stop command, that you don't want to dump them out to the terminal?

@jandubois jandubois requested a review from AkihiroSuda March 6, 2024 18:37
@jandubois
Copy link
Member Author

I just realized that there are tests in logrusutil_test.go that should be updated too. I won't have time for this for the next few days though, so will turn this into draft for now.

I don't plan on changing anything else but the tests though, so feel free to review the draft code in case you have any feedback!

@jandubois jandubois marked this pull request as draft March 7, 2024 02:44
@jandubois jandubois marked this pull request as ready for review March 7, 2024 07:34
@jandubois
Copy link
Member Author

I won't have time for this for the next few days though, so will turn this into draft for now.

I added the tests and took it out of draft; I think it is good now.

The fields are serialized in JSON by the hostagent and then
printed by the `limactl start` command to the console, so
need to be explicitly added back to the logger entry.

Signed-off-by: Jan Dubois <jan.dubois@suse.com>
@AkihiroSuda AkihiroSuda added this to the v0.21.0 milestone Mar 7, 2024
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jandubois jandubois merged commit fe633d4 into lima-vm:master Mar 7, 2024
24 checks passed
@jandubois jandubois deleted the hostagent-logger branch March 7, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants