Repository navigation
Integrations: Ariadne - handled tag is always False #2904
Description
How do you use Sentry?
Sentry Saas (sentry.io)
Version
1.42.0
Steps to Reproduce
- Define graphql query:
extend type Query {
testQuery: testPayload!
}
type TestResult {
field1: Boolean
}
type TestPayload {
status: Boolean!
result: TestResult
errors: [Error]
}
- Execute the query with requesting data not defined in
TestResult:
query {
testQuery{
resut{
non_existing_field
}
}
}
note: AriadneIntegration() is added upon sentry_sdk.init()
Expected Result
Error is reported in sentry with flag -> handled: True
Actual Result
Error is reported in sentry with flag -> handled: False
Why the error is reported as not handled when it seems that it's handled by library and a proper message is received
Message received and reported:
Cannot query field 'non_existing field' on type 'TestResult'
Activity
sentrivana commentedon Mar 26, 2024
Hey @bmarkovicvega, thanks for writing in. Looks like in the Ariadne integration we report all error events as unhandled, but this also seems to be a pattern in other similar integrations.
From your perspective as someone who uses the integration, what would you consider to be an unhandled Ariadne error? Would you prefer all Ariadne errors raised by Sentry to be marked as handled?
bmarkovicvega commentedon Mar 26, 2024
Hi @sentrivana thanks for answering. What I would expect, from my perspective, is that unhandled error would be the error raised by the code I've added, but for errors handled by
ariadnelibrary such as one I mentioned in the issue I would make them handled or even I wouldn't consider them as error that should be logged and reported in sentry.I do request with non existing field, I got message that it's not existing withing the certain query, for me it seems handled.
Giving an example and making a parallel with the REST endpoint: if we do
POST /usersthat requiresusernameandpasswordand if I don't passpasswordwhich is required I will get a response with http status400with a usual validation error message right, this doesn't seem like unhandled error that should be reported in sentry (at least it looks like handled)sentrivana commentedon Mar 28, 2024
Thanks @bmarkovicvega, that makes sense to me. For web frameworks we usually don't report client (4xx) errors at all since those should be reported by the client side, so it'd make sense to do something analogous for our GraphQL integrations as well. What's at the moment not clear to me is how we can categorize the errors we see e.g. in Ariadne into client and server errors, this needs some investigation into what the different errors look like and how we can tell them apart.
In the meantime though, if you find the errors not actionable, you can always define a custom
before_send(docs) where you either drop the error altogether (by returningNone) or you manually change thehandledfield.bmarkovicvega commentedon Mar 29, 2024
I appreciate you have looked at the issue and discussed it @sentrivana. Thanks for your time and thoughts about it. Also, thank you for giving a suggestion how to solve it for now.
17 remaining items
szokeasaurusrex commentedon Sep 13, 2024
Hey @gdalmau, with regards to the behavior in the Django integration that you are observing, I believe this happens because Django only raises exceptions for
5xxstatus codes. No exception is raised for4xxstatuses, or if one is raised, it gets handled before the Sentry instrumentation captures it.The technical problem is that there is no way for us to be certain whether a certain exception will eventually be handled. It is expected behavior that the SDK marks all exceptions which it captures through automatic instrumentation as
handled=False. I believe we almost never send an exception withhandled=True; this feature is mainly meant for users who manually capture an exception that they handle.We could, however, check whether there is a way to capture the exceptions later, after Ariadne would have handled any exceptions. These handled exceptions would then not be sent to Sentry at all; only the ones unhandled by Ariadne would show up.
gdalmau commentedon Sep 13, 2024
Thanks @szokeasaurusrex for the active conversation.
On second thought, you are absolutely right. As you said, we shouldn't even mark it as an exception (regardless of the
handledflag) and directly "ignore" it, thus treating it like a 4XX from Django, which doesn't send any event to sentry. If we agree on this behaviour, is there any information that we need to know to implement it?szokeasaurusrex commentedon Sep 13, 2024
@gdalmau To be honest, I am not fully convinced that it is a good idea to start ignoring errors that the SDK currently would send to Sentry. Even if we were certain that some class of exception raised by Ariadne always gets handled, what if there are some users relying on these exceptions getting sent to Sentry for whatever reason? If we start ignoring some errors by default, these would disappear for all users, including those who rely on the current behavior.
I will admit, though, that I personally have not worked too much with Ariadne, so if you can explain which exact errors you think should be ignored and why you expect they would not be relevant to users, this might convince me that we should be ignoring these errors.
Otherwise, if you are seeing some errors in Sentry that you know Ariadne handles, and which you are not interested in, my suggestion would be to set the
before_sendcallback to filter these out.before_sendgives you the maximum flexibility to decide what events you want to send to Sentry based on your needs, and using it does not require any changes in the SDK.getsantry commentedon Oct 5, 2024
This issue has gone three weeks without activity. In another week, I will close it.
But! If you comment or otherwise update it, I will reset the clock, and if you remove the label
Waiting for: Community, I will leave it alone ... forever!"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀