featurecontrol: log auto-gomemlimit activation at INFO - #5415
Conversation
auto-gomemlimit is not experimental in effect: it is how the Go heap learns about a container memory limit, and running without it inside a limited container is the worse choice. Logging its activation at WARN announces the safer configuration as a caution, so a deliberately configured start-up emits a warning line with nothing wrong. Log that one activation at INFO. Every other branch is left at WARN, including the unbounded-cardinality metrics flags, whose message does carry an operational caution. The message text is unchanged. Fixes prometheus#5412 Signed-off-by: brodowski <by.brodowski@pm.me>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe GOMEMLIMIT feature activation message in ChangesFeature control logging
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks @byBrodowski. By promtion I meant move it from a feature-flag to a normal config flag |
|
@TheMeier I don't think we should promote this flag, since there is a good chance this feature will it into the runtime at some point. |
SoloJacobs
left a comment
There was a problem hiding this comment.
Had a look, at it seems that we won't get this feature anytime soon:
golang/go#78912
golang/go#75164
So, promoting it might make sense. I'm not sure what the correct command-flags would be here.
@byBrodowski I don't think we will allow for some feature flags to log differently than others, consistency is important.
|
how abuot |
Narrowing #5412 to the one part that isn't a matter of taste.
@TheMeier's point in the issue is fair: whether a warning-heavy start-up log is a problem at all
depends on how you read logs, and I'm not going to argue anyone out of their preference. But the
agreement in that reply — that
auto-gomemlimitshould be promoted — doesn't depend on taste, sohere it is as a one-line change instead of a longer discussion.
auto-gomemlimitis how the Go heap learns about a container memory limit. In a memory-limitedcontainer, running without it is the worse of the two configurations. Logging its activation at
WARNtherefore announces the safer choice as a caution, which is backwards regardless of howloudly you think a log should speak.
What this changes
One
switchbranch infeaturecontrol/featurecontrol.go:The message text is unchanged. Each feature has its own branch with its own log call, so this
reaches nothing else — in particular the unbounded-cardinality metrics flags
(
alert-names-in-metrics,group-key-in-metrics) keepWARN, which I think is right: theirmessage carries a real operational cost, so a caution is the correct level for them. The
Experimental …messages also keepWARN. I've deliberately left the wider question in #5412alone; if a maintainer wants the same treatment for
utf8-strict-mode, that's a separate call andI'm happy to follow up.
Verification
go build,go testandgo veton./featurecontrol/pass with the change applied (Go 1.25,matching
go.mod).featurecontrol_test.goasserts the resulting flag state and makes noassertion about log level, so it neither needed nor received a change — the behaviour under test is
identical.
Pull Request Checklist
Which user-facing changes does this PR introduce?