-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
Change error responses from plain text to JSON #4845
Changes from 2 commits
75d4199
f700a24
4820322
c14902c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,18 +2,18 @@ package deletion | |
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"net/http" | ||
"time" | ||
|
||
"github.com/cortexproject/cortex/pkg/tenant" | ||
"github.com/cortexproject/cortex/pkg/util" | ||
util_log "github.com/cortexproject/cortex/pkg/util/log" | ||
"github.com/go-kit/log/level" | ||
"github.com/prometheus/client_golang/prometheus" | ||
"github.com/prometheus/common/model" | ||
"github.com/prometheus/prometheus/promql/parser" | ||
|
||
"github.com/cortexproject/cortex/pkg/tenant" | ||
"github.com/cortexproject/cortex/pkg/util" | ||
util_log "github.com/cortexproject/cortex/pkg/util/log" | ||
serverutil "github.com/grafana/loki/pkg/util/server" | ||
) | ||
|
||
// DeleteRequestHandler provides handlers for delete requests | ||
|
@@ -39,21 +39,21 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r | |
ctx := r.Context() | ||
userID, err := tenant.TenantID(ctx) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, err.Error()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not too familiar with how HTTP errors are handled, but are they not all handled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK thanks for that. I was looking for a way that we could somehow add middleware to catch these I would've loved to have stuck with idiomatic gorilla/mux & net/http, but it seems we don't have a good layer of indirection here. What you've got here looks like a good solution 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An option to catch and resolve errors in a middleware would be, to write an occurring error to the request context and check for it in the middleware handler. I don't think that is very idiomatic, though. Writing the error to the response writer seems to be the common way. |
||
return | ||
} | ||
|
||
params := r.URL.Query() | ||
match := params["match[]"] | ||
if len(match) == 0 { | ||
http.Error(w, "selectors not set", http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, "selectors not set") | ||
return | ||
} | ||
|
||
for i := range match { | ||
_, err := parser.ParseMetricSelector(match[i]) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, err.Error()) | ||
return | ||
} | ||
} | ||
|
@@ -63,7 +63,7 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r | |
if startParam != "" { | ||
startTime, err = util.ParseTime(startParam) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, err.Error()) | ||
return | ||
} | ||
} | ||
|
@@ -74,12 +74,12 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r | |
if endParam != "" { | ||
endTime, err = util.ParseTime(endParam) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, err.Error()) | ||
return | ||
} | ||
|
||
if endTime > int64(model.Now()) { | ||
http.Error(w, "deletes in future not allowed", http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, "deletes in future not allowed") | ||
return | ||
} | ||
} | ||
|
@@ -91,7 +91,7 @@ func (dm *DeleteRequestHandler) AddDeleteRequestHandler(w http.ResponseWriter, r | |
|
||
if err := dm.deleteRequestsStore.AddDeleteRequest(ctx, userID, model.Time(startTime), model.Time(endTime), match); err != nil { | ||
level.Error(util_log.Logger).Log("msg", "error adding delete request to the store", "err", err) | ||
http.Error(w, err.Error(), http.StatusInternalServerError) | ||
serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) | ||
return | ||
} | ||
|
||
|
@@ -104,20 +104,20 @@ func (dm *DeleteRequestHandler) GetAllDeleteRequestsHandler(w http.ResponseWrite | |
ctx := r.Context() | ||
userID, err := tenant.TenantID(ctx) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, err.Error()) | ||
return | ||
} | ||
|
||
deleteRequests, err := dm.deleteRequestsStore.GetAllDeleteRequestsForUser(ctx, userID) | ||
if err != nil { | ||
level.Error(util_log.Logger).Log("msg", "error getting delete requests from the store", "err", err) | ||
http.Error(w, err.Error(), http.StatusInternalServerError) | ||
serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) | ||
return | ||
} | ||
|
||
if err := json.NewEncoder(w).Encode(deleteRequests); err != nil { | ||
level.Error(util_log.Logger).Log("msg", "error marshalling response", "err", err) | ||
http.Error(w, fmt.Sprintf("Error marshalling response: %v", err), http.StatusInternalServerError) | ||
serverutil.JSONError(w, http.StatusInternalServerError, "error marshalling response: %v", err) | ||
} | ||
} | ||
|
||
|
@@ -126,7 +126,7 @@ func (dm *DeleteRequestHandler) CancelDeleteRequestHandler(w http.ResponseWriter | |
ctx := r.Context() | ||
userID, err := tenant.TenantID(ctx) | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, err.Error()) | ||
return | ||
} | ||
|
||
|
@@ -136,28 +136,28 @@ func (dm *DeleteRequestHandler) CancelDeleteRequestHandler(w http.ResponseWriter | |
deleteRequest, err := dm.deleteRequestsStore.GetDeleteRequest(ctx, userID, requestID) | ||
if err != nil { | ||
level.Error(util_log.Logger).Log("msg", "error getting delete request from the store", "err", err) | ||
http.Error(w, err.Error(), http.StatusInternalServerError) | ||
serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) | ||
return | ||
} | ||
|
||
if deleteRequest == nil { | ||
http.Error(w, "could not find delete request with given id", http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, "could not find delete request with given id") | ||
return | ||
} | ||
|
||
if deleteRequest.Status != StatusReceived { | ||
http.Error(w, "deletion of request which is in process or already processed is not allowed", http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, "deletion of request which is in process or already processed is not allowed") | ||
return | ||
} | ||
|
||
if deleteRequest.CreatedAt.Add(dm.deleteRequestCancelPeriod).Before(model.Now()) { | ||
http.Error(w, fmt.Sprintf("deletion of request past the deadline of %s since its creation is not allowed", dm.deleteRequestCancelPeriod.String()), http.StatusBadRequest) | ||
serverutil.JSONError(w, http.StatusBadRequest, "deletion of request past the deadline of %s since its creation is not allowed", dm.deleteRequestCancelPeriod.String()) | ||
return | ||
} | ||
|
||
if err := dm.deleteRequestsStore.RemoveDeleteRequest(ctx, userID, requestID, deleteRequest.CreatedAt, deleteRequest.StartTime, deleteRequest.EndTime); err != nil { | ||
level.Error(util_log.Logger).Log("msg", "error cancelling the delete request", "err", err) | ||
http.Error(w, err.Error(), http.StatusInternalServerError) | ||
serverutil.JSONError(w, http.StatusInternalServerError, err.Error()) | ||
return | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we special-case 404s? This is going to make writing Loki clients complex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404 errors are handled differently than errors that occur in our handlers. However, it should be possible to override the NotFoundHandler of the router.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you look into that please? We should really try have consistency across the board if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was actually pretty easy to achieve: 4820322