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 UUID to events, based on Segment's messageId #20

Merged
merged 2 commits into from
May 6, 2024
Merged

Conversation

bretthoerner
Copy link
Contributor

@bretthoerner bretthoerner commented Apr 29, 2024

Internal context: https://posthoghelp.zendesk.com/agent/tickets/12556

Segment documentation about uuidv5.uuidv5 being available: https://segment.com/docs/connections/functions/destination-functions/#runtime-and-dependencies

We are using their messageId to generate a UUID to remove duplicates.


The repo says to ping Tim for updates. He (or whoever can) should probably run this against real test data inside of Segment's app. It seems like they have a nice interface in there for testing this out?

@bretthoerner bretthoerner force-pushed the brett/uuid branch 2 times, most recently from 9265f5d to d8b2e48 Compare April 29, 2024 22:01
@@ -45,6 +45,9 @@ async function onTrack(event, settings) {
const endpoint = new URL(`${baseUrl}/capture/`)
endpoint.searchParams.set('ts', event.timestamp)

const namespace = uuidv5.uuidv5('null', 'PostHog', true)
const uuid = uuidv5.uuidv5(namespace, event.messageId)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

uuidv5 docs show this example for making a custom namespace: https://www.npmjs.com/package/uuidv5

(I don't think it's worth worrying about whether we can cache in global state or anything like that...)

@bretthoerner bretthoerner requested review from tiina303 and xvello April 29, 2024 22:26
Copy link

@tkaemming tkaemming left a comment

Choose a reason for hiding this comment

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

Makes sense to me, assuming our understanding of the available dependency is correct…

Copy link

@xvello xvello left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! We'll have to get a new version reviewed by the Segment folks, I'd love to bundle the other change to add $geoip_disable to groupidentify events. They are currently going through person processing due to the geoip $sets while it's not needed.

@bretthoerner
Copy link
Contributor Author

I'd love to bundle the other change to add $geoip_disable to groupidentify events. They are currently going through person processing due to the geoip $sets while it's not needed.

Can you verify 99c7c66 ?

@bretthoerner bretthoerner merged commit d321792 into main May 6, 2024
1 check passed
@bretthoerner bretthoerner deleted the brett/uuid branch May 6, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants