Allow server to return HTTP 4xx errors #7045

duricanikolic · 2024-01-03T21:32:52Z

What this PR does

This PR initializes dskit's server.Server and httpgrpc.Server in such a way that they return errors with HTTP status code 4xx, and therefore allow the latter to appear as status_code label in the request duration metrics.

Which issue(s) this PR fixes or relates to

Part of #1830

Checklist

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

dimitarvdimitrov

Do I understand correctly that this change will also force the httpgrpc server to return an error and ignore the httpgrpc response when the code is 4xx?

There is this place in the query-fronted which needs an http response (taken from http grpc) in order to propagate the error back to the user.

mimir/pkg/frontend/querymiddleware/codec.go

Lines 406 to 417 in 9fd2b19

    
           switch r.StatusCode { 
        
           case http.StatusServiceUnavailable: 
        
           	return nil, apierror.New(apierror.TypeUnavailable, string(mustReadResponseBody(r))) 
        
           case http.StatusTooManyRequests: 
        
           	return nil, apierror.New(apierror.TypeTooManyRequests, string(mustReadResponseBody(r))) 
        
           case http.StatusRequestEntityTooLarge: 
        
           	return nil, apierror.New(apierror.TypeTooLargeEntry, string(mustReadResponseBody(r))) 
        
           default: 
        
           	if r.StatusCode/100 == 5 { 
        
           		return nil, apierror.New(apierror.TypeInternal, string(mustReadResponseBody(r))) 
        
           	} 
        
           }

what's referred to as the r response in this snippet should be what the querier computes here

mimir/pkg/querier/worker/scheduler_processor.go

Line 247 in 5d0316a

response, err := sp.handler.Handle(ctx, request)

If my understanding is correct, then we will convert all 4xx promQL errors into 4xx errors. At the same time no tests are failing, so maybe I'm missing something

dimitarvdimitrov · 2024-01-04T08:43:47Z

pkg/alertmanager/multitenant.go

@@ -400,7 +400,7 @@ func createMultitenantAlertmanager(cfg *MultitenantAlertmanagerConfig, fallbackC
 		return nil, errors.Wrap(err, "failed to initialize Alertmanager's ring")
 	}

-	am.grpcServer = server.NewServer(&handlerForGRPCServer{am: am})
+	am.grpcServer = server.NewServer(&handlerForGRPCServer{am: am}, []server.Option{server.WithReturn4XXErrors}...)


isn't it enough to do this?

Suggested change

am.grpcServer = server.NewServer(&handlerForGRPCServer{am: am}, []server.Option{server.WithReturn4XXErrors}...)

am.grpcServer = server.NewServer(&handlerForGRPCServer{am: am}, server.WithReturn4XXError)

same applies to the other invocations

You are right, I will change this. Thank you

CHANGELOG.md

dimitarvdimitrov · 2024-01-04T09:02:21Z

is this PR a duplicate of #6983?

pracucci · 2024-01-04T10:19:48Z

Do I understand correctly that this change will also force the httpgrpc server to return an error and ignore the httpgrpc response when the code is 4xx?

Yes, that's correct. I've done an analysis of possible side effects here.

There is this place in the query-fronted which needs an http response (taken from http grpc) in order to propagate the error back to the user.

Going to take a look now.

pracucci · 2024-01-04T10:32:15Z

what's referred to as the r response in this snippet should be what the querier computes here mimir/pkg/querier/worker/scheduler_processor.go Line 247 in 5d0316a
response, err := sp.handler.Handle(ctx, request)

@dimitarvdimitrov That's correct, but I live it's handled correctly there. The error (which starting from this PR we'll get for 4xx errors too) is converted back to an HTTP response. When using httpgrpc, the HTTP response body is stored in status.Details, so HTTPResponseFromError() coverts it back to an HTTP response:

mimir/pkg/querier/worker/scheduler_processor.go

Lines 247 to 257 in fb7f57b

    
           response, err := sp.handler.Handle(ctx, request) 
        
           if err != nil { 
        
           	var ok bool 
        
           	response, ok = httpgrpc.HTTPResponseFromError(err) 
        
           	if !ok { 
        
           		response = &httpgrpc.HTTPResponse{ 
        
           			Code: http.StatusInternalServerError, 
        
           			Body: []byte(err.Error()), 
        
           		} 
        
           	} 
        
           }

duricanikolic · 2024-01-04T10:45:08Z

is this PR a duplicate of #6983?

It is, and #6983 will be closed.

dimitarvdimitrov · 2024-01-04T12:13:05Z

Do I understand correctly that this change will also force the httpgrpc server to return an error and ignore the httpgrpc response when the code is 4xx?

Yes, that's correct. I've done an analysis of possible side effects here.

I didn't check the linked issue before. The analysis there makes sense to me

dimitarvdimitrov

LGTM

pracucci · 2024-01-04T13:18:07Z

pkg/ruler/remotequerier.go

@@ -183,7 +183,9 @@ func (q *RemoteQuerier) Read(ctx context.Context, query *prompb.Query) (*prompb.

 	resp, err := q.client.Handle(ctx, &req)
 	if err != nil {
-		level.Warn(log).Log("msg", "failed to perform remote read", "err", err, "qs", query)
+		if code := grpcutil.ErrorToStatusCode(err); code/100 == 5 {


I would do the opposite check, because we may get a code < 100 in case of a non HTTP error.

Suggested change

if code := grpcutil.ErrorToStatusCode(err); code/100 == 5 {

if code := grpcutil.ErrorToStatusCode(err); code/100 != 4 {

pracucci · 2024-01-04T13:18:19Z

pkg/ruler/remotequerier.go

@@ -227,7 +229,9 @@ func (q *RemoteQuerier) query(ctx context.Context, query string, ts time.Time, l

 	resp, err := q.sendRequest(ctx, &req, logger)
 	if err != nil {
-		level.Warn(logger).Log("msg", "failed to remotely evaluate query expression", "err", err, "qs", query, "tm", ts)
+		if code := grpcutil.ErrorToStatusCode(err); code/100 == 5 {


Same here

pracucci · 2024-01-04T13:19:11Z

pkg/ruler/remotequerier_test.go

+		error4xx = status.Error(http.StatusUnprocessableEntity, "this is a 4xx error")
+		error5xx = status.Error(http.StatusInternalServerError, "this is a 5xx error")
+	)
+	testCases := map[string]struct {


Add a test case returning a non httpgrpc error (e.g. errors.New("mocked error")). We expect the log.

pracucci

LGTM (modulo a couple of minor comments)

…on metrics Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic self-assigned this Jan 3, 2024

dimitarvdimitrov reviewed Jan 4, 2024

View reviewed changes

duricanikolic force-pushed the yuri/allow-4xx-errors branch from c2297fd to 183a8f9 Compare January 4, 2024 10:41

duricanikolic force-pushed the yuri/allow-4xx-errors branch from 2f47c23 to 7dd99d5 Compare January 4, 2024 11:30

duricanikolic changed the title ~~Allow reporting HTTP 4xx codes in status_code label of request duration metrics~~ Allow server to return HTTP 4xx errors Jan 4, 2024

duricanikolic marked this pull request as ready for review January 4, 2024 11:36

duricanikolic requested review from grafanabot and a team as code owners January 4, 2024 11:36

dimitarvdimitrov approved these changes Jan 4, 2024

View reviewed changes

pracucci self-requested a review January 4, 2024 13:12

pracucci reviewed Jan 4, 2024

View reviewed changes

pracucci approved these changes Jan 4, 2024

View reviewed changes

duricanikolic added 5 commits January 4, 2024 15:19

Allow reporting HTTP 4xx codes in status_code label of request durati…

6b411cd

…on metrics Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Log only 5xx errors in RemoteQuerier

0e90b8f

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Fixing review findings

ca958c6

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Update vendored dskit

fda8d3d

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

Fixing review findings

6d078b4

Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>

duricanikolic force-pushed the yuri/allow-4xx-errors branch from 7a02a78 to 6d078b4 Compare January 4, 2024 14:20

duricanikolic merged commit 9f2a79a into main Jan 4, 2024
28 checks passed

duricanikolic deleted the yuri/allow-4xx-errors branch January 4, 2024 16:19

eyenx mentioned this pull request May 28, 2024

chore(mimir): update to 5.3.0 adfinis/helm-charts#1256

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow server to return HTTP 4xx errors #7045

Allow server to return HTTP 4xx errors #7045

duricanikolic commented Jan 3, 2024 •

edited

dimitarvdimitrov left a comment

dimitarvdimitrov Jan 4, 2024

duricanikolic Jan 4, 2024

dimitarvdimitrov commented Jan 4, 2024

pracucci commented Jan 4, 2024

pracucci commented Jan 4, 2024

duricanikolic commented Jan 4, 2024

dimitarvdimitrov commented Jan 4, 2024

dimitarvdimitrov left a comment

pracucci Jan 4, 2024 •

edited

pracucci Jan 4, 2024

pracucci Jan 4, 2024

pracucci left a comment

	switch r.StatusCode {
	case http.StatusServiceUnavailable:
	return nil, apierror.New(apierror.TypeUnavailable, string(mustReadResponseBody(r)))
	case http.StatusTooManyRequests:
	return nil, apierror.New(apierror.TypeTooManyRequests, string(mustReadResponseBody(r)))
	case http.StatusRequestEntityTooLarge:
	return nil, apierror.New(apierror.TypeTooLargeEntry, string(mustReadResponseBody(r)))
	default:
	if r.StatusCode/100 == 5 {
	return nil, apierror.New(apierror.TypeInternal, string(mustReadResponseBody(r)))
	}
	}

	am.grpcServer = server.NewServer(&handlerForGRPCServer{am: am}, []server.Option{server.WithReturn4XXErrors}...)
	am.grpcServer = server.NewServer(&handlerForGRPCServer{am: am}, server.WithReturn4XXError)

	if code := grpcutil.ErrorToStatusCode(err); code/100 == 5 {
	if code := grpcutil.ErrorToStatusCode(err); code/100 != 4 {

Allow server to return HTTP 4xx errors #7045

Allow server to return HTTP 4xx errors #7045

Conversation

duricanikolic commented Jan 3, 2024 • edited

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

dimitarvdimitrov Jan 4, 2024

Choose a reason for hiding this comment

duricanikolic Jan 4, 2024

Choose a reason for hiding this comment

dimitarvdimitrov commented Jan 4, 2024

pracucci commented Jan 4, 2024

pracucci commented Jan 4, 2024

duricanikolic commented Jan 4, 2024

dimitarvdimitrov commented Jan 4, 2024

dimitarvdimitrov left a comment

Choose a reason for hiding this comment

pracucci Jan 4, 2024 • edited

Choose a reason for hiding this comment

pracucci Jan 4, 2024

Choose a reason for hiding this comment

pracucci Jan 4, 2024

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

duricanikolic commented Jan 3, 2024 •

edited

pracucci Jan 4, 2024 •

edited