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

Add python sentry (and sentry & django) integrations #13

Merged
merged 7 commits into from
May 17, 2021

Conversation

macobo
Copy link
Contributor

@macobo macobo commented Oct 10, 2020

This creates a PostHogIntegration to use inside sentry_sdk to capture events in PostHog, and a PostHogDistinctIdMiddleware which allows creating links to PostHog in Sentry.

Usage examples in README.md and sentry_django_example repo.

For using inside PostHog/posthog, it would look like this(settings.py):

+POSTHOG_DJANGO = {
+    "distinct_id": lambda request: request.user and request.user.distinct_id
+}
+
 if not TEST:
     if os.environ.get("SENTRY_DSN"):
         # https://docs.sentry.io/platforms/python/
         sentry_sdk.init(
             dsn=os.environ["SENTRY_DSN"],
-            integrations=[DjangoIntegration(), CeleryIntegration(), RedisIntegration()],
+            integrations=[DjangoIntegration(), CeleryIntegration(), RedisIntegration(), PostHogIntegration()],
             request_bodies="always",
+            before_send=update_posthog
         )

 if get_bool_from_env("DISABLE_SECURE_SSL_REDIRECT", False):
@@ -210,6 +216,7 @@ MIDDLEWARE = [
     "django.contrib.messages.middleware.MessageMiddleware",
     "django.middleware.clickjacking.XFrameOptionsMiddleware",
     "whitenoise.middleware.WhiteNoiseMiddleware",
+    "posthoganalytics.sentry.django.PosthogDistinctIdMiddleware"
 ]

Testing this using unit tests or something similar is hard. Open to suggestions, I've created a manual end-to-end test which integrates with a Sentry + PostHog installation.

How it looks on the main repo (settings.py):

```
+POSTHOG_DJANGO = {
+    "distinct_id": lambda request: request.user and request.user.distinct_id
+}
+
 if not TEST:
     if os.environ.get("SENTRY_DSN"):
         # https://docs.sentry.io/platforms/python/
         sentry_sdk.init(
             dsn=os.environ["SENTRY_DSN"],
-            integrations=[DjangoIntegration(), CeleryIntegration(), RedisIntegration()],
+            integrations=[DjangoIntegration(), CeleryIntegration(), RedisIntegration(), PosthogSentryIntegration()],
             request_bodies="always",
+            before_send=update_posthog
         )

 if get_bool_from_env("DISABLE_SECURE_SSL_REDIRECT", False):
@@ -210,6 +216,7 @@ MIDDLEWARE = [
     "django.contrib.messages.middleware.MessageMiddleware",
     "django.middleware.clickjacking.XFrameOptionsMiddleware",
     "whitenoise.middleware.WhiteNoiseMiddleware",
+    "posthoganalytics.sentry.django.PosthogDistinctIdMiddleware"
 ]
```
@@ -0,0 +1,24 @@
from sentry_sdk import configure_scope
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# settings.py
middlewares += ['ourSentryMiddleWare']

POSTHOG_DJANGO = {
  distinct_id: lambda request: request.user.id
}

@neilkakkar neilkakkar moved this from Ongoing Sprint (1.25 2/2) to In Progress in Extensibility May 7, 2021
@neilkakkar
Copy link
Collaborator

An overall question about this ticket: Why isn't this a part of core PostHog? Since it's a middleware that depends on

  1. Running only PostHog on Django with Sentry
  2. The specific middleware added to the list

and can't be used anywhere else.

Maybe https://github.com/PostHog/posthog/blob/master/posthog/middleware.py is a better place to add this?

@mariusandra
Copy link
Contributor

I think the original idea between "adding sentry to X" has been that if you are building your own python app, and use both posthog and sentry, the two libraries could coordinate. What this means is that you would have a link to the user in posthog in sentry, and on the other side PostHog would also capture the error in a $exception event.

I think that's how it works with the posthog-js library, but double check to be sure. I think it was @timgl and @macobo (and or @yakkomajuri ?) who built the original JS sentry integration.

@neilkakkar
Copy link
Collaborator

neilkakkar commented May 7, 2021

Ah, that makes some sense.

So, the idea is to integrate posthog-python and sentry together, such that posthog.capture is included inside Sentry via a PostHogSentryIntegration, which has nothing to do with the main Django PostHog app. (while at the same time, adding the posthog_distinct_id to Sentry as well)

I think that's what posthog-js is doing: https://github.com/PostHog/posthog-js/blob/6bf9b1e0f59849cae9a1e00e56092a06842a8142/src/posthog-core.js#L1391-L1415

Edit: .. Diving deeper into posthog-js, things seem a bit weirder: posthog-js generates its own distinct_ids (for things like autocapture?) while posthog-python is stateless - it depends on user input to tell it what distinct_ids to work with. We could work around this by creating an Integration that takes in a callable (which must be user defined), that returns the current distinct_id.

Am I missing something obvious?

@macobo
Copy link
Contributor Author

macobo commented May 7, 2021

We could work around this by creating an Integration that takes in a callable (which must be user defined), that returns the current distinct_id.

There's no good way to figure that out from the global context, hence the django middleware (you can get the user from request)

@neilkakkar
Copy link
Collaborator

I guess the part I don't understand is: there's no django middleware on posthog-python. We're dealing directly with the API. So, how does this work?

So, let's say someone's building HogFlixApp, and using both, Sentry and PostHog. So, in their Flask app, they

import posthog
import sentry_sdk

sentry_sdk.init(integrations=[posthog.sentry.PostHogSentryIntegration(get_distinct_id?)]

which somehow sends things to both Sentry and PostHog.

@macobo
Copy link
Contributor Author

macobo commented May 7, 2021

To get request-specific information in sentry from flask, it also installs a middleware: https://github.com/getsentry/sentry-python/blob/master/sentry_sdk/integrations/flask.py

You hit the nail on the head before:

  1. You need user information to provide any value with the integration
  2. There's no real way to get that user information with "vanilla" python without user hooking posthog-python into $framework_of_choice AND sentry

If you're still confused happy to pass on context on monday or let marius do it. :)

@neilkakkar
Copy link
Collaborator

neilkakkar commented May 7, 2021

.. Okay, that clears things up a lot! So, it's for someone else building HogFlixApp only using Django with Sentry, and not just plain python / some other framework.

And after that expansion, compressing information back into the name of the ticket makes sense again 😂.

The point of confusion was the main repo: it's not our main PostHog repo, but randomDjangoApp.

@neilkakkar neilkakkar marked this pull request as ready for review May 10, 2021 12:39
@neilkakkar
Copy link
Collaborator

@macobo requesting review (can't add you the usual way since you're the PR creator)

@neilkakkar
Copy link
Collaborator

Friendly ping, @yakkomajuri xD

Copy link
Contributor

@yakkomajuri yakkomajuri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit otherwise looks good to me. Would be good to get some tests though although I appreciate it might be a tiny bit annoying to set up


from posthog.sentry.posthog_integration import PostHogIntegration

PostHogIntegration.organisation = "posthog" # TODO: your sentry organisation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we substitute all instances of "organisation" for the american spelling "organization"? Just to be consistent since we refer to it as the latter everywhere in the product

@neilkakkar
Copy link
Collaborator

re: tests - apologies if it wasn't clear: there is a working end to end test (hence the sample Django app setup), it just isn't automated, since it involves setting up Sentry DSN + PostHog keys on the fly. (and replication instructions in README)

I can't unit-test this works, since the Integration is managed by Sentry, and the middleware is managed by Django, neither of which is in our control.

But, if you have any ideas on how to go about it - all ears!

@neilkakkar neilkakkar merged commit fbde5ca into master May 17, 2021
@neilkakkar neilkakkar deleted the sentry-django-integration branch May 17, 2021 13:08
@mariusandra mariusandra moved this from In Progress to Done this sprint in Extensibility May 18, 2021
@Twixes Twixes moved this from Done this sprint to Done in Extensibility May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Extensibility
Done Historically
Development

Successfully merging this pull request may close these issues.

None yet

4 participants