Sanic integration performance#2419
Conversation
5f23719 to
119e251
Compare
antonpirker
left a comment
There was a problem hiding this comment.
Looks good, added some questions.
sentry_sdk/integrations/sanic.py
Outdated
| route_name = route.name.replace(request.app.name, "").strip(".") | ||
| 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
| # type: () -> None | ||
| Sanic._startup = _startup | ||
| ErrorHandler.lookup = _sentry_error_handler_lookup | ||
| # _patch_sanic_asgi() |
There was a problem hiding this comment.
should probably removed
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
| route_name = route.name.replace(request.app.name, "").strip(".") | ||
| 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.
sentrivana
left a comment
There was a problem hiding this comment.
Left a few comments/suggestions, but LGTM!
| 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.
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)