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

Raising exceptions without logging them #780

Open
demeralde opened this issue Sep 22, 2019 · 15 comments
Open

Raising exceptions without logging them #780

demeralde opened this issue Sep 22, 2019 · 15 comments

Comments

@demeralde
Copy link

demeralde commented Sep 22, 2019

Whenever exceptions are raised they're logged in the console (and in Sentry if it's used).

Many of these exceptions are only intended to be shown to the user. For example, django-graphql-jwt raises the PermissionDenied exception for the login_required decorator.

The problem is this pollutes the console output during testing/development and logs unnecessary errors to Sentry. For exceptions such as the example above, that's only intended to be shown to the user.

As a workaround I've tried writing middleware that catches any exceptions thrown:

class ExceptionFilterMiddleware:
    IGNORED_EXCEPTIONS = (
        # Local exceptions
        ValidationException,
        # Third-party exceptions
        JSONWebTokenExpired,
        PermissionDenied,
    )

    def on_error(self, error):
        if not isinstance(error, self.IGNORED_EXCEPTIONS):
            return error

    def resolve(self, next, *args, **kwargs):
        return next(*args, **kwargs).catch(self.on_error)

But if an exception is caught, it no longer populates the errors field in query/mutation output. Therefore all errors are logged, there's no way to conditionally log exceptions.

This means the only solution is to create a logging filter like the following:

def skip_valid_exceptions(record):
    """
    Skip exceptions for errors only intended to be displayed to the API user.
    """
    skip: bool = False

    if record.exc_info:
        exc_type, exc_value = record.exc_info[:2]
        skip = isinstance(exc_value, valid_exceptions)

    return not skip

But this doesn't work either because record.exc_info is None whenever an error is thrown with Graphene, therefore it's not possible to conditionally filter out exceptions based on their type.

Is there a solution for this? Seems like it'd be a common issue but I've had trouble finding one. Something like this would be nice to be able to implement. Thanks in advance.

@demeralde
Copy link
Author

Accidentally closed this issue.

@demeralde demeralde reopened this Sep 29, 2019
@stale
Copy link

stale bot commented Dec 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Dec 29, 2019
@demeralde
Copy link
Author

Responding to remove the bot's label. I still have this issue and it's important.

@stale
Copy link

stale bot commented May 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label May 2, 2020
@demeralde
Copy link
Author

Commenting to remove the wontfix label, this still needs to be fixed. Just had someone comment on my StackOverflow post with the same problem.

@ulgens ulgens self-assigned this May 13, 2020
@stale stale bot removed the wontfix label May 13, 2020
@ulgens
Copy link
Collaborator

ulgens commented May 13, 2020

Hey @dspacejs , I removed the label. I also having problems with exact same issue for a long time. graphql-core package has other issues with error logging, i created a PR for that graphql-python/graphql-core-legacy#269 but it's for the legacy version of the package and i don't think it will be ever merged. I agree on the importance of the issue you mention, but i think it would be better to revisit this after graphene 3 update.

@demeralde
Copy link
Author

@ulgens thanks. What will Graphene 3 update exactly that will address this?

@ulgens
Copy link
Collaborator

ulgens commented May 19, 2020

@dspacejs I don't think the update has anything to address this issue, but if the issue still exist after the update, fixes on the core package will be more likely to get merged.

@mcabrams
Copy link

mcabrams commented Aug 5, 2020

Definitely interested in a solution for this myself - don't necessarily need to raise errors instead of logging them, but figuring out a better way for sentry to at least track them (vs needing to look at logs) would be super helpful.

@marcelombc
Copy link

The problem is that report_error method from ExecutionContext class in graphql.execution.utilsis doing logger.error("".join(exception))
I have managed to fix this by monkey patch this class method.

from traceback import format_exception
from graphql.execution.utils import ExecutionContext, logger


def new_report_error(self, error, traceback=None):
    if not (
        hasattr(error, "original_error")
        and isinstance(error.original_error, IGNORED_EXCEPTIONS)
    ):
        exception = format_exception(
            type(error), error, getattr(error, "stack", None) or traceback
        )
        logger.error("".join(exception))

    self.errors.append(error)


# Monkey patch report_error method from ExecutionContext class
ExecutionContext.report_error = new_report_error

@jonathankao97
Copy link

Any updates on this issue? Running into the exact issue @dspacejs described

@ulgens ulgens removed their assignment Aug 27, 2021
@philipptrenz
Copy link

Would also like to have a solution for this issue

@demeralde
Copy link
Author

No idea what's going on here. I think this issue should be prioritised, it's been over 2 years.

@philipptrenz
Copy link

I'm currently using the following logging filter to filter out GraphQLLocatedErrors, which get raised by the @login_required decorator of graphql_jwt when users are not authenticated:

class GraphQLLogFilter(logging.Filter):
    def filter(self, record):
        if record.msg and 'GraphQLLocatedError' in record.msg:
            return None
        return True

# settings.py
LOGGING = {
    'version': 1,
    'disable_existing_loggers': False,
    'handlers': {
        'console': {
            'level': 'DEBUG',
            'class': 'logging.StreamHandler',
        },
    },
    'filters': {
        'graphql_log_filter': {
            '()': GraphQLLogFilter,
        }
    },
    'loggers': {
        'graphql.execution.utils': {
            'level': 'WARNING',
            'handlers': ['console'],
            'filters': ['graphql_log_filter'],
        },
    },
}

Nevertheless this exceptions should not end up in logging at all. I hope for a solution.

@LMPombar
Copy link

LMPombar commented Feb 1, 2024

Has this issue been fixed in the current version?

It would be nice to have more control over the verbosity of these errors. The stacktraces really worsen my logs readibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants