-
Notifications
You must be signed in to change notification settings - Fork 517
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
Fix parsing of Django path
patterns
#2452
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sentrivana
changed the title
Django resolver experiments
Fix parsing of new-style Django Oct 24, 2023
path
patterns
sentrivana
changed the title
Fix parsing of new-style Django
Fix parsing of Django Oct 24, 2023
path
patternspath
patterns
sl0thentr0py
approved these changes
Oct 25, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! don't really know how the resolver works in detail but looks ok high level
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TL;DR: Our way of parsing named group regexes out of Django URL patterns is not ideal. If someone is using (Django 2.0+)
<converter:parameter>
path
patterns, we can parse those directly without turning them into regexes first and potentially hitting our limitations there.Background
When instrumenting Django for performance monitoring, the Sentry Python SDK will by default name the transactions based on the URL pattern rule that was used to serve the request.
There are two main ways of creating a URL pattern in Django: by using regexes in the rule directly, or (since Django 2.0) by using the custom
<converter:parameter>
syntax, like so:These
<converter:parameter>
style patterns are also regexes in the background. Each of the converters defines its own regex (see e.g. the IntConverter) that is then used for matching URLs to the patterns.The SDK is parsing both types of patterns with the aim of replacing all named groups with placeholders, so that e.g. the URL pattern above creates transactions with the name
articles/{year}/{month}
. To do this, the URL pattern is first turned into its regex form and then parsed with the RavenResolver.This is not a problem with the default set of converters Django offers, but if folks define their own custom converters whose regexes don't play nice with our RavenResolver, we fail to extract the transaction name correctly. This can be prevented by parsing the original, simpler URL pattern instead without turning it into its regex form.
Note that this doesn't solve all transaction name problems as
re_path
s are still vulnerable, but it should make new-stylepath
s work.Closes #2446