-
Notifications
You must be signed in to change notification settings - Fork 33
🌿Fern -- feat: add feature flags with local evaluation, strongly typed errors, and smart endpoint configuration #46
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
…nitial-refactor-before-adding-feature chore: strongly typed errors with discrimination
…onfig-system feat: smart endpoint configuration system
…to feat/feature-flags
feat(flags): add feature flags with local evaluation, automatic `$feature_flag_called` event tracking support
oliverb123
left a comment
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.
A few more small pieces on shaping the public api, and removing some debug logging, but overall looks good
|
|
||
| /// Configuration options for the PostHog client. | ||
| #[derive(Debug, Clone)] | ||
| pub struct ClientOptions { |
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.
We should expose far less of this - none of these fields should be pub, maybe pub(crate) but definitely not part of the SDKs public API
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.
True made all of them pub(crate)
src/lib.rs
Outdated
| pub use client::ClientOptionsBuilderError; | ||
|
|
||
| // Endpoints | ||
| pub use endpoints::{ |
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.
Most of this can also stay private I think - stuff like Endpoint and EndpointManager you're being forced to expose because of all the pub fields in the config struct, but they're implementation details. At most, here, I'd expose a constant for the default host, plus the US and EU hostnames.
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.
only export
pub use endpoints::{DEFAULT_HOST, EU_INGESTION_ENDPOINT, US_INGESTION_ENDPOINT};
everything is pub(crate) now
| pub endpoint_manager: EndpointManager, | ||
| } | ||
|
|
||
| impl ClientOptions { |
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.
As above, the vast majority of this can be pub(crate) or not pub at all.
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.
same as above, all of them are pub(crate)
src/client/async_client.rs
Outdated
|
|
||
| (Some(LocalEvaluator::new(cache)), Some(poller)) | ||
| } else { | ||
| eprintln!("[FEATURE FLAGS] Local evaluation enabled but personal_api_key not set"); |
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.
We should never hit this case - it should be impossible to set enable_local_evaluation to true without having also provided a personal api key, and the client options builder should return an error from build if that does happen.
Aside from that, logging straight to eprintlin is also a totally inappropriate thing to do - we should either hook into tracing, or fail silently, but in practice as mentioned above, this invalid state should be unreachable, and you should be able to safely unwrap on the key in the present of enable_local_evaluation
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.
Added the checks and error handling before setting enable_local_evaluation to true
src/client/config.rs
Outdated
|
|
||
| /// Build the ClientOptions, validating all fields | ||
| pub fn build(self) -> Result<ClientOptions, Error> { | ||
| #[allow(deprecated)] |
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 are you returning the deprecated error enum here? I feel like we should be returning one of the new error types?
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.
updated with new Error enums
src/client/async_client.rs
Outdated
| let mut event = Event::new("$feature_flag_called", distinct_id); | ||
|
|
||
| // Add required properties | ||
| event.insert_prop("$feature_flag", flag_key).ok(); |
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.
Make this function return a result, and use the question mark operator here and below
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.
both async and sync will return Result<(), Error> now,
and use ?.
src/client/async_client.rs
Outdated
| // Flag not found locally, fall through to API | ||
| } | ||
| Err(e) => { | ||
| eprintln!( |
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.
We should never log like this. Either fallback to the api silently, or integrate tracing, but for now I'd go with silent. Applies to all later uses too
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.
removed all logs, will fallback the api silently
src/client/async_client.rs
Outdated
| } | ||
|
|
||
| /// Get a feature flag payload for a user | ||
| pub async fn get_feature_flag_payload<K: Into<String>, D: Into<String>>( |
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 should not be part of the public api
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.
updated to pub(crate)
src/client/async_client.rs
Outdated
| } | ||
|
|
||
| /// Get a specific feature flag value with control over event capture | ||
| pub async fn get_feature_flag_with_options<K: Into<String>, D: Into<String>>( |
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.
Let's make this not part of the public API for now, only pub(crate) or private. Users should only control whether the flag event is sent or not in the config.
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.
update to pub(crate)
| @@ -1,63 +1,516 @@ | |||
| use std::collections::{HashMap, HashSet}; | |||
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.
Assuming this is 1:1 with the async client above, most of the same comments probably apply re: particular functions being public/private and removing eprintln invocations.
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.
Yes, all chnages will reflect in both async and sync code.
fix: Add `ClientOptionsBuilderError` backward compatibility and fix endpoint override bug
…to feedback-fix
refactor: Improve configuration API, error handling, and local evaluation validation
Summary
This bundle brings the following to the PostHog Rust SDK, making the SDK feature-complete and developer-friendly.
Feature Flags with Local Evaluation
This adds feature flag support to the PostHog Rust SDK, including local evaluation, automatic
$feature_flag_calledevent tracking, and extensive test coverage.Key Features
$feature_flag_calledevents with intelligent deduplicationClientOptionsBuilderCore Implementation
New Files:
src/feature_flags.rs: Core feature flag evaluation logic with property matching operatorssrc/local_evaluation.rs: Local caching and evaluation system with background pollingsrc/endpoints.rs: PostHog API endpoint definitionsEnhanced Files:
src/client/blocking.rs: Feature flag methods with event trackingsrc/client/async_client.rs: Async feature flag supportsrc/client/mod.rs: Shared configuration and client optionsEvent Tracking System
$feature_flag_calledevent capture for all flag evaluationsdistinct_id + flag_key + responsecombinationArc<RwLock>with configurable size limits (default: 10,000 entries)send_feature_flag_eventsparameterTesting & Quality
Test Suites:
tests/test_async.rs: Comprehensive async client teststests/test_blocking.rs: Blocking client teststests/test_local_evaluation.rs: Local evaluation teststests/test.rs: E2E tests with real PostHog APIExamples:
examples/feature_flags.rs: A/B testing, property targeting, multivariate flagsexamples/local_evaluation.rs: Local evaluation patterns and performanceexamples/feature_flag_events.rs: Event tracking demonstrationsexamples/advanced_config.rs: Configuration optionsStrongly Typed Errors
Refactors the PostHog Rust SDK's error handling for better developer experience and maintainability.
Architecture Changes
Before:
After:
Benefits:
is_retryable(),is_client_error())thiserrorfor better error messages and source chain support (#[from])Key Improvements
1. Type-Safe Pattern Matching
Before:
After:
2. Structured Error Data
Before:
After:
3. Automatic Error Conversion
Before:
After:
4. Intelligent Retry Logic
Before:
After:
Backward Compatibility
No breaking changes - This is a backward-compatible refactor. All existing code will continue to work with deprecation warnings.
Tests added:
tests/api_compatibility.rs: API compatibility validationtests/backward_compatibility.rs: Backward compatibility verificationSmart Endpoint Configuration
Implements a flexible configuration system for the PostHog Rust SDK that accepts base URLs and automatically handles endpoint paths. This enables proper use of both
single-eventandbatchendpoints.Architecture Transformation
Previous approach: Required full endpoint URL (e.g.,
https://us.i.posthog.com/i/v0/e/)/i/v0/e/and/batch/endpointsNew approach: Accept base URL and normalize automatically
https://us.posthog.com/i/v0/e/for single events,/batch/for batchesBefore
SDK required full endpoint URLs, preventing simultaneous use of different endpoints:
This design prevented the SDK from using both
/i/v0/e/(single events) and/batch/(batch events) simultaneously.After
Accept base URL, SDK appends the correct path automatically:
Backward Compatibility
Breaking Changes
None - All changes are backward compatible. Existing code will continue to work, though some patterns are deprecated with warnings to guide migration to the new APIs.