feat(django): add instrumentation for django signals

[BeryJu]

Add instrumentation for Django signals

[antonpirker]

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!

[antonpirker]

Here is a request without the Signals instrumentation:

Here is the same request with the Signals instrumentation:

[antonpirker]

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!

[BeryJu]

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

[antonpirker]

Here you basically replace the existing send() function with your own version.

Best practice is, that we just wrap the existing function and run our code before or after the existing function.

See this as a template: https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/starlette.py#L204-L218

[BeryJu]

yeah I wanted to originally just wrap the existing function for improved compatibility, but with that there's no way to instrument individual signal handlers

[antonpirker]

I guess it can work by patching _live_receivers() and instead of returning an array of receivers (=functions) we wrap all the elements of the array in a function that creates a span and then calls the old receiver.

Something like this:

old_live_receivers = Signal._live_receivers

def _sentry_live_receivers(self, sender)
    receivers = old_live_receivers(self, sender)

    def _receiver_with_span(*args, **kwargs):
        old_receiver = receivers[idx]
        name = old_receiver.__name__
        with hub.start_span(    
            op="django.signals",
            description=name,
        ) as span:
            span.set_data("signal", name)
            return old_receiver(*args, **kwargs)
            
    for idx, receiver in enumerate(receivers):
        receivers[idx] = _receiver_with_span

    return receivers

Signal._live_receivers = _sentry_live_receivers

Note: this is untested code, I just wrote this on top of my head without ever running it. But I did something similar in another integration a couple of days ago, so it might work.

[BeryJu]

yeah thats a good idea actually

[BeryJu]

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)

[antonpirker]

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.

[antonpirker]

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.

[antonpirker]

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.

[antonpirker]

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) :-)

[BeryJu]

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

[antonpirker]

Hey @BeryJu

Sorry for the delay! I will have a look at the PR tomorrow, promised!

[antonpirker]

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!

[BeryJu]

Tests should be fixed now, also added type annotations

btw, stickers arrived, thank you very much!

[antonpirker]

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 ?

[BeryJu]

Should be all good now, missed some special cases for older django versions

[antonpirker]

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

[antonpirker]

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?

[antonpirker]

If yes, you can message me at anton dot pirker ät sentry dot io