load1n9 / openai

Unofficial Deno wrapper for the Open Ai api
MIT License
73 stars 23 forks source link

Improve error handling #18

Open ChuckJonas opened 1 year ago

ChuckJonas commented 1 year ago

Right now the error handling only exposes the response body via the standard Error class.

  1. This makes it hard to know if the exception was a result of this library or something else
  2. You do not have access other response properties (most importantly statusCode

I'd probably copy axios and do something like this:

class HttpResponseError extends Error {
  httpResponse: Response;

  constructor(message: string, httpResponse: Response) {
    super(message);

    // Set the prototype explicitly to allow instanceof checks
    Object.setPrototypeOf(this, HttpResponseError.prototype);

    this.name = 'HttpResponseError';
    this.httpResponse = httpResponse;
  }
}

Or maybe even just have properties forstatus, body, etc if it's already been parsed/consumed.

Then you can do something like this in your catch blocks:

}catch(e){
  if(e instanceof HttpResponseError){
     if(e.statusCode === 429){
        // retry request with backoff
        return retry();
     }
  }
  throw e;
}
lino-levan commented 1 year ago

I think it would be nice if we retry 429 errors by default... definitely a footgun that we do not.

ChuckJonas commented 1 year ago

think it would be nice if we retry 429 errors by default... definitely a footgun that we do not.

Typically, I'd leave that up to the consumer. Otherwise, it makes it harder to compose into circuit breakers or more general async retry frameworks.

If that feature was implemented, I would definitely want a way to configure it (number of retries, expo backoff, alternate server, etc) and would default it to off.

IMO, there are too many good libraries out there that support this already, this one should only be concerned with exposing the information needed to hook into those (raw request/response info)

lino-levan commented 1 year ago

Being configurable makes sense to me. My thinking is that having it on by default is a good, and users with more advanced usecases can just disable it and handle it themselves.