Skip to content

Make async gRPC less noisy#2507

Merged
sentrivana merged 2 commits intogetsentry:masterfrom
jyggen:fix-noisy-grpc
Nov 17, 2023
Merged

Make async gRPC less noisy#2507
sentrivana merged 2 commits intogetsentry:masterfrom
jyggen:fix-noisy-grpc

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 •
edited
Loading

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 •
edited
Loading

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?

sentry-python/sentry_sdk/integrations/aiohttp.py

Lines 130 to 132 in 0c9803a

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

sentrivana commented Nov 17, 2023

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sentrivana sentrivana sentrivana approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jyggen @sentrivana @Dreamsorcerer