Skip to content
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

Add a final error handler package #310

Closed
rowanmanning opened this issue Nov 8, 2022 · 3 comments
Closed

Add a final error handler package #310

rowanmanning opened this issue Nov 8, 2022 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@rowanmanning
Copy link
Member

rowanmanning commented Nov 8, 2022

Add a final error handling package which outputs the error status code, any headers that need to be set, and a basic text-based error page which is intended to be overridden by next-errors.

What problem does this feature solve?

We currently maintain code to output final error information within n-express. At time of writing, it looks like this:

this.app.use((err, req, res, next) => {
	let statusCode = parseInt(err.statusCode || err.status || 500, 10);

	// There's clearly an error, so if the error has a status
	// code of less than `400` we should default to `500` to
	// ensure bad error handling doesn't send false positive
	// status codes. We also check that the status code is
	// a valid number.
	const isValidErrorStatus = (
		!Number.isNaN(statusCode) && // Possible if `error.status` is something unexpected, like an object
		statusCode >= 400 &&
		statusCode <= 599
	);

	res.statusCode = isValidErrorStatus ? statusCode : 500;
	const statusMessage = STATUS_CODES[res.statusCode] || STATUS_CODES[500];

	// If headers have been sent already then we need to hand off to
	// the final Express error handler as documented here:
	// https://expressjs.com/en/guide/error-handling.html#the-default-error-handler
	//
	// This ensures that the app doesn't crash with `ERR_STREAM_WRITE_AFTER_END`
	if (res.headersSent) {
		nLogger.warn({
			event: 'EXPRESS_ERROR_HANDLER_FAILURE',
			message: 'The default n-express error handler could not output because the response has already been sent',
			error: serializeError(err),
			request: serializeRequest(req)
		});
		return next(err);
	}

	// If Sentry is in use, the error id is attached to `res.sentry`
	// to be returned and optionally displayed to the user for support.
	// If Sentry is disabled we fall back to the relevant HTTP status
	// code and message
	const nonSentryOutput = `${res.statusCode} ${statusMessage}`;
	const output = `${appOptions.withSentry ? res.sentry : nonSentryOutput}\n`;
	res.end(output);
});

This is a lot of code maintained outside of Reliability Kit with a focus on errors and how they get presented. We (the CP Reliability team) were the last people who made any changes to this code too.

It's difficult for us to own one part of one file within n-express, n-express does too much anyway, and this doesn't align with our ethos of "trust our engineers" – there's not an easy way to remove this error handling or configure it to suit your app, and it's hidden away in some n-express magic rather than being explicitly included.

Ideal solution

I think we should review what we need from this error rendering middleware and provide a new Reliability Kit package which does this instead. Unsure what it'd be called yet. It feels separate to #227 as we'd be moving existing functionality into Reliability Kit.

Alternatives

We could do nothing and keep this code in n-express, optionally trying to add us as owners of this file. I think we miss an opportunity to reduce the footprint and complexity of n-express though and hand some control back to our teams.

We could also release this package in Reliability Kit and import it into n-express in place of the custom code that'd currently maintained there.

@rowanmanning rowanmanning added the enhancement New feature or request label Nov 8, 2022
@rowanmanning rowanmanning moved this to 📥 Inbox in Reliability Kit Roadmap Nov 8, 2022
@rowanmanning rowanmanning moved this from 📥 Inbox to 📝 Planned in Reliability Kit Roadmap Nov 8, 2022
@rowanmanning
Copy link
Member Author

Note: I don't think we should do this until Sentry has been removed from n-express fully, which will be a later major breaking version. Trying to move this code out right now will result in some unnecessary Sentry-related stuff being codified in Reliability Kit

@rowanmanning
Copy link
Member Author

To add some firmer dates, we should be able to do this work after mid-August 2023. At this point we'll be fully removing Sentry from n-express

@rowanmanning rowanmanning moved this from 📝 Planned to 📥 Inbox in Reliability Kit Roadmap Apr 12, 2023
@rowanmanning rowanmanning self-assigned this Nov 14, 2023
@rowanmanning rowanmanning moved this from 📥 Inbox to 🏗 In Progress in Reliability Kit Roadmap Nov 14, 2023
rowanmanning added a commit that referenced this issue Nov 15, 2023
This makes the error rendering middleware also take responsibility for
outputting an error page in production environments. In production no
error details are leaked and we only render the status code and message,
this replicates the behaviour currently in n-express.

We're doing this to reduce the footprint of n-express (#310) which
currently contains some error rendering logic. Once use of the
Reliability Kit error rendering middleware is more ubiquitous we should
be able to remove the error handling code from n-express in a new major
version.

While this is technically a breaking change, if an app is using
n-express then it's safe to upgrade. This is because this package
behaves in exactly the same way as the n-express error handler.
rowanmanning added a commit that referenced this issue Nov 15, 2023
This makes the error rendering middleware also take responsibility for
outputting an error page in production environments. In production no
error details are leaked and we only render the status code and message,
this replicates the behaviour currently in n-express.

We're doing this to reduce the footprint of n-express (#310) which
currently contains some error rendering logic. Once use of the
Reliability Kit error rendering middleware is more ubiquitous we should
be able to remove the error handling code from n-express in a new major
version.

While this is technically a breaking change, if an app is using
n-express then it's safe to upgrade. This is because this package
behaves in exactly the same way as the n-express error handler.
rowanmanning added a commit that referenced this issue Nov 16, 2023
This makes the error rendering middleware also take responsibility for
outputting an error page in production environments. In production no
error details are leaked and we only render the status code and message,
this replicates the behaviour currently in n-express.

We're doing this to reduce the footprint of n-express (#310) which
currently contains some error rendering logic. Once use of the
Reliability Kit error rendering middleware is more ubiquitous we should
be able to remove the error handling code from n-express in a new major
version.

While this is technically a breaking change, if an app is using
n-express then it's safe to upgrade. This is because this package
behaves in exactly the same way as the n-express error handler.
@rowanmanning
Copy link
Member Author

This work is complete and made it into [email protected]

@rowanmanning rowanmanning moved this from 🏗 In Progress to ✅ Complete in Reliability Kit Roadmap Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

1 participant