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

Make async gRPC less noisy #2507

Merged
merged 2 commits into from
Nov 17, 2023
Merged

Conversation

jyggen
Copy link
Contributor

@jyggen jyggen commented Nov 14, 2023

The new aio integration for gRPC creates new Sentry events for each and every exception raised by the server. This becomes a problem if the user's server code uses context.abort() (like mine does!). From the documentation:

Raises an exception to terminate the RPC with a non-OK status.

Imagine this server code:

async def GetUser(self, request, context):
        user = self._users.find_by_id(request.id)

        if not user:
            await context.abort(grpc.StatusCode.NOT_FOUND)

        return GetUserResponse(user=user)

AbortError is raised from await context.abort() every time a user is not found which, with the new Sentry integration, will also send an error event to Sentry every time a user is not found. This is comparable to sending an error to Sentry for every 404 (or any status code really) that your HTTP API returns, aka. probably not something you want to do :)

This PR fixes that by simply catching AbortError separately.

@sentrivana
Copy link
Contributor

sentrivana commented Nov 16, 2023

Hey @jyggen, thank you for the contribution! ❤️

Not super familiar with gRPC so please bear with me: according to the docs StatusCode.NOT_FOUND is just one possibility for what context.abort() can report as status code to the client, it could also be any other StatusCode. Would it make sense to differentiate based on which StatusCode the abort was called with? Let's say we're not interested in StatusCode.NOT_FOUNDs and would drop those (I'm with you on that), but on the other hand maybe StatusCode.INTERNAL or StatusCode.UNKNOWN AbortErrors should be captured by default?

If I look at this from an HTTP API point of view, on the server side I wouldn't be interested in client errors, but anything 500+ I'd like to have reported by Sentry. Is there an equivalent to this in gRPC?

@Dreamsorcerer
Copy link

Dreamsorcerer commented Nov 16, 2023

If I look at this from an HTTP API point of view, on the server side I wouldn't be interested in client errors, but anything 500+ I'd like to have reported by Sentry. Is there an equivalent to this in gRPC?

Comparing what this does in a web framework like aiohttp, it looks like it should just set the status_code of the transaction?

except HTTPException as e:
transaction.set_http_status(e.status_code)
raise

From a user perspective, if I explicitly return/raise a response with a given status, then that is expected behaviour which I wouldn't expect to be logged. If I wanted to log such a response as a warning or something, I'd add a log call.

@sentrivana
Copy link
Contributor

Gotcha @Dreamsorcerer, makes sense to me.

@sentrivana sentrivana enabled auto-merge (squash) November 17, 2023 08:34
@sentrivana sentrivana merged commit 9bf6c13 into getsentry:master Nov 17, 2023
312 of 314 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants