Support pluggable error logging in shelf_io

[kevmoo]

[nex3]

This already exists: if it's used within an error Zone, shelf_io will pass errors to that zone rather than logging them itself.

[natebosch]

@kevmoo I wonder if this is something we to try to tackle quickly in case it's easier to do with a breaking change. Do you have any thoughts on what the best API would be?

The error zone behavior we have now helps with exceptions other than ArgumentError, but it doesn't help with specific errors we log like these:

shelf/lib/shelf_io.dart

Lines 90 to 104 in 5a77d25

	_logTopLevelError('Error parsing request.\n$error', stackTrace);
	final response = Response(
	400,
	body: 'Bad Request',
	headers: {HttpHeaders.contentTypeHeader: 'text/plain'},
	);
	await _writeResponse(response, request.response);
	} else {
	_logTopLevelError('Error parsing request.\n$error', stackTrace);
	final response = Response.internalServerError();
	await _writeResponse(response, request.response);
	}
	return;
	} catch (error, stackTrace) {
	_logTopLevelError('Error parsing request.\n$error', stackTrace);

[kevmoo]

I just did this here: https://github.com/GoogleCloudPlatform/functions-framework-dart/blob/main/functions_framework/lib/src/logging.dart

Not sure what else we'd do...

Are you thinking we do this in shelf_io?

[natebosch]

My goal is to be able to use shelf_io without risk of writing to stderr. I don't think that is feasible today because if a request comes in that can't be parsed we hit the code path I linked above which will do a stderr.writeln.

[kevmoo]

Oh! That would be nice – just for testing! Agreed here!

…

On Fri, Dec 4, 2020 at 2:50 PM Nate Bosch ***@***.***> wrote: My goal is to be able to use shelf_io without risk of writing to stderr. I don't think that is feasible today because if a request comes in that can't be parsed we hit the code path I linked above which will do a stderr.writeln. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEFCW7GTHGDKPA4367QTLSTFRSJANCNFSM4A47KE5Q> .

[kevmoo]

Could we just have an optional function param all the way down for logTopLevelError? Non-breaking!

[natebosch]

Could we just have an optional function param all the way down for logTopLevelError? Non-breaking!

It looks feasible. I almost wonder if we should drop the behavior around error zones entirely though. That makes it breaking but it means there aren't two ways to handle the same problem. WDYT?

[natebosch]

@jakemac53 @kevmoo and I chatted about this.

Our biggest concern was that the removal of hidden magic behavior, is also hidden. We don't like that there would be a breaking change with no static indication that anything was breaking. We don't expect all users carefully read the CHANGELOG for breaking changes.

If we make the new onError argument required though, then it is statically breaking and will call attention during the migration. It's also sensible that the error handling be explicit at the call site - if the caller does want the behavior of writing to stderr we can make that easy for them.

[natebosch]

@kevmoo - I think at some point you had mentioned wanting to have access to the Request in the error handler. I think we could do that if we use a separate error zone for every request.

The downside is that we want to keep a nice static type for the argument, so we can't allow functions that don't want the Request, which also means that in the case where we know we are already running in an error zone (like in tests) and we want to just allow that to handle error we'd need to wrap it (error, stackTrace, request) => Zone.current.handleUncaughtError(error, stackTrace)

If there are errors parsing the IO request we also wont' have a shelf request to give to the error handler, so the type would need to be void Function(Object, StackTrace, Request?) to handle those errors as well.

There are no existing use cases for reading the Request since the current error handlers don't have access. If we think some exist we might want to provide it anyway though.

[kevmoo]

Wow...this bug has been around for a while! Let's chat tomorrow.