-
Notifications
You must be signed in to change notification settings - Fork 103
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
Speed up launcher startup #1567
Speed up launcher startup #1567
Conversation
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
c5e5f0b
checkpointer := checkups.NewCheckupLogger(logger, k) | ||
checkpointer.Once(ctx) | ||
go checkpointer.Once(ctx) |
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.
The prior way was intentional -- it was intending to get some of the identifying information into the logs before any early crashes might occur. It's fine to revisit, maybe we don't need it with all the things we've subsequently added.
Though I think this begs the question -- why does it take 5s to log, and should we add go routines downstack?
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 figured, good to get identifying info into the logs as soon as possible, but no need to block on it. And I think I remember we added this around when we were still working through the Monterey bug issues, so thought this might not be as critical anymore -- but I could be misremembering.
I will check to see which of the checkups is causing the slowness!
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.
As suspected -- it's the connectivity checkup: https://github.com/kolide/launcher/blob/main/ee/debug/checkups/connectivity.go
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 if we should just async that one check. Not sure
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.
That seemed like maybe overly complicated to coordinate an exclusion for that one checkup for the checkpoint only, but not doctor/flare
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.
Thinking about it... Maybe this simple thing is good enough for now.
We probably do want to keep this in the log checkpoint, it provides an interesting point-in-time view.
I suspect that the theoretical correct state would be to make the timer log checkpoints use a different set of debuggers than the initial startup ones. And I'm not sure it's worth doing that this moment
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.
Agreed!
Once()
can take up to 5 seconds to run -- don't block on running it