emmett-framework / granian

A Rust HTTP server for Python applications
BSD 3-Clause "New" or "Revised" License
2.86k stars 83 forks source link

Error log on application exception causes additional Sentry event #392

Closed iamkhav closed 2 months ago

iamkhav commented 2 months ago

Hey all!

Problem

On application exceptions we will log out

Application callable raised an exception

with error level from here.

If one uses Sentry in their app, this will make the Sentry default integration for Logging to trigger an event.

This could cause confusion in Sentry because if one uses either

they end up with two events that have a similar traceback but different information. One from the error log in our Rust code with sparse information (-> stringified traceback) and one that is captured by the Excepthook and potentially filled with information.

Also, this uses up Sentry quota unnecessarily.

Discussion

I'm unsure what a good solution would be here. This might not even be seen as an issue at all. Perhaps it makes sense to log error level on fatal issues only and everything else would be a warn?

gi0baro commented 2 months ago

@iamkhav you can add ignore rules for specific loggers in Sentry, see https://docs.sentry.io/platforms/python/integrations/logging/#ignoring-a-logger

ignore_logger("_granian") should do the trick and prevent Sentry to capture errors reported by Granian

iamkhav commented 2 months ago

Of course, this would work and we can close the issue with this (seeing as Uvicorn and Gunicorn have similar behaviour). The logger being prefixed with an _ is indicative of it making sense to be ignored.

However, with the default logging config initializing _granian on INFO level, everyone who uses Sentry will probably want to ignore that logger from generating Sentry events.

🤔 My proposal of setting the level to WARNING would be unfaithful towards the descriptions given here.

iamkhav commented 2 months ago

I'm closing the issue, it makes no sense to adjust Granian code for this case.

Best solution is your proposal.

iamkhav commented 2 months ago

ignore_logger does have the drawback of disabling breadcrumb collection as well.

I'll go with something like this fn for the before_send callback instead

def before_send(event, hint):
    # Filter out events triggered by `_granian` logger
    if event.get("logger", "").startswith("_granian"):
        return None

    return event

Maybe this'll be useful for someone else who's exploring potential solutions.