Implement proper error response for server endpoints
Closed this issue · 4 comments
GregoryAlbouy commented
moreirathomas commented
Also: we need to define how to handle non-critical errors (the client sends invalid data, it needs to send it again correctly shaped) vs critical errors (aka status 500 errors).
Notably is there a reconnection procedure?
moreirathomas commented
Types of error a run can produce:
ErrUnknownField
orErrUnknownPredicate
when parsing the config json/yamlInvalidConfigError
: the config is invalid, it holds an array of errors (currently produces a pre-formatted string)- native error on
config.Request.Value()
: we cannot create the request for any reason ErrConnection
: when the recorder fails to connect to the url requested- NO error on
context.DeadlineExceeded
: we simply stop the run where we are and continue on with the metrics and the tests ErrCanceled
: when the run is explicitly canceled- native error on internal errors
Types of error the server can produce:
- all of the above are forwarded
- internal error on
io.ReadAll(r.Body)
error - internal error on
configparse.JSON(body)
error - internal error on encoding any response (report or progress)
Goal:
- flag client errors differently than internal errors
- provide maximum context on client errors
- handle
ErrCanceled
differently than other errors - handle server and runner errors differently (?)
- settle on if how we deal with
context.DeadlineExceeded
currently is what we want (ie: should going above the global timeout be an error ?)
moreirathomas commented
graph TD;
A(start)-->B{parse input config}
B-->|invalid|BB[ParseConfigError: ErrUnknownField, ErrUnknownPredicate, error]
B-->C{validate config}
C-->|invalid|CC[InvalidConfigError]
C-->D{create request}
D-->|invalid|DD[ErrInternal, InvalidConfigError]
D-->E{record requests}
E-->|dead ping|EE[ErrConnection]
E-->|canceled|EEE[ErrCanceled]
E-->|unexpected|EEEE[ErrInternal]
E-->F{respond}
F-->|unexpected|FF[ErrInternal]
F-->G(end)
moreirathomas commented
Improvements can be found but the issue is viewed as implemented for now.