-
Notifications
You must be signed in to change notification settings - Fork 231
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
Update - refactor telemetry tracking logic in oss #1069
Conversation
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.
Thanks for the quick PR @aybruhm !
Code wise, I approve it. However, there are failing tests. Can you please look into this.
@devgenix can you please test the implementation (serve from cli (current version) then use in web cloud version to check for any bugs). Thank you! |
@devgenix please see above |
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.
@devgenix can you look more into this?! |
I just checked out main and it's happening there too. |
@devgenix and? |
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 did a re-check after noticing the error to be happening in other branches too.
It happens that the error I was getting earlier was coming from the clashing dependencies of Langchain and Openai in my local environment. Once I resolved it and re-tested this, all was good.
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.
@aybruhm sorry for the very late review. Please see the comment
|
||
export const generateOrRetrieveDistinctId = (): string => { | ||
if (typeof localStorage !== "undefined") { | ||
let distinctId = localStorage.getItem("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.
why do we need to generate a posthog distinct id in this case, I though if we leave it empty posthog would take care of all of this automatically
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.
@aybruhm reminder
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 made changes because PostHog wasn't keeping IDs unique for user requests.
When I tested it without the current code changes, I noticed that after a while, PostHog started using a different ID for the events been triggered. So, I figured it'd be better to generate a UUID for each user, stick it to their browser, and let PostHog use that as their distinct ID. This way, even if a user takes off for a few days and comes back (unless they clear their browser storage), PostHog will keep on tracking them with the generated UUID as their 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.
Thanks for the PR @aybruhm and sorry for the late review
Description
This PR refactors the telemetry tracking logic in OSS.
Related Issue
Closes #199_ag-cloud