Error reporting improvements #22
Conversation
Zlata-Zueva
requested review from
stanislav-bedunkevich,
NRHelmi and
larf311
|
I notice the Java SDK just returns the HTTP errors directly (e.g. 404 for not found). I suspect that was a deliberate choice by @bradlo and that we should probably do the same here. |
RelationalAI/Errors/ApiException.cs
Outdated
| { | ||
| StatusCode = statusCode; | ||
| Response = response; | ||
| Headers = headers; |
There was a problem hiding this comment.
am curious why the api exception carries the HTTP headers? in the other SDKs we generally try to abstract away transport details at the API layer.
There was a problem hiding this comment.
@bradlo we really need that for the request id, as that's always important for tracing. We could extract the X-Request-ID header value and pass it directly if we know the exact header name.
Co-authored-by: Stanislav Bedunkevich <[email protected]>
| if (status != 404 && status < 500) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
what happens if resource is forbidden (403) ? Do we swallow client errors other than 404 ?
There was a problem hiding this comment.
@NRHelmi yeah, we mostly wanted to leave the behaviour as it was before. I think it's better for the SDK team to agree on the http status code handling to be consistent across all of the SDKs.
The reason for 5xx is simple, as we run into them (occasionally we do), we want to be able to see the request id in logs.
Your point is very valid though, but we'd like to leave it out of the scope of this PR is that's fine with you?
There was a problem hiding this comment.
Thank you for the clarification, can we create an issue as part of the project ?
cc @bradlo
RelationalAI/Services/Rest.cs
Outdated
| @@ -293,14 +314,14 @@ private async Task<AccessToken> RequestAccessTokenAsync(string host, ClientCrede | |||
| var resp = await RequestHelperAsync("POST", creds.ClientCredentialsUrl, data); | |||
| if (!(resp is string stringResponse)) | |||
| { | |||
| throw new SystemException("Unexpected response type"); | |||
| throw new InvalidResponseException("Unexpected response type, expected a string", resp); | |||
There was a problem hiding this comment.
maybe also include the current response type in the exception message
There was a problem hiding this comment.
@NRHelmi addressed in the latest commit. Thank you.
SystemExceptions with more meaningful errors and improves messages.Partly addresses #12, but there are still places to improve.
TODO: