Add details for setting memory limit automatically. #655

mattdurham · 2024-04-23T20:14:46Z

PR Description

This will set the automemory limit if cgroups exist and the setting is enabled, the setting defaults to true. If there is a GOMEMLIMIT set already it will not overwrite it. If there are no cgroups then no limit will be applied.

Which issue(s) this PR fixes

Fixes #651

Notes to the Reviewer

PR Checklist

CHANGELOG.md updated
Documentation added

spartan0x117

LGTM!

spartan0x117 · 2024-04-23T20:33:49Z

internal/alloycli/cmd_run.go

+	// Set the memory limit, this will honor GOMEMLIMIT if set
+	// If there is a cgroup will follow that
+	if fr.enableAutoMemoryLimit {
+		memlimit.SetGoMemLimitWithOpts()


Is it possible to pass in a compatible logger here? If so, some of the logs might be helpful (e.g. if GOMEMLIMIT is already set it won't honor the cgroups settings, as well as confirmation of the limit)

Was thinking something like from the example:

memlimit.SetGoMemLimitWithOpts( memlimit.WithRatio(0.9), memlimit.WithProvider(memlimit.FromCgroup), memlimit.WithLogger(slog.Default()), // Can we pass a logger here? )

I can but I have to move it til after our logging is fully initialized, which likely isn't the end of the world its just later in the process to pass down the bool flag.

Added the slog.Default, its...a bit different looking but likely works for ease of use.

2024/04/23 17:01:27 INFO memory is not limited, skipping: %v package=github.com/KimMachineGun/automemlimit/memlimit !BADKEY="memory is not limited"

docs/sources/reference/cli/run.md

internal/alloycli/cmd_run.go

Co-authored-by: Mischa Thompson <mischabear@gmail.com>

…_memlimit

rfratto · 2024-04-24T17:47:09Z

internal/alloycli/cmd_run.go

+	// 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()))
+	}
+


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)

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.

IE if stability is experimental automatically enable?

Yeah, like we would say that "auto memory limit is an experimental performance improvement" and only applies when --stability.level=experimental. This is something I expect we'll be doing at some frequency, including for the eventual controller refactor.

I can also set the flag as experimental

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.

Marked as public preview and added a helper function.

rfratto

LGTM following some feedback

rfratto · 2024-04-24T19:45:28Z

docs/sources/reference/cli/run.md

@@ -164,6 +164,7 @@ original configuration.
 Include `--config.extra-args` to pass additional command line flags from the original format to the converter.
 Refer to [alloy convert][] for more details on how `extra-args` work.

+


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.

rfratto · 2024-04-24T19:48:18Z

internal/alloycli/cmd_run.go

+	// Set the memory limit, this will honor GOMEMLIMIT if set
+	// If there is a cgroup will follow that
+	if fr.minStability.AtLeast(featuregate.StabilityPublicPreview) {
+		memlimit.SetGoMemLimitWithOpts(memlimit.WithLogger(slog.Default()))


I think this should use slog.New(l.Handler()) here instead of slog.Default(); the controller's logger implements the slog.Handler interface. I think this is important so that log entries are emitted in the proper format if the user requests json vs the default.

I tried that. The format isnt set by this point so it errors, its possible we could default to one?

I could always move it after the logger is resolved but that feels like an odd time to set memory limits.

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.

I think it might be worth fixing that so we can use the right logger here.

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)

I am going to fork this out into its own PR, making the logger work with slog is bigger than this change.

@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.

It is, wrapping up testing now, it ended up being slightly more complicated than expected.

internal/featuregate/featuregate.go

CHANGELOG.md

* Add details for setting memory limit automatically. * fix typo * Update docs/sources/reference/cli/run.md Co-authored-by: Mischa Thompson <mischabear@gmail.com> * Update internal/alloycli/cmd_run.go Co-authored-by: Mischa Thompson <mischabear@gmail.com> * add logging * Mark as public preview. * Mark as public preview. * pr feedback * use default logger --------- Co-authored-by: Mischa Thompson <mischabear@gmail.com>

mattdurham added 2 commits April 23, 2024 16:10

Add details for setting memory limit automatically.

032281b

fix typo

b002fd5

mattdurham requested a review from clayton-cornell as a code owner April 23, 2024 20:14

mattdurham requested a review from spartan0x117 April 23, 2024 20:18

mattdurham assigned tpaschalis and spartan0x117 Apr 23, 2024

spartan0x117 approved these changes Apr 23, 2024

View reviewed changes

mattdurham and others added 4 commits April 23, 2024 16:46

Update docs/sources/reference/cli/run.md

fc54278

Co-authored-by: Mischa Thompson <mischabear@gmail.com>

Update internal/alloycli/cmd_run.go

c45082d

Co-authored-by: Mischa Thompson <mischabear@gmail.com>

add logging

aaa5be2

Merge remote-tracking branch 'origin/auto_set_memlimit' into auto_set…

6cfbadd

…_memlimit

rfratto reviewed Apr 24, 2024

View reviewed changes

rfratto added the backport-to-agent:no PR should NOT be backported to the agent repo. label Apr 24, 2024

mattdurham added 3 commits April 24, 2024 15:32

Mark as public preview.

532893c

Mark as public preview.

782cb59

merge main

f4464d6

rfratto approved these changes Apr 24, 2024

View reviewed changes

mattdurham added 3 commits April 24, 2024 16:00

pr feedback

d484362

use default logger

0e9756a

Merge branch 'main' into auto_set_memlimit

d7cc888

mattdurham enabled auto-merge (squash) April 25, 2024 19:37

mattdurham merged commit 148f6a3 into main Apr 25, 2024
13 checks passed

mattdurham deleted the auto_set_memlimit branch April 25, 2024 19:40

rfratto mentioned this pull request May 13, 2024

Add automemlimit grafana/agent#6525

Closed

1 task

aerfio mentioned this pull request May 16, 2024

Add GOMAXPROCS and GOMEMLIMIT env vars to alloy container in the Helm chart #578

Closed

4 tasks

github-actions bot added the frozen-due-to-age label May 31, 2024

github-actions bot locked as resolved and limited conversation to collaborators May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add details for setting memory limit automatically. #655

Add details for setting memory limit automatically. #655

mattdurham commented Apr 23, 2024

spartan0x117 left a comment

spartan0x117 Apr 23, 2024

spartan0x117 Apr 23, 2024

mattdurham Apr 23, 2024

mattdurham Apr 23, 2024

rfratto Apr 24, 2024 •

edited

Loading

mattdurham Apr 24, 2024

rfratto Apr 24, 2024 •

edited

Loading

mattdurham Apr 24, 2024

rfratto left a comment

rfratto Apr 24, 2024

rfratto Apr 24, 2024

mattdurham Apr 24, 2024

mattdurham Apr 24, 2024

rfratto Apr 24, 2024

rfratto Apr 24, 2024

mattdurham Apr 25, 2024

rfratto Apr 29, 2024

mattdurham Apr 29, 2024

		@@ -164,6 +164,7 @@ original configuration.
		Include `--config.extra-args` to pass additional command line flags from the original format to the converter.
		Refer to [alloy convert][] for more details on how `extra-args` work.

Add details for setting memory limit automatically. #655

Add details for setting memory limit automatically. #655

Conversation

mattdurham commented Apr 23, 2024

PR Description

Which issue(s) this PR fixes

Notes to the Reviewer

PR Checklist

spartan0x117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfratto Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfratto Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfratto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfratto Apr 24, 2024 •

edited

Loading

rfratto Apr 24, 2024 •

edited

Loading