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" | ||
|
|
@@ -138,6 +140,7 @@ depending on the nature of the reload error. | |
| BoolVar(&r.disableReporting, "disable-reporting", r.disableReporting, "Disable reporting of enabled components to Grafana.") | ||
| cmd.Flags().StringVar(&r.storagePath, "storage.path", r.storagePath, "Base directory where components can store data") | ||
| cmd.Flags().Var(&r.minStability, "stability.level", fmt.Sprintf("Minimum stability level of features to enable. Supported values: %s", strings.Join(featuregate.AllowedValues(), ", "))) | ||
| cmd.Flags().BoolVar(&r.enableAutoMemoryLimit, "memory.auto-limit", true, "Enables automatic memory limit configuration when using cgroups.") | ||
| return cmd | ||
| } | ||
|
|
||
|
|
@@ -161,9 +164,11 @@ type alloyRun struct { | |
| configFormat string | ||
| configBypassConversionErrors bool | ||
| configExtraArgs string | ||
| enableAutoMemoryLimit bool | ||
| } | ||
|
|
||
| func (fr *alloyRun) Run(configPath string) error { | ||
|
|
||
| var wg sync.WaitGroup | ||
| defer wg.Wait() | ||
|
|
||
|
|
@@ -192,6 +197,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.enableAutoMemoryLimit { | ||
| 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. |
||
| } | ||
|
|
||
|
There was a problem hiding this comment. Rather than exposing this as a new configuration option, what do you think about having this automatically be enabled for users using experimental/public-preview, and then once we get enough information on it, we can decide whether it should always be on or needs to be configurable? (The larger our API surface is the more annoying backwards compatibility can be, so avoiding new configuration options when possible is preferable for me) There was a problem hiding this comment. IE if stability is experimental automatically enable? I can also set the flag as experimental, though unsure if we have a precedent for that with alloy. There was a problem hiding this comment.
Yeah, like we would say that "auto memory limit is an experimental performance improvement" and only applies when
We don't have an existing precedent but I expect that most things will support feature gating at some point, including flags. However in this case I don't think marking the flag as experimental fully addresses my concerns, since presumably we intend for this to GA, and I prefer not adding too many more GA flags because of the increased API surface. There was a problem hiding this comment. Marked as public preview and added a helper function. |
||
| // 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.