feat(errors): Add django integration and in app frames #131
Conversation
neilkakkar
commented
Aug 29, 2024
•
neilkakkar
commented
| def extract_person_data(self): | ||
| headers = self.headers() | ||
| return { | ||
| "distinct_id": headers.get("X-PostHog-Distinct-ID"), |
There was a problem hiding this comment.
hmm, this doesn't work as well as I'd hoped, because the client side requests coming in don't have this header, only our SDK requests coming in will, which is useless 🤦.
Need something like we did with the X-posthog-session-id, but automatic, where requests from a clients app add this header. Need to research if this is possible. But the good thing is, the django integration part is covered - if the requests have it, it will assign it correctly.
There was a problem hiding this comment.
there are specs for this already, see PostHog/posthog#20026 (trace-context, or baggage), I'd say we just follow the spec instead of having our own.
hmm, this doesn't work as well as I'd hoped, because the client side requests coming in don't have this header, only our SDK requests coming in will, which is useless 🤦.
yes, but this is how it works, the distributed tracing only works if you have the SDK on both ends, either ours or any SDK that implements the trace-context, or baggage spec
There was a problem hiding this comment.
I don't think that's useful enough, at minimum we need to then make it easier to do this 🤔 .
Will update this to use trace-state though, thanks 👍
There was a problem hiding this comment.
yep, if the SDK is on both sides, everything should be automatic via monkey patching whenever possible, or easy enough via docs, that's the state of the art today, otherwise there's no way to connect the dots on both ends
| for integration in integrations or []: | ||
| # TODO: Maybe find a better way of enabling integrations | ||
| # This is very annoying currently if we had to add any configuration per integration | ||
| if integration == Integrations.Django: | ||
| try: | ||
| from posthog.exception_integrations.django import DjangoIntegration | ||
|
|
||
| enabled_integration = DjangoIntegration(self.exception_receiver) | ||
| self.enabled_integrations.append(enabled_integration) | ||
| except Exception as e: | ||
| self.log.exception(f"Failed to enable Django integration: {e}") |
There was a problem hiding this comment.
it'd be cool if the integration could configure itself.
integrations = [DjangoIntegration]
init(client, integrations)
The Integration interface has a install|uninstall method that is called after the SDK is init.
the install method in the DjangoIntegration class does all the config.
if the SDK close is called, we also call all uninstall methods from all integrations
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.
maybe integrations can be also a property in the client
client.integrations = [DjangoIntegration]
init(client)
There was a problem hiding this comment.
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.
| 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.
i dont remember the spec by heart but I know its possible base64 in some cases
https://www.w3.org/TR/trace-context/
There was a problem hiding this comment.
we are just reading from tracestate so we don't need to encode anything, or rather, should we decode? it depends on what the client does
There was a problem hiding this comment.
yeah I meant client should encode (and thus decode here) as otherwise distinct ids with , or = in them will go wonky
