-
Notifications
You must be signed in to change notification settings - Fork 425
feat(scaling): add logs on up and down scaling #1695
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
b3c28aa to
4621450
Compare
7c6b6d7 to
a4b0f31
Compare
a4b0f31 to
e361321
Compare
|
PR updated with your suggestions. Thanks! |
scaling.go
Outdated
| // convert threads to inactive if they have been idle for too long | ||
| if thread.state.is(stateReady) && waitTime > maxThreadIdleTime.Milliseconds() { | ||
| logger.LogAttrs(context.Background(), slog.LevelDebug, "auto-converting thread to inactive", slog.Int("threadIndex", thread.threadIndex)) | ||
| logger.LogAttrs(context.Background(), slog.LevelInfo, "downscaling thread", slog.Int("thread", thread.threadIndex), slog.Int64("wait_time", waitTime), slog.Int("num_threads", len(autoScaledThreads)-1)) |
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.
Nit: shouldn't we log after having done the work? (so the -1 will not be necessary)
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.
We can! I chose the way that brings the smallest diff but totally fine with it
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.
Let's do that then.
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.
(Logging can be slow, depending on the log backend, let's do the actual work first)
e361321 to
47a8581
Compare
soyuka
left a comment
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's awesome!
47a8581 to
228ba71
Compare
|
Thanks! |
This would help better understand optimal scaling parameters per application
cc @soyuka