Fetch Error Handling: proposed new module

[rowanmanning]

Now that Reliability Kit has been around for a while, we're noticing some patterns in the way we need to handle errors. One of the things we've found we write a lot of boilerplate code for is handling errors that occur as a result of an HTTP request, e.g. via the fetch API.

An example of this is in next-newsletters-page here. Often with fetches we find we need to do the following to get full and detailed error reporting for our HTTP requests:

• Did the fetch throw a network error?
  • Yes: rethrow
  • No: continue
• Did the fetch throw a timeout error?
  • Yes: throw a new UpstreamServiceError
  • No: continue
• Is the response status code between 400 and 499?
  • Yes: throw a new OperationalError - the current app has an issue
  • No: continue
• Is the response status code between 500 and 599?
  • Yes: throw a new UpstreamServiceError with the upstream system code - the upstream service has an issue
  • No: continue
• (optional) Is the response body valid JSON?
  • No: throw a new UpstreamServiceError - the upstream app has an issue
  • Yes: continue

I'm proposing we wrap this logic into a new Reliability Kit module which can be used to wrap calls to fetch. Here's my current thinking on some options for the API.

Option 1: wrap the fetch function

const { wrapFetch } = require('@dotcom-reliability-kit/fetch');
const nodeFetch = require('node-fetch');

const fetch = wrapFetch({ fetchFunction: nodeFetch });

// All errors are handled as above, the API is the same as `fetch`
await fetch('https://example.com/');

If you want the errors to be associated correctly with the upstream system that's erroring:

const fetch = wrapFetch({
    fetchFunction: nodeFetch,
    systemCode: 'example'
});

I think the positive of this approach is that once you have the generated fetch function the API is familiar – it's the same as regular fetch. The negative is that it could be confusing to debug in a scenario where somebody doesn't realise that this fetch is not native code. We'd also have to cater for multiple different fetch implementations with different APIs, e.g. node-fetch supports timeout but native fetch uses AbortController.

Option 2: wrap the fetch promise

const { handleFetchErrors } = require('@dotcom-reliability-kit/fetch');
const nodeFetch = require('node-fetch');

await handleFetchErrors(nodeFetch('https://example.com/'));

If you want the errors to be associated correctly with the upstream system that's erroring:

await handleFetchErrors(nodeFetch('https://example.com/'), {
    systemCode: 'example'
});

The positive of this approach is that you separate the error handling completely from the fetch implementation, which means that you don't need to cater for differences in third-party vs native APIs. As long as the handleFetchErrors function can deal with a Response-like object then fetch can be anything. The negative is that it'd be easy to make some small mistakes in implementing, e.g. what happens if the inner fetch is awaited too? like:

await handleFetchErrors(await nodeFetch('https://example.com/'));

Update: with the above case we'd probably just error if the value passed in isn't a promise, something like:

if (typeof value?.then !== 'function') {
    throw new TypeError(
        `handleFetchErrors expects a Promise, ${typeof value} was given. Check that you're not awaiting the promise before passing it in – check the documentation here: https://example.com/`
    )
}

Option 3: provide a rejection handler

Edit: this is an adaption of option 2 which was suggested by @vbachev in this comment and @wheresrhys in this comment, it's a little more verbose but probably the more idiomatic Node.js/promise-based approach.

We could provide a function which generates a rejection handler which can then be passed directly into fetch().catch(). However we'd also need a second handler for when the fetch doesn't reject but it it's not OK:

const { handleFetchErrors, handleFetchResponseErrors } = require('@dotcom-reliability-kit/fetch');
const nodeFetch = require('node-fetch');

await nodeFetch('https://example.com/')
    .catch(handleFetchErrors)
    .then(handleFetchResponseErrors);

If you want the errors to be associated correctly with the upstream system that's erroring, we could expose a second method to generate a handler:

const { handleFetchErrors, createFetchResonseErrorHandler } = require('@dotcom-reliability-kit/fetch');
const nodeFetch = require('node-fetch');

await nodeFetch('https://example.com/')
    .catch(handleFetchErrors)
    .then(createFetchResonseErrorHandler({
        systemCode: 'example'
    }));

If you don't want to use catch/then, it's also possible to use these methods like this, but I think it's harder to read (because you have to resolve whether the error is a fetch error or one thrown by the response handler):

try {
    const response = await nodeFetch('https://example.com/');
    handleFetchResponseErrors(response);
} catch (error) {
    if (!error.isOperational) {
        handleFetchErrors(error);
    } else {
        throw error;
    }
}

The positive of this approach is that you separate the error handling completely from the fetch implementation, like option 2. I think this reduces the possibility of it being implemented incorrectly as we're leaning more on native promise handling. The negative is the verbosity, and there's still a small chance that an implementer will use the wrong handler in the catch or then.

It's also possible that someone will accidentally assign the catch/then in the wrong order, which completely changes the way this would work.

Feedback please

I'm interested in your thoughts on this. Are we over-engineering? Is this useful? Do you have a different opinion of how this could work? Thanks! In terms of options I'm leaning towards 2 I think, option 3 seemed good on the surface but actually ended up being more complex and boilerplate than I initially thought.

[AniaMakes]

I can't decide if I like it or not. So here's a bit of a brain monologue:

I think a good question to ask is if people would actually take time to go back to the code that was written months / years ago to implement this. I'm inclined to think that's not going to be a super high priority work, maybe something that will live in the backlog for months and will get given to a new comer to the team.

I am wondering about discoverability. You mention this would be one of many modules - there's a chance that people will forget it's there at the time they are setting up a new app - or never realise it exists in the first place.

The reason this repeated code is in many places because the teams want customise handling - this allows them to set up more or less detailed (or more or less human-readable) messages, allowing them to note any idiosyncrasies into the messages (that may not apply to other systems).

Having said this, I'd lean towards option 2, mainly to distinguish it from native fetch.

[rowanmanning]

Thanks Ania, like all of Reliability Kit I'd say this isn't a "we go and fix everything" module but more a "let's use this in future and update old code if you're working in it" module. I'm happy with this as incremental improvements like this are more sustainable and every extra fetch that's instrumented like this is an improvement to our monitoring.

On discoverability, I think we'll push adoption of this through blog posts and workshops initially. We've been thinking about re-running some workshops. I don't think this will be any less discoverable than, for example, Crash Handler.

The last point on customising handling, I'd want to capture people's use-cases so that we can allow customisation through config options so that we cater for as many use-cases as possible. It's worth noting that, when it comes to error reporting, uniformity is better. I'd generally discourage customising messages or error codes because the real power of this module would come with being able to build dashboards to get a view of fetch errors across our whole estate.

[phelipemaia]

That's really nice @rowanmanning ! I would just change to have everything as options so we can switch on/off things like retry on error.

I think the best solution will depends on the project really. I can see there will be cases that option 1 would be great, for example if you have a global fetch - you would only need to configure the wrapFetch once. However, I don't like the wrapFetch naming and of course I don't have a better naming to suggest 😄
The second one I think it give us more control around where to use the wrapper and it will be clear for maintainers what the code is doing - we are explicitly saying we want to handle errors. So, I would vote option 2.

Also, for legacy code, I think we can adopt the refactoring idea of: if we are going to change anything in a code that uses fetch and handles errors, let's replace with the new implementation - not doing all at once.

Is n-fetch a good library to use an example? This would kit come for free for systems/libs using n-fetch.

[rowanmanning]

Thanks Phelipe, yep I think lots of options would be needed and we'd probably discuss each of those with interested people.

Also, for legacy code, I think we can adopt the refactoring idea of: if we are going to change anything in a code that uses fetch and handles errors, let's replace with the new implementation - not doing all at once.

I agree with this, incremental change is fine.

Is n-fetch a good library to use an example? This would kit come for free for systems/libs using n-fetch.

I just used this as one example, but I would expect the examples above to work with any fetch implementation ideally, including Node.js 18's native fetch.

[vbachev]

Very well put Rowan 🙌
That's something that's been on my mind as well!
My teammates keep comparing fetch to axios and sometimes complain how hard it is to cover all error states with the fetch API - all that boilerplate handling of cases people rarely think of. It does sound like a thing we can abstract away so people can forget about the underlying networking mechanics!
What i've been thinking of as the least friction approach was to put all that error handling logic in a function we can slap on to a fetch promise 🤷 This way you dont ask people to change their coding patterns, its a one- or two-liner they can blindly paste without supplying any arguments to it or anything. It can handle proper logging, proper throwing of relevant errors, etc.
e.g.

try {
  const myInnocentJsonResponse = await fetch({ ... something ... })
+   .catch(handleNetworkErrors) // this takes care of comms issues
+   .then(handleFetchErrors) // this takes care of non-200 responses
    .then(response => response.json())
  console.log({ myInnocentJsonResponse })
} catch (error) { ... }

Alternatively ... Can we supply a custom package that wraps node-fetch and provides these goodies out of the box? We keep the same fetch API so nobody needs to change anything except the import at the top?

[rowanmanning]

Thanks Vasil 🙂

Hard agree on comparing fetch to axios – we do lose a lot of useful handling that we used to get in this library as well as request or got. It'd be great to bring this to fetch.

I like your suggestion of making it work as a promise handler, I wonder if allowing for multiple interfaces would work? Because we could expose these methods (handleNetworkErrors and handleFetchErrors etc) and then use them under the hood for my option 2 where you wrap the whole promise. That'd give people a lot of choice.

I did think about whether we could provide a custom package that wraps node-fetch, but I think I'd really like to avoid having any of the fetch libraries as a dependency – this results in extra maintenance and makes it so it's harder to switch out, e.g. switching to native fetch would require a major version bump but if we support passing either in then we have less churn.

[wheresrhys]

I guess await fetch(...).then(handleFetchErrors) is functionally equivalent?? Perhaps worth including this pattern of usage in your example to demonstrate its flexibility (to me it's an easier to read pattern too... but just an opinion)

[wheresrhys]

re the {systemCode} option, could this default to process.env.SYSTEM_CODE?

[rowanmanning]

Yep definitely could. We'd probably use appInfo.systemCode as the default and allow it to be overridden.

[rowanmanning]

Oh I made a mistake here actually, the systemCode in the examples above doesn't refer to the system that the code's running in, but the system that the fetch is being made to. This allows us to correctly log when an error has been caused by a 50x in an upstream service as opposed to a bug in the current system.