Skip to content
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.

Sign up for GitHub

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

Ensure the services status page renders as HTML #1265

Closed
wants to merge 3 commits into from
Closed

Ensure the services status page renders as HTML #1265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c63e8a7
Ensure the services status page renders as HTML
simonswine Feb 22, 2022
0f6dfd6
Fix spelling of services' status
simonswine Feb 23, 2022
f8588ac
Set text/html type explicitly and do not call WriteHeader
simonswine Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ func (a *API) RegisterQueryScheduler(f *scheduler.Scheduler) {
// TODO: Refactor this code to be accomplished using the services.ServiceManager
// or a future module manager #2291
func (a *API) RegisterServiceMapHandler(handler http.Handler) {
a.indexPage.AddLink(SectionAdminEndpoints, "/services", "Service Status")
a.indexPage.AddLink(SectionAdminEndpoints, "/services", "Service's status")
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be rebased.

a.RegisterRoute("/services", handler, false, true, "GET")
}

Expand Down
7 changes: 3 additions & 4 deletions pkg/mimir/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ const tpl = `
<html>
<head>
<meta charset="UTF-8">
<title>Services Status</title>
<title>Services' status</title>
</head>
<body>
<h1>Services Status</h1>
<h1>Services' status</h1>
<p>Current time: {{ .Now }}</p>
<table border="1">
<thead>
Expand Down Expand Up @@ -55,8 +55,7 @@ func init() {
}

func (t *Mimir) servicesHandler(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Header().Set("Content-Type", "text/plain")
w.Header().Set("Content-Type", "text/html; utf=8")
Comment on lines -59 to +58
Copy link
Contributor

@colega colega Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

util.RenderHTTPResponse will try to set the content-type to application/json if that was requested, but we've already sent a content-type here. Why do we need this content-type setting at all?

Copy link
Member

Choose a reason for hiding this comment

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

RenderHTTPResponse only sets JSON content type. I think we should pass "non-JSON" content type to it, so that RenderHTTPResponse is always responsible for setting it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked the response for the /store-gateway/tenants, for example, which does not explicitly write the header, and the response contains the text/html content-type:

curl -I -XGET localhost:8080/store-gateway/tenants
HTTP/1.1 200 OK
Vary: Accept-Encoding
Date: Tue, 22 Mar 2022 09:22:40 GMT
Content-Type: text/html; charset=utf-8
Transfer-Encoding: chunked

So there's some kind of magic happening somewhere, I can recall that Golang detected the content type based on what the output looks like.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, see doc for Write method of ResponseWriter: https://pkg.go.dev/net/http#ResponseWriter

If the Header does not contain a Content-Type line, Write adds a Content-Type set to the result of passing the initial 512 bytes of written data to DetectContentType.

I don't think we should rely on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's happening here:

https://github.com/golang/go/blob/86c8075675ed74c1f404894242c26b99800f1639/src/net/http/server.go#L1408

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should rely on it.

I think it's not critical and we can expect it to work properly when we're sending HTML, but if not, sure, we can always specify it explicitly.

Anyway, my main comment was that we shouldn't set the content type here prematurely.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, my main comment was that we shouldn't set the content type here prematurely.

Agree.


svcs := make([]renderService, 0)
for mod, s := range t.ServiceMap {
Expand Down