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

"Generic WSGI Request" naming #3094

Closed
interDist opened this issue May 21, 2024 · 7 comments · Fixed by #3104
Closed

"Generic WSGI Request" naming #3094

interDist opened this issue May 21, 2024 · 7 comments · Fixed by #3104
Assignees

Comments

@interDist
Copy link

How do you use Sentry?

Sentry Saas (sentry.io)

Version

2.2.1

Steps to Reproduce

About half a year ago I started seeing more and more "Generic WSGI Request"s in the Sentry panels. At first I was sure the problem is with our code (even though we use the bare minimum sentry_init with DjangoIntegration, the only customization being a TracesSampler). But after multiple debug sessions I believe I isolated the core issue to this commit: 0ce9021, and specifically to the addition of lines 73–80 0ce9021#diff-2d58ee5eb379689063d5e76a9d48bd6945136fb45533809ef5b39b480afb250cR73-R80 (return self._new_style_group_matcher.sub(...) in RavenResolver._simplify ).

For URL patterns which do not contain a group (e.g., a simple “account/”), the regex replacement returns a None, which is then concatenated to a prefix (which is a str), on line 142 0ce9021#diff-2d58ee5eb379689063d5e76a9d48bd6945136fb45533809ef5b39b480afb250cR142.

This causes a TypeError exception “expected string or bytes-like object” in _set_transaction_name_and_source, and as a result, scope.set_transaction_name is not called, even not with the request.path_info fallback. Thus the “Generic WSGI Request” name set by SentryWsgiMiddleware litters the logs...

Expected Result

The path of the request is used as a fallback if the name cannot be obtained for some reason.

Actual Result

Each request which does not have a group in its URL pattern is named “Generic WSGI Request”.

@sl0thentr0py
Copy link
Member

@sentrivana I think you worked on this so I'm assigning it to you

@sentrivana
Copy link
Contributor

Hey @interDist, thanks for reaching out and for looking into this!

Trying to repro this, I set up a new Django app. This is my app's views.py:

# app/views.py

from django.http import HttpResponse
import time

def abc(request):
    time.sleep(2)
    return HttpResponse("ok")

And this is my project's urls.py:

# project/urls.py

from django.contrib import admin
from django.urls import path
from app import views

urlpatterns = [
    path('admin/', admin.site.urls),
    path('abc/', views.abc),
]

When I request my app's /abc, I get a transaction called /abc/:

Screenshot 2024-05-22 at 16 45 04

So I'm thinking there must be something more going on. What Django version are you using? Could you post a small (sanitized) view and urls.py entry where you're encountering this?

@getsantry getsantry bot moved this from Waiting for: Product Owner to Waiting for: Community in GitHub Issues with 👀 3 May 22, 2024
@interDist
Copy link
Author

I am using Django 3.2.25 with the latest version of Sentry SDK. Here are the relevant files, in my opinion, stripped down to the necessary bits:

# project/urls.py

urlpatterns = [
    path('', include('core.urls')),
]
# core/urls.py

from django.urls import path
from django.utils.translation import pgettext_lazy
from .views import LoginView

urlpatterns = [
    path(
        pgettext_lazy("URL", 'login/'),
        LoginView.as_view(), name='login'),
]
# core/views.py

from django.contrib.auth.views import LoginView as LoginBuiltinView

class LoginView(LoginBuiltinView):
    redirect_authenticated_user = True
    redirect_field_name = settings.REDIRECT_FIELD_NAME
# project/settings/sentry.py (included from other setting files)

import sentry_sdk
from sentry_sdk.integrations.django import DjangoIntegration

def sentry_init(env=None):
    sentry_sdk.init(
        dsn='SENTRY_DSN_FROM_ENV_VARIABLE',
        integrations=[DjangoIntegration()],
        environment=env or "development",
        # Percentage of error events to sample.
        sample_rate=1.0,
        # Percentage of transaction events to sample for performance monitoring.
        traces_sample_rate=0.1,
    )

Stepping with the debugger when the login view is loaded in the browser, I arrive at line 76 of transactions.py :

return self._new_style_group_matcher.sub(
lambda m: "{%s}" % m.group(2), pattern.pattern._route
)

where:

    _new_style_group_matcher.pattern = '<(?:([^>:]+):)?([^>]+)>'
    pattern = <URLPattern object 'login/' [name='login']>
    pattern.pattern = <django.urls.resolvers.RoutePattern object>
    pattern.pattern.regex = re.compile('^login/\\Z')
    pattern.pattern._route = 'login/'

Since the regular expression does not match, a None is returned by _simplify. If I comment out the whole if on lines 71–78, the regex of the pattern is used instead, resulting in the correct transaction_name = '/login/' ... Which means that the improvement introduced in commit 0ce9021 overlooked some part of Django’s routing.

@getsantry getsantry bot moved this from Waiting for: Community to Waiting for: Product Owner in GitHub Issues with 👀 3 May 22, 2024
@antonpirker
Copy link
Member

Hey @interDist !

Thanks for all this information.

I checked it now in Python 3.12 like this:

import re
_new_style_group_matcher = re.compile(r"<(?:([^>:]+):)?([^>]+)>")   # taken from transactions.py
result = _new_style_group_matcher.sub(lambda m: "{%s}" % m.group(2), 'login/') 

# result: 'login/'

So for me the result is correct. So there must be something different happening for you that makes makes _simplify return None.

Can you turn on debugging (sentry_sdk.init(..., debug=True)) and then post the output you get when hitting the route in question? (In the output there are messages for create a transaction with the "generig WSGI" name and then there should also be a message when it sets the correct name, iirc)

@sentrivana
Copy link
Contributor

I'm able to repro now using the example. Looks like this has to do with the pgettext. If I just change the path to plain "login/", the transaction name is correct.

I'm thinking for a quick fix we could try subbing and if the result is None, just fall back to the regex parsing logic after that. 👀

@sentrivana
Copy link
Contributor

Ok I think the fix is actually even simpler.

What I'm observing is that for a route that uses pgettext_lazy, pattern.pattern._route is not a string but rather some sort of proxy object. So when we try to use it in the sub, we get expected string or bytes-like object, got '__proxy__', and stuff silently blows up.

I think the fix is actually even more trivial than I thought and it's enough to stringify pattern.pattern._route:

diff --git a/sentry_sdk/integrations/django/transactions.py b/sentry_sdk/integrations/django/transactions.py
index a8e756cc..409ae77c 100644
--- a/sentry_sdk/integrations/django/transactions.py
+++ b/sentry_sdk/integrations/django/transactions.py
@@ -74,7 +74,7 @@ class RavenResolver:
             and isinstance(pattern.pattern, RoutePattern)
         ):
             return self._new_style_group_matcher.sub(
-                lambda m: "{%s}" % m.group(2), pattern.pattern._route
+                lambda m: "{%s}" % m.group(2), str(pattern.pattern._route)
             )

@interDist If you have the opportunity to try out the above ^ and see if it fixes the transaction names for you that'd be awesome! No worries if not, I'll prepare a PR regardless.

@interDist
Copy link
Author

pattern.pattern._route is not a string but rather some sort of proxy object

Yes, it is Django’s lazy object (of an internal type Promise).

I think the fix is actually even more trivial than I thought and it's enough to stringify pattern.pattern._route

Coercing the route to str works. I tested different combinations of paths locally and all of them get the appropriate transaction name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants