Skip to content

Conversation

@wbh1
Copy link
Contributor

@wbh1 wbh1 commented Nov 5, 2025

When using --web.route-prefix or --web.external-url with a path component, the pprof debug endpoints were not accessible. The handlers were directly passing requests to http.DefaultServeMux without adjusting the URL path to match what the automatic pprof handlers expect.

For example, with --web.route-prefix=/prometheus/alertmanager, a request to /prometheus/alertmanager/debug/pprof/ was being forwarded with the full path intact, but http.DefaultServeMux only has handlers registered for paths starting with /debug/pprof/.

This fix reconstructs the request path by extracting the subpath parameter from the route and prepending /debug to it, ensuring the path matches what http.DefaultServeMux expects. The fix also preserves trailing slashes, which are required by some pprof handlers.

Added tests to verify pprof endpoints work correctly both with and without route prefix configuration.

@wbh1 wbh1 force-pushed the claude/fix-alertmanager-pprof-routing-011CUqANv2Q8WLPTVMP995Yk branch from ea08ffa to c77837c Compare November 5, 2025 18:25
@siavashs siavashs added component/ui go Pull requests that update Go code labels Nov 11, 2025
@wbh1 wbh1 force-pushed the claude/fix-alertmanager-pprof-routing-011CUqANv2Q8WLPTVMP995Yk branch from c77837c to fba85a3 Compare November 12, 2025 22:31
When using --web.route-prefix or --web.external-url with a path component,
the pprof debug endpoints were not accessible. The handlers were directly
passing requests to http.DefaultServeMux without adjusting the URL path
to match what the pprof handlers expect.

For example, with --web.route-prefix=/prometheus/alertmanager, a request
to /prometheus/alertmanager/debug/pprof/ was being forwarded with the full
path intact, but http.DefaultServeMux only has handlers registered for
paths starting with /debug/pprof/.

This fix reconstructs the request path by extracting the subpath parameter
from the route and prepending /debug to it, ensuring the path matches what
http.DefaultServeMux expects. The fix also preserves trailing slashes which
are required by some pprof handlers.

Added unit tests to verify pprof endpoints work correctly both with and
without route prefix configuration.

Co-authored-by: Claude <[email protected]>
Signed-off-by: Will Hegedus <[email protected]>
@wbh1 wbh1 force-pushed the claude/fix-alertmanager-pprof-routing-011CUqANv2Q8WLPTVMP995Yk branch from fba85a3 to 45851d9 Compare November 12, 2025 22:34
wbh1 and others added 2 commits November 13, 2025 11:42
Co-authored-by: Siavash Safi <[email protected]>
Signed-off-by: Will Hegedus <[email protected]>
@wbh1 wbh1 force-pushed the claude/fix-alertmanager-pprof-routing-011CUqANv2Q8WLPTVMP995Yk branch from e50f8a3 to 81053db Compare November 13, 2025 16:42
@siavashs siavashs self-assigned this Nov 14, 2025
Updated based on PR comments to conform with existing testing patterns

Signed-off-by: Will Hegedus <[email protected]>
@wbh1 wbh1 force-pushed the claude/fix-alertmanager-pprof-routing-011CUqANv2Q8WLPTVMP995Yk branch from 6e16897 to b1f68ca Compare November 17, 2025 15:35
@wbh1 wbh1 requested a review from siavashs November 17, 2025 15:41
ui/web_test.go Outdated
router.ServeHTTP(w, req)

require.Equal(t, http.StatusOK, w.Code)
require.NotEqual(t, 0, w.Body.Len())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require.NotEqual(t, 0, w.Body.Len())
require.Positive(t, w.Body.Len(), "body should not be empty")

Alternatively we could replace the checks with https://pkg.go.dev/github.com/stretchr/testify/require#Assertions.HTTPBodyContains
but feel free to skip it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I think HTTPBodyContains might not be suitable here, it is best for custom handlers and not a router.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 7317f2d to use require.Contains() for checking the body, since -- as you said -- HTTPBodyContains would've required having access to the handlerFunc to pass to the test case.

@wbh1 wbh1 force-pushed the claude/fix-alertmanager-pprof-routing-011CUqANv2Q8WLPTVMP995Yk branch from 8e7f650 to 7317f2d Compare November 25, 2025 18:32
@wbh1 wbh1 requested a review from siavashs November 25, 2025 18:34
Copy link
Contributor

@siavashs siavashs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you for your contribution!

@siavashs
Copy link
Contributor

siavashs commented Dec 2, 2025

@SuperQ we can merge this one.

@SuperQ SuperQ merged commit 5362823 into prometheus:main Dec 3, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/ui go Pull requests that update Go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants