Refactor query frontend to return prometheus error response #5811

yeya24 · 2024-03-13T07:49:04Z

What this PR does:

Change QFE to return Prometheus type error response so that the response can be parsed directly by Grafana.

Which issue(s) this PR fixes:
Fixes #5324

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

yeya24 · 2024-03-14T06:55:17Z

@alanprot @friedrichg Can you help review the pr and see if the error response code makes sense?

@emanlodovice @rajagopalanand Can you help review the ruler part since this pr refactors the Ruler response API?

yeya24 · 2024-03-14T06:59:36Z

pkg/ruler/api.go

@@ -163,19 +117,19 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
 	userID, err := tenant.TenantID(req.Context())
 	if err != nil || userID == "" {
 		level.Error(logger).Log("msg", "error extracting org id from context", "err", err)
-		respondError(logger, w, "no valid org id found")


This error response got changed from 5xx to 4xx, is it ok?

I think if you run cortex without an auth service in front this change would make sense because its the client's fault for not setting the header, but if there is another service that sets the header then we will be returning 4xx when that service is not working fine. My vote is keeping the 5xx just because I think its more common that it's another service that sets the header and not the client itself.

We should only return 5xx if it is a Cortex server side issue. The case you mentioned is Proxy deployed together with Cortex server. However, Proxy can be something deployed at Client side, too. But both ways, the proxy is still a client to Cortex.

Anyway, I think this code is only used when Cortex Server Auth is not enabled. When auth is enabled, there is an auth middleware before which returns 401 when there is no valid Org ID. https://github.com/cortexproject/cortex/blob/master/vendor/github.com/weaveworks/common/middleware/http_auth.go#L12

These errors catchs are weird, why would it not have the tenant id here?

I think the thing to remember here is that rulers can also send requests to other rulers, so the question is what happens if one ruler returns 4xx to another, instead of 5xx.

When one Ruler calls other Rulers to fetch rules for example, those are gRPC calls and does not go thru API layer. It calls Rules(ctx context.Context, in *RulesRequest). So it is OK I think

yeya24 · 2024-03-14T06:59:48Z

pkg/ruler/api.go

@@ -275,7 +229,7 @@ func (a *API) PrometheusAlerts(w http.ResponseWriter, req *http.Request) {
 	userID, err := tenant.TenantID(req.Context())
 	if err != nil || userID == "" {
 		level.Error(logger).Log("msg", "error extracting org id from context", "err", err)
-		respondError(logger, w, "no valid org id found")


This error response got changed from 5xx to 4xx, is it ok?

yeya24 · 2024-03-14T07:00:03Z

pkg/ruler/api.go

@@ -433,7 +387,7 @@ func (a *API) ListRules(w http.ResponseWriter, req *http.Request) {

 	userID, namespace, _, err := parseRequest(req, false, false)
 	if err != nil {
-		respondError(logger, w, err.Error())


This error response got changed from 5xx to 4xx, is it ok?

yeya24 · 2024-03-14T07:00:14Z

pkg/ruler/api.go

@@ -466,7 +420,7 @@ func (a *API) GetRuleGroup(w http.ResponseWriter, req *http.Request) {
 	logger := util_log.WithContext(req.Context(), a.logger)
 	userID, namespace, groupName, err := parseRequest(req, true, true)
 	if err != nil {
-		respondError(logger, w, err.Error())


This error response got changed from 5xx to 4xx, is it ok?

yeya24 · 2024-03-14T07:00:21Z

pkg/ruler/api.go

@@ -488,7 +442,7 @@ func (a *API) CreateRuleGroup(w http.ResponseWriter, req *http.Request) {
 	logger := util_log.WithContext(req.Context(), a.logger)
 	userID, namespace, _, err := parseRequest(req, true, false)
 	if err != nil {
-		respondError(logger, w, err.Error())


This error response got changed from 5xx to 4xx, is it ok?

yeya24 · 2024-03-14T07:00:25Z

pkg/ruler/api.go

@@ -570,7 +524,7 @@ func (a *API) DeleteNamespace(w http.ResponseWriter, req *http.Request) {

 	userID, namespace, _, err := parseRequest(req, true, false)
 	if err != nil {
-		respondError(logger, w, err.Error())


This error response got changed from 5xx to 4xx, is it ok?

yeya24 · 2024-03-14T07:00:31Z

pkg/ruler/api.go

@@ -592,7 +546,7 @@ func (a *API) DeleteRuleGroup(w http.ResponseWriter, req *http.Request) {

 	userID, namespace, groupName, err := parseRequest(req, true, true)
 	if err != nil {
-		respondError(logger, w, err.Error())


This error response got changed from 5xx to 4xx, is it ok?

yeya24 · 2024-03-14T07:22:31Z

pkg/util/api/response.go

+	Data      interface{}  `json:"data,omitempty"`
+	ErrorType v1.ErrorType `json:"errorType,omitempty"`
+	Error     string       `json:"error,omitempty"`
+	Warnings  []string     `json:"warnings,omitempty"`


Having omitempty to be consistent with Prometheus API behavior.

rajagopalanand · 2024-03-14T16:16:32Z

LGTM

friedrichg

query frontend looks fine, not sure about ruler

friedrichg · 2024-03-14T15:54:23Z

pkg/ruler/api.go

@@ -163,19 +117,19 @@ func (a *API) PrometheusRules(w http.ResponseWriter, req *http.Request) {
 	userID, err := tenant.TenantID(req.Context())
 	if err != nil || userID == "" {
 		level.Error(logger).Log("msg", "error extracting org id from context", "err", err)
-		respondError(logger, w, "no valid org id found")


These errors catchs are weird, why would it not have the tenant id here?

I think the thing to remember here is that rulers can also send requests to other rulers, so the question is what happens if one ruler returns 4xx to another, instead of 5xx.

alanprot · 2024-03-15T17:12:21Z

pkg/frontend/transport/handler.go

+	headers := w.Header()
+	for k, values := range additionalHeaders {
+		for _, value := range values {
+			headers.Set(k, value)
+		}
+	}
 	resp, ok := httpgrpc.HTTPResponseFromError(err)
 	if ok {
-		for k, values := range additionalHeaders {
-			resp.Headers = append(resp.Headers, &httpgrpc.Header{Key: k, Values: values})
+		code := int(resp.Code)
+		var errTyp v1.ErrorType
+		switch resp.Code {
+		case http.StatusBadRequest, http.StatusRequestEntityTooLarge:
+			errTyp = v1.ErrBadData
+		case StatusClientClosedRequest:
+			errTyp = v1.ErrCanceled
+		case http.StatusGatewayTimeout:
+			errTyp = v1.ErrTimeout
+		case http.StatusUnprocessableEntity:
+			errTyp = v1.ErrExec
+		case int32(codes.PermissionDenied):
+			// Convert gRPC status code to HTTP status code.
+			code = http.StatusUnprocessableEntity
+			errTyp = v1.ErrBadData
+		default:
+			errTyp = v1.ErrServer
 		}


There are some places that seems the response is already in the prometheus format? (i think when is returned by the querier itself, not the query frontend?) https://github.com/cortexproject/cortex/blob/master/pkg/querier/tripperware/queryrange/query_range.go#L269
In this case u think make sense to just "pipe" it?

Im afraid the message in this case is another prometheus response serialized. haha

Nit: Does it make sense to move this method now to be in the util/response.go?

I can move the method to util.

In this case u think make sense to just "pipe" it?

Do you have any suggestion, how I can get the error and know it is already Prometheus formatted? I have to deserialize the error in this case?

Fixed in latest commit

emanlodovice · 2024-03-27T21:42:24Z

pkg/util/api/response.go

+
+type Response struct {
+	Status    string       `json:"status"`
+	Data      interface{}  `json:"data,omitempty"`


This could unexpected break some consumer code because now all 5xx responses will not have a data key. Maybe we can move this on a separate PR?

Thanks for the comment. Sure I will remove this change.

Fixed in latest commit

Signed-off-by: Ben Ye <[email protected]>

friedrichg

Good tests

pull-request-size bot added the size/L label Mar 13, 2024

yeya24 force-pushed the refactor-query-frontend-api-response branch 3 times, most recently from 7e750b4 to 5fed352 Compare March 13, 2024 23:18

yeya24 force-pushed the refactor-query-frontend-api-response branch from 5fed352 to b94715c Compare March 14, 2024 06:56

yeya24 commented Mar 14, 2024

View reviewed changes

yeya24 force-pushed the refactor-query-frontend-api-response branch 2 times, most recently from ba3687e to 57d4a0a Compare March 14, 2024 07:21

yeya24 commented Mar 14, 2024

View reviewed changes

yeya24 force-pushed the refactor-query-frontend-api-response branch from 57d4a0a to 254527f Compare March 14, 2024 07:26

friedrichg reviewed Mar 14, 2024

View reviewed changes

alanprot reviewed Mar 15, 2024

View reviewed changes

emanlodovice reviewed Mar 27, 2024

View reviewed changes

yeya24 force-pushed the refactor-query-frontend-api-response branch from 254527f to 0ba10db Compare April 10, 2024 02:51

pull-request-size bot added size/XL and removed size/L labels Apr 11, 2024

yeya24 added 3 commits April 10, 2024 23:15

refactor query frontend to return prometheus error response

2fc08ee

Signed-off-by: Ben Ye <[email protected]>

stash

f483eb8

Signed-off-by: Ben Ye <[email protected]>

add test

fa57317

Signed-off-by: Ben Ye <[email protected]>

yeya24 force-pushed the refactor-query-frontend-api-response branch from 736e4fd to fa57317 Compare April 11, 2024 06:15

fix lint

3b7955a

Signed-off-by: Ben Ye <[email protected]>

friedrichg approved these changes Apr 11, 2024

View reviewed changes

alanprot approved these changes Apr 22, 2024

View reviewed changes

alanprot merged commit 6bca2d5 into cortexproject:master Apr 22, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor query frontend to return prometheus error response #5811

Refactor query frontend to return prometheus error response #5811

yeya24 commented Mar 13, 2024 •

edited

Loading

yeya24 commented Mar 14, 2024

yeya24 Mar 14, 2024

emanlodovice Mar 14, 2024

yeya24 Mar 14, 2024

friedrichg Mar 14, 2024

rajagopalanand Mar 14, 2024

yeya24 Mar 14, 2024

yeya24 Mar 14, 2024

yeya24 Mar 14, 2024

yeya24 Mar 14, 2024

yeya24 Mar 14, 2024

yeya24 Mar 14, 2024

yeya24 Mar 14, 2024

rajagopalanand commented Mar 14, 2024

friedrichg left a comment

friedrichg Mar 14, 2024

alanprot Mar 15, 2024

yeya24 Mar 15, 2024

yeya24 Apr 11, 2024

emanlodovice Mar 27, 2024

yeya24 Mar 27, 2024

yeya24 Apr 11, 2024

friedrichg left a comment

Refactor query frontend to return prometheus error response #5811

Refactor query frontend to return prometheus error response #5811

Conversation

yeya24 commented Mar 13, 2024 • edited Loading

yeya24 commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajagopalanand commented Mar 14, 2024

friedrichg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedrichg left a comment

Choose a reason for hiding this comment

yeya24 commented Mar 13, 2024 •

edited

Loading