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

feat: v2 analytics and split testing #247

Closed
wants to merge 21 commits into from
Closed

Conversation

kyle-ssg
Copy link
Member

@kyle-ssg kyle-ssg commented Sep 10, 2024

  • Uses the new v2 analytics endpoint
  • Exposes a trackEvent function, this will let applications record conversion events e.g. checkout and have them recorded in split testing analytics

To opt in, this adds splitTestingAnalytics to flagsmith.init

@kyle-ssg kyle-ssg marked this pull request as draft September 10, 2024 16:36
@kyle-ssg kyle-ssg changed the title feat: v2 analytics feat: v2 analytics and split testing Sep 10, 2024
@kyle-ssg kyle-ssg marked this pull request as ready for review September 10, 2024 19:31
Copy link
Member

@rolodato rolodato left a 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>;
Copy link
Member

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.

Copy link
Member Author

@kyle-ssg kyle-ssg Sep 18, 2024

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.

Copy link
Member

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
@matthewelwell
Copy link
Contributor

Since we're deprioritising this work in the short-medium term, I'm going to close this PR.

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