Sanic integration performance #2419
Conversation
szokeasaurusrex
force-pushed
the
szokeasaurusrex/sanic-performance
branch
from
5f23719
to
119e251
Compare
sentry_sdk/integrations/sanic.py
Outdated
| scope.set_transaction_name( | ||
| route_name, source=TRANSACTION_SOURCE_COMPONENT | ||
| "/%s" % route.path, source=TRANSACTION_SOURCE_COMPONENT |
There was a problem hiding this comment.
What is in route.path? Because we should make sure that transaction names have low cardinality. (So /user/1231231 would be not good, but /users/{id} or similar would be good. (this is important for grouping events together on the backend.
There was a problem hiding this comment.
Also if the transaction name is a route /users/{id} the source should be TRANSACTION_SOURCE_ROUTE. See https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations
There was a problem hiding this comment.
also applicable above in the continue_trace call.
There was a problem hiding this comment.
The route.path is similar to what you wrote in your example, but for Sanic, it also includes the type of the URL parameter. So, for your example, the route.path would likely be users/{id:int}, and then we would prepend a / to obtain /users/{id:int} as the transaction name (prepending the / ensures that the root route displays as / instead of as an unnamed transaction, since the root route's route.path is an empty string).
There was a problem hiding this comment.
I would not add a /. Because the user will know what is meant anyhow, and maybe his app runs on /api/user/... and then /user/... would be wrong.
sentry_sdk/integrations/sanic.py
Outdated
| @@ -133,6 +135,7 @@ def _setup_sanic(): | |||
| # type: () -> None | |||
| Sanic._startup = _startup | |||
| ErrorHandler.lookup = _sentry_error_handler_lookup | |||
| # _patch_sanic_asgi() | |||
There was a problem hiding this comment.
Yes, I have this in my list of to-do items before merge
sentry_sdk/integrations/sanic.py
Outdated
| scope.set_transaction_name( | ||
| route_name, source=TRANSACTION_SOURCE_COMPONENT | ||
| "/%s" % route.path, source=TRANSACTION_SOURCE_COMPONENT |
There was a problem hiding this comment.
also applicable above in the continue_trace call.
| @@ -54,6 +58,16 @@ class SanicIntegration(Integration): | |||
| identifier = "sanic" | |||
| version = None | |||
|
|
|||
| def __init__(self, unsampled_statuses=frozenset({404})): | |||
There was a problem hiding this comment.
Please also add the new option to the Sanic docs if you haven't already.
szokeasaurusrex
changed the title
Closes #2388
Before this PR is ready to merge, a few things need to be addressed.
and ASGI mode cases (for ASGI mode, at least verify that the SDK does not crash it)