Skip to content

Conversation

machine424
Copy link
Member

As suggested in #14176 (comment)

@machine424
Copy link
Member Author

cc @SuperQ
I think they're some cases we didn't consider: We're setting gogc to 0 when the config file is empty (see 2 first cases) as the unmarshalling code is never executed, this should be rare for a scraper Prom but not for a Prom used as a RW receiver e.g.
Especially that in such cases, we're ignoring the GOGC env var when set.

@machine424
Copy link
Member Author

machine424 commented Mar 27, 2025

it should be ready now @SuperQ
we can uncomment the other cases later when #16334 is fixed

@machine424
Copy link
Member Author

some cases are failing on windows

 === NAME  TestRuntimeGOGCConfig/empty_config_file
    main_test.go:780: 
        	Error Trace:	D:/a/prometheus/prometheus/cmd/prometheus/main_test.go:780
        	Error:      	Not equal: 
        	            	expected: 75
        	            	actual  : 100
        	Test:       	TestRuntimeGOGCConfig/empty_config_file
=== NAME  TestRuntimeGOGCConfig/incomplete_runtime_block
    main_test.go:780: 
        	Error Trace:	D:/a/prometheus/prometheus/cmd/prometheus/main_test.go:780
        	Error:      	Not equal: 
        	            	expected: 75
        	            	actual  : 100
        	Test:       	TestRuntimeGOGCConfig/incomplete_runtime_block

I'll take a look

@github-actions github-actions bot removed the stale label Mar 31, 2025
@machine424 machine424 force-pushed the gogc-test branch 3 times, most recently from a260a90 to 8b0e413 Compare April 23, 2025 14:28
@machine424
Copy link
Member Author

cc @SuperQ when you have some time.

@machine424
Copy link
Member Author

test failure is due to #16473

@machine424 machine424 requested review from SuperQ and removed request for SuperQ April 24, 2025 05:00
@machine424 machine424 requested a review from bboreham May 1, 2025 15:42
wbh1 added a commit to wbh1/prometheus that referenced this pull request May 5, 2025
…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>
@machine424 machine424 force-pushed the gogc-test branch 2 times, most recently from df4d44a to 23be5ec Compare May 6, 2025 05:48
As suggested in prometheus#14176 (comment)

Signed-off-by: machine424 <ayoubmrini424@gmail.com>
Copy link
Member

@bboreham bboreham left a 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.

Comment on lines +752 to +759
// 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)
Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

@machine424 machine424 May 6, 2025

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)

@machine424 machine424 merged commit ca70ed4 into prometheus:main May 6, 2025
27 checks passed
wbh1 added a commit to wbh1/prometheus that referenced this pull request May 12, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants