-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: stream bootloader logs #111
Conversation
@@ -378,6 +380,63 @@ func (c *Command) watchEvents(ctx context.Context) { | |||
} | |||
} | |||
|
|||
// 2024-09-10 20:16:24 WARN i.m.s.r.u.Loggers$Slf4JLogger(warn):299 - [273.... | |||
var javaLogRx = regexp.MustCompile(`^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2} \x1b\[(?:1;)?\d+m(?P<level>[A-Z]+)\x1b\[m (?P<msg>\S+ - .*)`) |
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 wonder how fragile this is. We're likely changing our logging implementation on the platform shortly, and I don't think we have any guarantees that this format won't be modified with that change.
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.
Yes, all this log stuff is definitely fragile, but it's better than nothing for now. We need to do some work in the bootloader to make all this stuff clean and easy and stable over time.
And, maybe it's reasonable to say that the current log format should be the standard going forward. It's a very common format. Note that there are actually multiple formats in these logs too, which is annoying.
What's best might be to not parse logs in abctl, but have some other summarized bootloader status.
level := pterm.Debug | ||
scanner := bufio.NewScanner(r) | ||
|
||
for scanner.Scan() { |
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.
Can we check the state of the ctx
in this loop? Break out if the context is no longer active?
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 think that's necessary. The context is passed to the k8s.StreamPodLogs
call, which returns a reader, which I would expect to close when the context is canceled.
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 proved this to myself. I couldn't think of a nice way to write an actual test for it (since it depends on the real k8s implementation), but I hacked in this code (note javaLogScanner
is not in this PR, but it's just a scanner with the java parsing wrapped in)
func (c *Command) streamPodLogs(ctx context.Context, namespace, podName, prefix string, since time.Time) error {
ctx, cancel := context.WithCancel(ctx)
go func() {
log.Printf("sleeping\n")
time.Sleep(30 * time.Second)
log.Printf("canceling\n")
cancel()
}()
r, err := c.k8s.StreamPodLogs(ctx, namespace, podName, since)
if err != nil {
return err
}
defer r.Close()
s := newJavaLogScanner(r)
for s.Scan() {
if s.line.level == "ERROR" {
pterm.Error.Printfln("%s: %s", prefix, s.line.msg)
} else {
pterm.Debug.Printfln("%s: %s", prefix, s.line.msg)
}
}
log.Printf("scanner exited: %v", s.Err())
return s.Err()
}
Stream bootloader logs to the debug output, unless they're at the "ERROR" level, then write them as error logs.