Sentry integration

[sondrelg]

I still need to:

• Bump version
• Write docs

but the core logic for this PR is done, and I think it would be good to review that before writing docs, to make sure you agree with the some of the design choices.

---

This PR introduces a new package setting, INTEGRATIONS which will be a list of pre-made integrations from the new integrations.py.

The idea for this PR originally, was only to add some functionality for integrating the correlation ID with Sentry, but I think it might be a good idea to design this feature in a way that opens up for more integrations in the future.

The second addition is the Sentry logic, which I've packaged in a class so that we can bundle validation logic and execution logic for each integration, and separate it from other code.

Also made some adjustments to doc-strings and inline comments here and there, but feel free to change them back

[JonasKs]

I think we should create a base class like they do in Sentry to ensure the integrations never deviate from each other.

Snippet from Sentry:

class Integration(object):
    """Baseclass for all integrations.
    To accept options for an integration, implement your own constructor that
    saves those options on `self`.
    """

    identifier = None  # type: str
    """String unique ID of integration type"""

In this project, it could look something like this:

class Integration(object):
    """
    Integration baseclass. 
    """

    identifier = None    # Name the identifier
    
    @staticmethod
    def run() -> None:
        """
        This is run when the integration is initialized in the clients settings.py.

        Put all validation logic here.
        """
        pass

This makes us able to:

• Set a standard.
• Allows us to push new integrations without typing the same documentation over and over again in every integration.
• Implement a debug log to django_guid which will log the name of all integrations on startup.

I haven't really tested anything or tried out this code, so I'll go more in depth on the actual solution tomorrow.

[sondrelg]

Added a base class + new tests. Put the base class in the init and moved the sentry integration to sentry.py. Everything still works the same.

---

One thing to note is that the SentryIntegration run method currently takes middleware_context as an argument - this is the self instance of the middleware class.

This was done intentionally so that we can run all integrations like this in the middleware:

        # Run all integrations
        for integration in settings.INTEGRATIONS:
            logger.debug('Running integration: `%s`', integration.identifier)
            integration.run(self)

This might not be the best implementation, but the idea here is that, while the sentry run method could have easily looked like this

    def run(self, guid) -> None:
        import sentry_sdk
        with sentry_sdk.configure_scope() as scope:
            logger.debug('Setting Sentry transaction_id to %s', guid)
            scope.set_tag('transaction_id', guid)

...instead of this

    def run(self, middleware_context) -> None:
        import sentry_sdk
        with sentry_sdk.configure_scope() as scope:
            guid = middleware_context.get_guid()
            logger.debug('Setting Sentry transaction_id to %s', guid)
            scope.set_tag('transaction_id', guid)

... this choice gives custom/new integrations access to the middleware itself, which means it's more flexible and less prone to rewrites. The one thing I think might be "wrong" with this implementation is that self probably shouldnt be passed as an arg, but should be inherited. I could explain this to you directly if you prefer.

[JonasKs]

Dig it! A few more notes and comments, though.

[JonasKs]

Perfect, just what I had in mind!!

I’m on my phone right now reading through, but passing self as an argument seems a bit whacky. Not sure if it really is, though.

How ever, I think the solution could be just as slick with Django Signals approach. We send the GUID as an argument, but every integration must allow kwargs, so that if we have to pass arguments to an integration at a later point we can do so without breaking anything.

https://docs.djangoproject.com/en/3.0/topics/signals/#receiver-functions

[sondrelg]

Not sure if this fully addresses the main problem which is that by sending the GUID in middleware.py, we will send a GUID, and only a GUID to all custom integrations

[JonasKs]

I can’t really see where this is an issue though. The middleware does close to nothing but stores a GUID. If that changes in the future, kwargs will allow for changes without breaking backwards compatibility. The internal API can be used if one wants to delete the GUID. (Requires an important, obviously, but it feels less whacky than passing self)

I’ll read up on passing self as an argument before totally dismissing the idea, but I suspect I’ll find that it’s a “nono”.

[JonasKs]

Actually, quickly looked into asyncio, seems like they’re doing it. Maybe just be me, and it’s totally okay to do it.🤷‍♂️
I’ll look more into it tomorrow.

[JonasKs]

I know we talked about this this morning but I like having everything written down:
Essentially it seems like it’s okay, but I feel like it’s safer to do kwargs considering backwards compatibility and to make it more clear what’s happening. So to send the guid and require all functions to allow kwargs seems to be the best idea to me.

[sondrelg]

I added kwarg and callability-validation, and put it in the config.py. Last we spoke, it was in the middleware, but I don't think it belongs there when thinking it through properly.

[JonasKs]

On phone so just read quickly over it. I’ll look into your comment and the code tomorrow :)
Looks like we’re super close tho!