ui: Move useful parts to httpserver#5097
Merged
SoloJacobs merged 1 commit intoprometheus:mainfrom Mar 26, 2026
Merged
Conversation
ultrotter
approved these changes
Mar 24, 2026
|
|
||
| ui.Register(router, webReload, logger) | ||
| ui.Register(router) | ||
| weboperations.Register(router, webReload) |
Contributor
There was a problem hiding this comment.
I would call it httpserver, or apiserver, or something like that? Or webops if you want to be shorter...
But apiserver should make it clear that it's the server exposing our api (via http, implementaton detail)
Contributor
Author
There was a problem hiding this comment.
This is the list of endpoints we have in this package:
GET /metrics
POST /-/reload
GET /-/healthy
GET /-/ready
GET /debug/*subpath
The api.Register function actually lives in the api package, so I find apiserver quite confusing. httpserver seems ok to. I think we could then consider moving other functionality in cmd/alertmanager/main.go into that package. I think the routing logic from ui should maybe also live in this package, since then users have an easier time migrating. So, hopefully httpserver captures these different parts well enough.
Contributor
|
If you are thinking of moving things from main there, maybe we should call
it the "alertmanager" package... ? :)
Cheers,
Guido
…On Tue, 24 Mar 2026 at 18:21, Solomon Jacobs ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In cmd/alertmanager/main.go
<#5097 (comment)>
:
> @@ -582,7 +583,8 @@ func run() int {
webReload := make(chan chan error)
- ui.Register(router, webReload, logger)
+ ui.Register(router)
+ weboperations.Register(router, webReload)
This is the list of endpoints we have in this package:
GET /metrics
POST /-/reload
GET /-/healthy
GET /-/ready
GET /debug/*subpath
The api.Register function actually lives in the api package, so I find
apiserver quite confusing. httpserver seems ok to. I think we could then
consider moving other functionality in cmd/alertmanager/main.go into that
package. I think the routing logic from ui should maybe also live in this
package, since then users have an easier time migrating. So, hopefully
httpserver captures these different parts well enough.
—
Reply to this email directly, view it on GitHub
<#5097?email_source=notifications&email_token=AD3UDVZ2TTPHXDOGRRXWNXL4SLG2LA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTIMBQGEZTMOBYGU22M4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#discussion_r2983484843>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3UDV44EPDEBPVYG453UBL4SLG2LAVCNFSM6AAAAACWSXGCS2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DAMBRGM3DQOBVGU>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Contributor
Author
|
I specifically meant code such as this // Make routePrefix default to externalURL path if empty string.
if *routePrefix == "" {
*routePrefix = amURL.Path
}
*routePrefix = "/" + strings.Trim(*routePrefix, "/")
logger.Debug("route prefix", "routePrefix", *routePrefix)
router := route.New().WithInstrumentation(instrumentHandler)
if *routePrefix != "/" {
router.Get("/", func(w http.ResponseWriter, r *http.Request) {
http.Redirect(w, r, *routePrefix, http.StatusFound)
})
router = router.WithPrefix(*routePrefix)
}But splitting up the remainder of |
SoloJacobs
force-pushed
the
move-functionality
branch
from
d6d409e to
026d9da
Compare
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAuxiliary HTTP endpoints (metrics, reload, health/readiness, debug/pprof) were moved from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant HTTPServer as httpserver.Handler
participant Main as cmd/main
participant Worker as reloadWorker
Client->>Router: POST /-/reload
Router->>HTTPServer: route to reload handler
HTTPServer->>Main: send chan error (respCh) on reloadCh
Main->>Worker: perform reload (worker reads respCh)
Worker-->>Main: send error or nil on respCh
Main-->>HTTPServer: respCh receives result
HTTPServer-->>Client: 200 OK or 500 with error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
SoloJacobs
force-pushed
the
move-functionality
branch
from
026d9da to
6e87eb6
Compare
SoloJacobs
changed the title
weboperationshttpserver
In the future, we want to remove the ui build artifacts from the repository. This will break the usages of ```sh go get github.com/prometheus/alertmanager/ui ``` That is exactly as expected, since the ui can't function correctly without the ui artefacts. This change attempts to make the transition a bit less painful. Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
SoloJacobs
force-pushed
the
move-functionality
branch
from
6e87eb6 to
b4475f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/web.go (1)
27-28: Make the split contract explicit here.
Registernow only mounts static UI assets, but the exported name/comment still read like the old all-in-one entrypoint. A tighter doc comment (or a more specific name such asRegisterAssets) would make it easier for embedders to spot the companionhttpserver.Registercall now required incmd/alertmanager/main.go, Line 586-587.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/web.go` around lines 27 - 28, The exported Register(r *route.Router) currently only mounts static UI assets but its docstring still implies it registers the whole web entrypoint; update the comment to explicitly state that Register only mounts static UI assets (or rename the function to RegisterAssets) and mention in the comment that embedders must call the companion httpserver.Register entrypoint in main to register dynamic HTTP handlers; reference the Register function and route.Router (and the companion httpserver.Register) so readers can find and wire both pieces together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@httpserver/httpserver.go`:
- Around line 31-36: The POST /-/reload handler currently does two potentially
blocking operations (sending errc into reloadCh and waiting on <-errc). Change
the handler to create a buffered reply channel (e.g., errc := make(chan error,
1)) and use select when sending to reloadCh and when waiting for the reply so
you also listen on req.Context().Done(); on context cancellation return early
with an error response. Update the anonymous handler function (the closure
passed to r.Post) to perform both the send to reloadCh and the receive from errc
via select statements that respect req.Context().Done() to avoid indefinite
blocking.
---
Nitpick comments:
In `@ui/web.go`:
- Around line 27-28: The exported Register(r *route.Router) currently only
mounts static UI assets but its docstring still implies it registers the whole
web entrypoint; update the comment to explicitly state that Register only mounts
static UI assets (or rename the function to RegisterAssets) and mention in the
comment that embedders must call the companion httpserver.Register entrypoint in
main to register dynamic HTTP handlers; reference the Register function and
route.Router (and the companion httpserver.Register) so readers can find and
wire both pieces together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01bdf608-ac31-4351-b214-c29785df8c11
📒 Files selected for processing (5)
cmd/alertmanager/main.gohttpserver/httpserver.gohttpserver/httpserver_test.goui/web.goui/web_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacks on top of #5092
We want to allow users to still be able to import parts of the
ui, just not the ones depending onjs. Ideas for naming welcome. I also considered changing the name ofRegisterto make it a bit more obvious that this function now does something different, but decided to only change the signature in the end, since we don't plan to expose it.Pull Request Checklist
Which user-facing changes does this PR introduce?
Summary by CodeRabbit
Refactor
Tests