feat(flags): adding support for GeoIP and person property overrides to rust flag service #24536
Conversation
…osthog into feat/make-rust-decide-curlable
…osthog into feat/make-rust-decide-curlable
… the config, rather than just existing as a standalone service
| @@ -165,6 +165,12 @@ jobs: | |||
| DATABASE_URL: 'postgres://posthog:posthog@localhost:5432/posthog' | |||
| run: cd ../ && python manage.py setup_test_environment --only-postgres | |||
|
|
|||
| - name: Download MaxMind Database | |||
There was a problem hiding this comment.
Is there versioning for this database? I dunno how the database gets into our self-hosted mmdbcdn.posthog.net URL
There was a problem hiding this comment.
lol idk I pulled it from here: https://github.com/PostHog/posthog/blob/master/.github/actions/run-backend-tests/action.yml#L125-L132
There was a problem hiding this comment.
yes its updated weekly, we use latest, https://github.com/PostHog/http-mmdb
| assert_eq!(redis_flag.deleted, pg_flag.deleted); | ||
| assert_eq!(redis_flag.key, pg_flag.key, "Flag key mismatch"); | ||
| assert_eq!( | ||
| redis_flag.name, pg_flag.name, |
There was a problem hiding this comment.
👍🏾 Good call on adding more descriptive assert messages here.
| if let Some(person_overrides) = &self.person_property_overrides { | ||
| // Check if all required properties are present in the overrides | ||
| // and none of them are of type "cohort" | ||
| let should_prefer_overrides = properties |
There was a problem hiding this comment.
Interesting, to confirm, this checks that all the person properties from the request payload match the spec for a feature flag and short-circuits the database call?
Can we add a metric here? I'm curious how many requests submit all properties needed to evaluate a feature flag.
There was a problem hiding this comment.
metrics are coming in a later PR, but I'll add a TODO to add a counter here, it's a good place for it.
There was a problem hiding this comment.
Interesting, to confirm, this checks that all the person properties from the request payload match the spec for a feature flag and short-circuits the database call?
Yes, that's how the functionality works.
There was a problem hiding this comment.
We automatically do this for any call from the SDKs where props have been added. It's also the only way to subvert the delay with ingesting properties problem.
There was a problem hiding this comment.
also note that since conditions are OR'ed, as long as even one property is sent where the condition matches, you can short circuit flag evaluation without needing all properties.
| @@ -243,6 +299,15 @@ impl FeatureFlagMatcher { | |||
|
|
|||
| Ok(props) | |||
| } | |||
|
|
|||
| // async fn get_group_properties_from_cache_or_db( | |||
There was a problem hiding this comment.
I would probably stash these plan for group_properties locally and remove them from the PR.
There was a problem hiding this comment.
yeah that PR is ready now too, so I don't think it's worth doing since it'll be a fast follow
| async fn del(&self, k: String) -> Result<(), CustomRedisError> { | ||
| let mut conn = self.client.get_async_connection().await?; | ||
|
|
||
| let results = conn.del(k); |
There was a problem hiding this comment.
I'm not very familiar with Rust's redis library, do we need to check here if the key exists or does this client library do that by itself?
There was a problem hiding this comment.
this wraps Redis' API, in which DEL is noop if there's no key.
| info!("Attempting to open GeoIP database at: {:?}", geoip_path); | ||
|
|
||
| let reader = Reader::open_readfile(&geoip_path)?; | ||
| info!("Successfully opened GeoIP database"); |
There was a problem hiding this comment.
I can't tell from the calling code, and that might be my own ignorance showing 😄
Do we read this database on every call to the GeoIPService? and do we need to close the reader somewhere or does the caller of this service close the handle to the geoIp file somewhere?
There was a problem hiding this comment.
we create this connection once, on startup. See here:
#[derive(Clone)]
pub struct State {
// TODO add writers when ready
pub redis: Arc<dyn RedisClient + Send + Sync>,
pub postgres: Arc<dyn DatabaseClient + Send + Sync>,
pub geoip: Arc<GeoIpService>,
}
pub fn router<R, D>(redis: Arc<R>, postgres: Arc<D>, geoip: Arc<GeoIpService>) -> Router
where
R: RedisClient + Send + Sync + 'static,
D: DatabaseClient + Send + Sync + 'static,
{
let state = State {
redis,
postgres,
geoip,
};
Router::new()
.route("/flags", post(v0_endpoint::flags).get(v0_endpoint::flags))
.with_state(state)
}(from router.rs)
Every subsequent call of get_geoip_properties hits that same in-memory database.
There was a problem hiding this comment.
(nit: maybe I should call it a GeoIpClient instead of a GeoIpService)
Co-authored-by: Phani Raj <phani@posthog.com>
…o rust flag service (#24536) Co-authored-by: Phani Raj <phani@posthog.com>
Changes
See this TODO list for features I'm burning down
flag_request,request_handler, andv0_endpointsa bit to separate concerns and make testing easierDoes this work well for both Cloud and self-hosted?
No impact yet, no one is using this service.
How did you test this code?
Manual tests demonstrated here: https://posthog.slack.com/archives/C034XD440RK/p1724364084555299
wrote a bunch of new unit tests, too.