How to fix backend error system hindering debugging?

[dmos62]

The problem is that when an exception is caught by our error system, it loses information that would be useful when debugging it. Information like the stack trace. Usually what I do to find out where the exception is being raised is insert breakpoints in the code that catches a regular exception and converts it into our custom exception. Something like:

        except (BadDBFunctionFormat, UnknownDBFunctionID, ReferencedColumnsDontExist) as e:
       ---> breakpoint()
            raise database_api_exceptions.BadFilterAPIException(
                e,
                field='filters',
                status_code=status.HTTP_400_BAD_REQUEST
            )

If I'm not sure where the exception is being converted, that's harder. For example for a 500 server response with the following JSON body, the way to troubleshoot it is not clear at all:

[{'code': 4999, 'message': "'anon_1'", 'field': None, 'detail': None}]

There's ways to debug all these cases, but they are all tedious.

My questions to us:

1. Does this error system serve its purpose?
   • Is there a trade-off happening where some experiences are improved by it and others (like those I outlined) are worse? I.e. are there other aspects to this?
2. What could be the next steps to improve developer experience?
   • I think it's fairly essential that we fix this, not only for our core team, but also for new contributors that might be more sensitive to difficulties debugging.

[kgodey]

Does this error system serve its purpose?

The error system as implemented is for API clients to have a consistent error experience. I do think it serves that goal well, but keep in mind that implementation is currently incomplete.

We've implemented the framework for returning errors, but haven't put a lot of work into defining individual errors. The idea is that backend developers will opportunistically define new, specific, errors when they find them useful. Eventually, code 4999 (which is UnknownError) should be almost never returned, most errors should handled by specific exceptions.

What could be the next steps to improve developer experience?

I think it would be ideal if we could use a setting/environment variable that would not catch exceptions and return the stack trace when it was on. We could use Django's DEBUG setting for this since it's already used for a similar purpose (it will show stack traces when you go to a webpage when it's on).

We would need to remember to write tests for API errors to make sure they're working appropriately since we wouldn't see them as often when developing.

[dmos62]

That's useful insight.

I think that we shouldn't handle 4999 errors at all. Because, if it's not a 4999 error, then we can use the message or code to find where the error was "handled" and insert a breakpoint. But, if the error is 4999, then its source is obfuscated, like in the example above. If some user submits a bug report with a JSON message like the 4999 I copy-pasted above, you don't have a lot to go on.

However, even for handled (non-4999) errors, the developer experience is subpar, since you have to go and insert those breakpoints. If you're getting the error in a test suite environment that is. If not (say you're using curl or the browser), you have to resort to tracing, or switch to the test suite environment, which takes a lot of effort.

Can we check if backend is in debug-mode and add a pretty-printed stacktrace alongside the current error JSON? That should go a long way.

Actually, it's not obvious to me that the addition of a stacktrace would be detrimental in a production setting either.

[kgodey]

Can we check if backend is in debug-mode and add a pretty-printed stacktrace alongside the current error JSON? That should go a long way.

That's fine with me.

Actually, it's not obvious to me that the addition of a stacktrace would be detrimental in a production setting either.

Off the top of my head, stacktraces in production:

• leak information about the deployment to the world, which is especially bad for a self-hosted product
• end up in a lot more data being sent over the wire

So let's not do this.

I think that we shouldn't handle 4999 errors at all.

That's fine with me, it'll help us identify specific errors more easily.

---

Could you create implementation issues for the above? Maybe give @mathemancer and @silentninja a day or two to respond to this discussion first in case they have concerns.

[silentninja]

I think that we shouldn't handle 4999 errors at all.

This might break the frontend if it expects an error response according to the spec. Until error handling gets stable or till Mathesar is not ready for production, we can disable handling unknown errors, so that we can find out the errors that will be thrown. This is a problem with most dynamic languages where you don't know the exceptions that would be thrown by a function.

If some user submits a bug report with a JSON message like the 4999 I copy-pasted above, you don't have a lot to go on.

The correct way of handling it in production is to send the stacktrace to an error logging server and debug based on it. For the reasons, @kgodey mentioned It is very unlikely and not a good practice to have the user give us a stacktrace.

[kgodey]

This might break the frontend if it expects an error response according to the spec.

The frontend will need to have general purpose handling for errors (e.g. in case the backend is down), it'll be covered by that.

Until error handling gets stable or till Mathesar is not ready for production, we can disable handling unknown errors, so that we can find out the errors that will be thrown.

Agreed on disabling unknown errors now. We can decide whether to re-enable it in the future based on API client and frontend needs.

[silentninja]

I have felt the pain too when debugging certain exceptions. There is certainly a lot to be done with exception handling, it is very easy to go down the rabbit hole and spending time perfecting it would take a lot of time from other important issues because there are a lot of questions that need to be answered For example,

• Separation of concern on Which layer should be responsible for validation, capturing error
• How to deal with assertion type errors meant only to be a 500 error. For example error like ValueError('Number of columns passed in must equal number of columns in table') in db.columns.operations.alter._validate_columns_for_batch_update

Initially, I used DEBUG to throw stack traces instead of handling them, but later changed it as I wanted to have an option to enable stack traces in production mode too as well as making the test case pass in DEBUG mode too which turned out to be a mistake. I will create an issue so that the error handler does not capture errors in DEBUG mode.

[kgodey]

I agree on both points - there's a lot to be done with improving exception handling and it's also not the best use of our time right now. Let's keep this discussion focused on the single issue that @dmos62 brought up, which is helping us debug errors faster. We can address other issues as they come up or in the dedicated API milestone.

Initially, I used DEBUG to throw stack traces instead of handling them, but later changed it as I wanted to have an option to enable stack traces in production mode too as well as making the test case pass in DEBUG mode too which turned out to be a mistake. I will create an issue so that the error handler does not capture errors in DEBUG mode.

This sounds good, it addresses the first action item in #1150 (reply in thread).

The other action item from #1150 (reply in thread) is not handling 4999 errors so that we become aware of errors that we need write custom exceptions for.

I think both of these should handle the immediate problem.

[silentninja]

The other action item from #1150 (reply in thread) is not handling 4999 errors so that we become aware of errors that we need write custom exceptions for.

The reason error handling PR introduced UnknownError was to replace the broad exception capture that was already in place.

try:
    ...
except Exception as e:
    raise ValidationError(e) # or raise APIException(e)

So anyone creating the bug report should consider the reason behind the broad capture and make sure if it can be removed.