-
Notifications
You must be signed in to change notification settings - Fork 9.8k
test(cmd): add test for GOGC setting #15238
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
cc @SuperQ |
some cases are failing on windows
I'll take a look |
a260a90
to
8b0e413
Compare
cc @SuperQ when you have some time. |
test failure is due to #16473 |
…exists Fixes: prometheus#16334 Related to: - prometheus#15238 - prometheus#16052 Currently, when the GOGC environment variable is set -- and no `runtime` block is set in the Prometheus config file -- it is ignored and the default value of 75% is always used. However, if there is an empty runtime block (e.g. `runtime: {}`), _then_ the GOGC environment variable is checked. This PR changes this behavior to consistently check and use the GOGC environment variable when it is set (unless the `gogc` field is set in the `runtime` block of the loaded config file, in which case it still gives that precedence). Co-authored-by: Adam Rambo <arambo@protonmail.com> Signed-off-by: Will Hegedus <whegedus@akamai.com>
df4d44a
to
23be5ec
Compare
As suggested in prometheus#14176 (comment) Signed-off-by: machine424 <ayoubmrini424@gmail.com>
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.
It's fine, but I think it could be a bit simpler.
// Wait for the /metrics endpoint to be ready. | ||
require.Eventually(t, func() bool { | ||
r, err = http.Get(fmt.Sprintf("http://127.0.0.1:%d/metrics", port)) | ||
if err != nil { | ||
return false | ||
} | ||
return r.StatusCode == http.StatusOK | ||
}, 5*time.Second, 50*time.Millisecond) |
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.
Alternatively, look at Go's internal metric /gc/gogc:percent
.
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.
how can we get that from a running process?
(the prom metric is based on /gc/gogc:percent)
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.
(I wanted to run it as a "blackbox" becaue it's simple and because of the gogc logic in main.go itself)
…exists Fixes: prometheus#16334 Related to: - prometheus#15238 - prometheus#16052 Currently, when the GOGC environment variable is set -- and no `runtime` block is set in the Prometheus config file -- it is ignored and the default value of 75% is always used. However, if there is an empty runtime block (e.g. `runtime: {}`), _then_ the GOGC environment variable is checked. This PR changes this behavior to consistently check and use the GOGC environment variable when it is set (unless the `gogc` field is set in the `runtime` block of the loaded config file, in which case it still gives that precedence). Co-authored-by: Adam Rambo <arambo@protonmail.com> Signed-off-by: Will Hegedus <whegedus@akamai.com>
As suggested in #14176 (comment)