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
Have a global signalling mechanism for request/response lifecycle #1348
Comments
Hmmm. My initial thought here is, can’t you already just wrap the ASGI app in a Sentry provided app/middleware (which in ASGI is the same thing really)? |
@carltongibson no serious apm tool gives the user something to configure, as sooner or later it has to monkeypatch anyway for things that cannot be instrumented through official APIs. From my op:
|
I’m not sure I quite follow you. (Surely I have to configure something...) Why not just say “Edit asgi.py to wrap the default application in the Sentry middleware, and you’re done! 💃”? (Just trying to understand what you’re proposing.) |
@carltongibson Take a look here and here to see what is actually configured, and take a look at the code I linked in the OP to see how Django is then instrumented. Ideally things "just work" by calling Django's and Flask's global signalling mechanism are the kind of API I'd ultimatively like to see, so that's what I am proposing. |
Note that channels already has a signals module that contains some definitions, but those signals seem to be never fired. |
@carltongibson wrapping in a sentry middleware does not help that much since as an example here is the middleware I have used: https://gist.github.com/hishnash/4e44f2b617a67379cfc63129e141f9c1 this groups issues by the URL (that is not optimal since 2 most of our urls have variables within the path) to enable grouping on a per-handler (like sentry does for vanilla Django) we need to either wrap each consumer or Patch/Subclass the PathRouter middleware. this makes adding sentry to an application a big code change in comparison to adding sentry to vanilla Django were you just init sentry and it |
Yeah... At this point, I'm still not sure what to say — I haven't had the time to follow the various links and internalise all that, so...? I'd suggest the best way forward would be a PoC PR saying, "This is what it would look like, and these are the benefits it would bring". If the footprint were small enough (relative the gain) why not? (It's not that I'm anti any proposal here, but rather I'm not yet sure what's involved.) |
@carltongibson this would be the minimal patch: diff --git a/channels/routing.py b/channels/routing.py
index 62d7bd5..a7bd06d 100644
--- a/channels/routing.py
+++ b/channels/routing.py
@@ -39,7 +39,17 @@ def get_default_application():
raise ImproperlyConfigured(
"Cannot find %r in ASGI_APPLICATION module %s" % (name, path)
)
- return value
+ return DefaultApplication(value)
+
+
+class DefaultApplication:
+ __slots__ = ("inner",)
+
+ def __init__(self, inner):
+ self.inner
+
+ def __call__(self, scope):
+ return self.inner(scope)
class ProtocolTypeRouter: With that alone all the APM tools can monkeypatch The nicer solution would be to emit some signals in |
OK, but then this is exactly what I first suggested, except I'd have you do this: # project/asgi.py
...
application = SentryWrapper(get_default_application()) Which is clear and explicit, and doesn't need any monkey patching. I don't then understand this comment at all:
What am I missing here? |
That's the difference I care about. This is the entire premise of this issue:
I believe @hishnash's issue is unrelated. |
OK, I understand. In that case, no, this isn't something I'd want to add. Given the choice between explicitly wrapping the ASGI application or opaquely monkey patching in, I would always opt for the former. I think we do no-one any real favours otherwise. Explicit beats implicit, and ASGI's one obvious way to do things is wrapping applications in this manner. Sorry. |
The third choice is to have global signals like Flask. I mentioned this possibility four times now. Honestly it's frustrating to engage with someone who doesn't bother to read anything that I write (and doesn't ask for clarification either in case my comments were too vague). Be that as it may, Django 3 will have the signals we need, so I guess time will solve this. |
Core Django, Flask and some other web frameworks have a way to register a callback whenever a request is handled or a response is sent out. This registry exists globally and independently of application state. It's invaluable to monitoring tools like Newrelic and Sentry, as their SDKs/Agents can just subscribe to signals at any point in time without requiring the user to register a middleware or really do any other kind of configuration (example). For frameworks/libraries where such signals don't exist, there is still the possibility of monkeypatching (example). I am affiliated with Sentry, as I work on the Sentry SDK for Python.
Channels offers no possibility to safely hook into the request/response lifecycle of the ASGI application it exports. Like most frameworks it has no global signals, but the routing mechanism is left up to the user in a way that makes it very hard to wrap the outermost ASGI application even through monkeypatching. Django also allows the user to wrap the Django-exported WSGI application in arbitrarily many layers of WSGI middleware that we cannot instrument, but Channels makes it a common pattern for the user to use ASGI middleware while it's uncommon enough in Django to ignore the issue there.
What I would like to see is the following:
channels.routing.get_default_application()
to wrap the discovered ASGI application in a middlewareChannelsApp
that does nothing but forward to the inner application. This already allows us to monkeypatch to achieve our goal.Even just the first change enables Sentry and similar tools to hook into channels before Django settings have been loaded. This point is crucial because:
The status-quo solution to this problem is to do one of the following:
Patch
AsgiHandler
exclusively if you only care about HTTPPatch
ProtocolTypeRouter
under the assumption that most people use this type to wrap everything elseUse some sort of trampoline to look into Django settings to discover
ASGI_APPLICATION
, and then patch that object. This opens two more choices:This is a bit involved because
ASGI_APPLICATION
can be any kind of object that might be callable without providing you a settable__call__
. For example it might be a primitive function, in which case patchingfunc_code
/__code__
might do the trick.Would love to know what you think about this. I am happy to prepare the patch for this.
Django's upcoming ASGI support seems to do much better in this regard, as there only
ASGIHandler
needs to be patched.The text was updated successfully, but these errors were encountered: