-
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
Conversation
neilkakkar
commented
Aug 29, 2024
•
edited
Loading
edited
def extract_person_data(self): | ||
headers = self.headers() | ||
return { | ||
"distinct_id": headers.get("X-PostHog-Distinct-ID"), |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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
client.integrations = [DjangoIntegration]
init(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.
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 comment
The 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
https://www.w3.org/TR/trace-context/
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.
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.
Choose a reason for hiding this comment
The 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 ,
or =
in them will go wonky