Add details for setting memory limit automatically. #655
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,9 @@ import ( | |
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "github.com/KimMachineGun/automemlimit/memlimit" | ||
| "io/fs" | ||
| "log/slog" | ||
| "os" | ||
| "os/signal" | ||
| "path/filepath" | ||
|
|
@@ -164,6 +166,7 @@ type alloyRun struct { | |
| } | ||
|
|
||
| func (fr *alloyRun) Run(configPath string) error { | ||
|
|
||
| var wg sync.WaitGroup | ||
| defer wg.Wait() | ||
|
|
||
|
|
@@ -192,6 +195,12 @@ func (fr *alloyRun) Run(configPath string) error { | |
|
|
||
| level.Info(l).Log("boringcrypto enabled", boringcrypto.Enabled) | ||
|
|
||
| // Set the memory limit, this will honor GOMEMLIMIT if set | ||
| // If there is a cgroup will follow that | ||
| if fr.minStability == featuregate.StabilityPublicPreview { | ||
| memlimit.SetGoMemLimitWithOpts(memlimit.WithLogger(slog.Default())) | ||
|
There was a problem hiding this comment. I think this should use There was a problem hiding this comment. I tried that. The format isnt set by this point so it errors, its possible we could default to one? There was a problem hiding this comment. I could always move it after the logger is resolved but that feels like an odd time to set memory limits. There was a problem hiding this comment. That seems like a bug in the logging code, it's supposed to buffer logs until the controller evaluates, but it only seems to do that when you're using the go-kit interface and not the slog.Handler interface. I think it might be worth fixing that so we can use the right logger here. I don't think defaulting to a format is the right move, since when we used to do this, users complained that some of the early log lines weren't in the format they were expecting and parsing the log lines at ingestion time was failing. There was a problem hiding this comment.
Since we've fixed issues about this in the past I'd say not using the right logger would be a regression here. IMO I think we need to do something to use the right logger, either fixing the buffering issue with the slog.Handler implementation or moving this later in evaluation at least temporarily (I agree it's weird for setting the memory limit to not happen as early as possible) There was a problem hiding this comment. I am going to fork this out into its own PR, making the logger work with slog is bigger than this change. There was a problem hiding this comment. @mattdurham Is creating a PR to fix the logger still on your radar? Given this was merged first, there's a known regression in main now, and we're running out of time for 1.1 (RC gets cut this Thursday). If we can't fix this by Thursday we'll have to discuss this feature skipping 1.1 or putting out the release with a known regression. There was a problem hiding this comment. It is, wrapping up testing now, it ended up being slightly more complicated than expected. |
||
| } | ||
|
|
||
| // Enable the profiling. | ||
| setMutexBlockProfiling(l) | ||
|
|
||
|
|
||
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.
IMO I think we should find a way to document this somewhere just so people know which improvements lower stability levels include. I don't know what page it should be on, but I'll leave that up to you since I'm not too opinionated.