-
Notifications
You must be signed in to change notification settings - Fork 57
feat: add beforeSend hook #255
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
base: main
Are you sure you want to change the base?
Conversation
| /// Callback to intercept and modify events before they are sent to PostHog. | ||
| /// | ||
| /// Return a possibly modified event to send it, or return `null` to drop it. | ||
| typedef BeforeSendCallback = PostHogEvent? Function(PostHogEvent event); |
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 think this type should be `FutureOr<PostHogEvent?> since people might need to use async methods to filter stuff
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.
Not sure about this one since we've talked about eventually moving away from async method channels. This would complicate the capture pipeline potentially. Plus all the other sdks are sync hooks?
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.
agreed, but right now even our sdk is async, so if you call isFeatureFlag something is async, which maybe its an use case to be used for filtering things, so we need to make it FutureOr which allows to make it async or not.
Ps I used many async methods in the past to filter out events.
I think when we redesign to be sync first, we'd figure this out differently.
| /// Represents an event that can be modified or dropped by the [BeforeSendCallback]. | ||
| /// | ||
| /// This class is used in the beforeSend callback to allow modification of events before they are sent to PostHog. | ||
| class PostHogEvent { |
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.
at least on the other SDKs, this event has a specific map for $set and $set_once
i think we should do the same
see https://github.com/PostHog/posthog-js/pull/2931/files#diff-5415311d4de5614e8deee6eddaa62716f3528992c20787a89433c2b99d2cdc9a capture event
not a must, just wanted to share this
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 think I'll skip this for now because anw those props are set from native layer and would not be present on beforeSend callback Dart side. I think we skipped/missed this on iOS and Android as well
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.
Ah just merging main and remembered we just added userProperties and userPropertiesSetOnce to capture(). So this would make sense now
lib/src/posthog_config.dart
Outdated
| /// **Note:** | ||
| /// - This callback runs synchronously on the Dart side | ||
| /// - Exceptions in the callback will cause the event to be sent unchanged | ||
| BeforeSendCallback? beforeSend; |
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.
the other SDKs we allow one function or a list of tunction, we dont support the list of functions here
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.
Ah good point
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.
Went with List<BeforeSendCallback> beforeSend = []; here:
- simple API, single callback is just [fn], multiple is [fn1, fn2]
- this is usually set once on setup so I stayed away from mutation APIs like setCallback(), remoteCallback() etc
# Conflicts: # CHANGELOG.md # lib/src/posthog_flutter_io.dart
💡 Motivation and Context
Closes #155
Docs: PostHog/posthog.com#14482
Adds a
beforeSendcallback toPostHogConfigthat lets you drop or modify events before they're sent to PostHog.Initially tried implementing this on the native side with method channels calling back to Dart in an attempt to also capture native initiated events, but hit timing issues since method channels are async but the beforeSend hook is called synchronously from native side. Also explored FFI but it added way too much complexity for the benefit.
For now, decided to keep it dart only so that it stays fast, synchronous and reliable. Should be good for the majority of the use cases.
Limitations in changelog
💚 How did you test it?
📝 Checklist