getsentry
feat(django): add instrumentation for django signals
Hey @BeryJu ! Thanks for this contribution! On the first glance it looks great. Please fix the one problem the linter is reporting. I will see If I can review and test this properly this week, so we can get this merged!
Here is a request without the Signals instrumentation:
Here is the same request with the Signals instrumentation:
Hey @BeryJu !
I have now tried your signals instrumentation. See the screenshots above.
Are you creating a span for each signal that is triggered, right?
Because those timings are always 0.01 it is not really adding a lot of value.
But, would be it be very difficult to add a span for all the receiver functions? So we would get the timing of the actual functions that where running because of signals? This would add really a LOT of value for all the Django developers out there!
Hey @antonpirker, the PR should add a span for each signal receiver function. I used to run this modified library for https://github.com/goauthentik/authentik but have since reverted to the upstream SDK so I haven't actually tested this in a bit. I'll have a look this evening
I just re-checked and indeed a trace is created for each handler, the dotted-import-path for each entry is a signal handler (the times still seem a bit low though)
I just updated my sample project to actually doing some work in the signal receivers (added a time.sleep()) and now the screenshot looks like I envisioned it:
So this adds really value to Django projects!
Now we just need to find a way to instrument individual signal handlers with wrapping instead of overriding the send function and we are good. Having it like as is is not going to work, because it could change the behavior of users code when they update the django version and we never do that.
I added a comment with a possible solution to my review @BeryJu . Please have a look.
And a heads up: I will be on vacation next week and the following week. So this will probably be merged after my vacation.
Thanks for the quick update @BeryJu !
This is now working for .send() and .send_robut() right?
Unfortunately I will not be able to review it befor my 2 week vacation, but maybe a colleague (@sl0thentr0py ) picks it up and reviews it.
Hey @BeryJu ! Before I head into my vacation: if you send me your shipping address to: anton (dot) pirker at sentry |dot| io, I can send you a little token of appreciation. (after my vacation) :-)
Thanks for the quick update @BeryJu ! This is now working for
.send()and.send_robut()right?
Should work for both, I have not actually tested it myself though (will see if I get some time to do so this weekend). Funnily enough I'll also be on vacation for the next two weeks, so no worries about the review
Hey @BeryJu
Sorry for the delay! I will have a look at the PR tomorrow, promised!
The new behavior breaks some our our tests. Most notably this one: https://github.com/getsentry/sentry-python/blob/master/tests/integrations/django/test_basic.py#L687-L731
Could you change this test (and maybe other failing ones) to reflect the new behavior @BeryJu ?
HINT: Navigating the test errors is still a bit cumbersome. But If you click on the details of the "CI / Run Tests" check above and then search for "error:" you find the actual error message the fastest.
Thanks a lot!
Tests should be fixed now, also added type annotations
btw, stickers arrived, thank you very much!
That was fast! Amazing! Only the tests for old versions (Python 2.7 and 3.5) are failing. Can you please take a look @BeryJu ?
I will do some manual testing again, and then it will be released with the next version. (so probably early next week) Congratulations and Thankyou @BeryJu
Hey @BeryJu
Really great work. You are not by any chance a Go Freelancer and want to help us give our Go SDK a little love?