-
Notifications
You must be signed in to change notification settings - Fork 39
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: v2 analytics and split testing #247
Conversation
# Conflicts: # flagsmith-core.ts
# Conflicts: # flagsmith-core.ts # lib/flagsmith-es/package.json # lib/flagsmith/package.json # lib/react-native-flagsmith/package.json
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.
Left some armchair-reviewer comments, I'm really not familiar with this v2 API yet :)
* Only available for self hosted split testing analytics. | ||
* Track a conversion event within your application, used for split testing analytics. | ||
*/ | ||
trackEvent: (event: string) => Promise<void>; |
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 it's safe to assume that events could be recorded at a much higher rate than flags are evaluated, so they should definitely be batched and flushed in the background. The current implementation is probably good enough to validate the idea in a PoC but I would exclude this method from this SDK's public/versioned API until we can implement event batching.
This method being async is also not ergonomic to use on event handlers for DOM events like clicking, hovering or navigating.
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 can't think of a case where we'd track events often, and even if they were it'd happen way less than flag evaluations since they'd occur potentially every render. The only usecase I can imagine is conversions e.g. checkout.
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.
Another use case for batching (or decoupling tracking from pushing) is for mobile/low-powered devices, where you might want to either not send events or reduce the rate at which they're sent when running on battery or on metered connections. This is a tradeoff between metrics accuracy and device impact that customers need to make, so I'd like to have some API that lets them make this decision.
I suppose nothing prevents customers from implementing the batching themselves if they really want, so maybe we could rename this to pushEvent
and add trackEvent
/flushEvents
/whatever later. At least that way it's clearer what this method does, and leaves the door open for a batched API in the future.
# Conflicts: # flagsmith-core.ts # lib/flagsmith-es/package.json # lib/flagsmith/package.json # lib/react-native-flagsmith/package.json # test/test-constants.ts # types.d.ts
Since we're deprioritising this work in the short-medium term, I'm going to close this PR. |
trackEvent
function, this will let applications record conversion events e.g. checkout and have them recorded in split testing analyticsTo opt in, this adds
splitTestingAnalytics
to flagsmith.init