-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat(errors): Add django integration and in app frames #131
Changes from 7 commits
f4a3f85
9b82c9f
5c0cb9d
01f5228
9ca93f2
6f877dc
211ba07
901731a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
class IntegrationEnablingError(Exception): | ||
""" | ||
The integration could not be enabled due to a user error like | ||
`django` not being installed for the `DjangoIntegration`. | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
import re | ||
import sys | ||
from typing import TYPE_CHECKING | ||
|
||
from posthog.exception_integrations import IntegrationEnablingError | ||
|
||
try: | ||
from django import VERSION as DJANGO_VERSION | ||
from django.core import signals | ||
|
||
except ImportError: | ||
raise IntegrationEnablingError("Django not installed") | ||
|
||
|
||
if TYPE_CHECKING: | ||
from typing import Any, Dict # noqa: F401 | ||
|
||
from django.core.handlers.wsgi import WSGIRequest # noqa: F401 | ||
|
||
|
||
class DjangoIntegration: | ||
# TODO: Abstract integrations one we have more and can see patterns | ||
""" | ||
Autocapture errors from a Django application. | ||
""" | ||
|
||
identifier = "django" | ||
|
||
def __init__(self, capture_exception_fn=None): | ||
|
||
if DJANGO_VERSION < (4, 2): | ||
raise IntegrationEnablingError("Django 4.2 or newer is required.") | ||
|
||
# TODO: Right now this seems too complicated / overkill for us, but seems like we can automatically plug in middlewares | ||
# which is great for users (they don't need to do this) and everything should just work. | ||
# We should consider this in the future, but for now we can just use the middleware and signals handlers. | ||
# See: https://github.com/getsentry/sentry-python/blob/269d96d6e9821122fbff280e6a26956e5ed03c0b/sentry_sdk/integrations/django/__init__.py | ||
|
||
self.capture_exception_fn = capture_exception_fn | ||
|
||
def _got_request_exception(request=None, **kwargs): | ||
# type: (WSGIRequest, **Any) -> None | ||
|
||
extra_props = {} | ||
if request is not None: | ||
# get headers metadata | ||
extra_props = DjangoRequestExtractor(request).extract_person_data() | ||
|
||
self.capture_exception_fn(sys.exc_info(), extra_props) | ||
|
||
signals.got_request_exception.connect(_got_request_exception) | ||
|
||
def uninstall(self): | ||
pass | ||
|
||
|
||
class DjangoRequestExtractor: | ||
|
||
def __init__(self, request): | ||
# type: (Any) -> None | ||
self.request = request | ||
|
||
def extract_person_data(self): | ||
headers = self.headers() | ||
|
||
# Extract traceparent and tracestate headers | ||
traceparent = headers.get("traceparent") | ||
tracestate = headers.get("tracestate") | ||
|
||
# Extract the distinct_id from tracestate | ||
distinct_id = None | ||
if tracestate: | ||
# TODO: Align on the format of the distinct_id in tracestate | ||
# We can't have comma or equals in header values here, so maybe we should base64 encode it? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i dont remember the spec by heart but I know its possible base64 in some cases There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are just reading from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I meant client should encode (and thus decode here) as otherwise distinct ids with |
||
match = re.search(r"posthog-distinct-id=([^,]+)", tracestate) | ||
if match: | ||
distinct_id = match.group(1) | ||
|
||
return { | ||
"distinct_id": distinct_id, | ||
"ip": headers.get("X-Forwarded-For"), | ||
"user_agent": headers.get("User-Agent"), | ||
"traceparent": traceparent, | ||
} | ||
|
||
def headers(self): | ||
# type: () -> Dict[str, str] | ||
return dict(self.request.headers) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import pytest | ||
|
||
pytest.importorskip("django") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
from posthog.exception_integrations.django import DjangoRequestExtractor | ||
|
||
DEFAULT_USER_AGENT = ( | ||
"Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.3" | ||
) | ||
|
||
|
||
def mock_request_factory(override_headers): | ||
class Request: | ||
META = {} | ||
# TRICKY: Actual django request dict object has case insensitive matching, and strips http from the names | ||
headers = { | ||
"User-Agent": DEFAULT_USER_AGENT, | ||
"Referrer": "http://example.com", | ||
"X-Forwarded-For": "193.4.5.12", | ||
**(override_headers or {}), | ||
} | ||
|
||
return Request() | ||
|
||
|
||
def test_request_extractor_with_no_trace(): | ||
request = mock_request_factory(None) | ||
extractor = DjangoRequestExtractor(request) | ||
assert extractor.extract_person_data() == { | ||
"ip": "193.4.5.12", | ||
"user_agent": DEFAULT_USER_AGENT, | ||
"traceparent": None, | ||
"distinct_id": None, | ||
} | ||
|
||
|
||
def test_request_extractor_with_trace(): | ||
request = mock_request_factory({"traceparent": "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01"}) | ||
extractor = DjangoRequestExtractor(request) | ||
assert extractor.extract_person_data() == { | ||
"ip": "193.4.5.12", | ||
"user_agent": DEFAULT_USER_AGENT, | ||
"traceparent": "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01", | ||
"distinct_id": None, | ||
} | ||
|
||
|
||
def test_request_extractor_with_tracestate(): | ||
request = mock_request_factory( | ||
{ | ||
"traceparent": "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01", | ||
"tracestate": "posthog-distinct-id=1234", | ||
} | ||
) | ||
extractor = DjangoRequestExtractor(request) | ||
assert extractor.extract_person_data() == { | ||
"ip": "193.4.5.12", | ||
"user_agent": DEFAULT_USER_AGENT, | ||
"traceparent": "00-0af7651916cd43dd8448eb211c80319c-b7ad6b7169203331-01", | ||
"distinct_id": "1234", | ||
} | ||
|
||
|
||
def test_request_extractor_with_complicated_tracestate(): | ||
request = mock_request_factory({"tracestate": "posthog-distinct-id=alohaMountainsXUYZ,rojo=00f067aa0ba902b7"}) | ||
extractor = DjangoRequestExtractor(request) | ||
assert extractor.extract_person_data() == { | ||
"ip": "193.4.5.12", | ||
"user_agent": DEFAULT_USER_AGENT, | ||
"traceparent": None, | ||
"distinct_id": "alohaMountainsXUYZ", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be cool if the integration could configure itself.
The
Integration
interface has ainstall|uninstall
method that is called after the SDK is init.the
install
method in theDjangoIntegration
class does all the config.if the SDK close is called, we also call all
uninstall
methods from allintegrations
an example https://github.com/PostHog/posthog-android/blob/bb0f3d30cc3d9819000b43c281296a30483841fa/posthog-android/src/main/java/com/posthog/android/internal/PostHogActivityLifecycleCallbackIntegration.kt#L73-L77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe integrations can be also a property in the
client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was a bit reluctant to spend more time on this interface, given i'm not yet sure how most integrations will look like. If they need to be initialised post client initialisation / need some config options from users to happen first, would make sense. We can make breaking changes here though as long as its in alpha, so not too worried.