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(flags): support groups and payloads correctly in new /flags service #26817

Merged
merged 31 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
029061f
first pass of easy cleanups
dmarticus Nov 21, 2024
8ed5d01
Merge branch 'master' into chore/optimize-new-flags-service
dmarticus Nov 25, 2024
f111c7d
Merge branch 'master' into chore/optimize-new-flags-service
dmarticus Nov 26, 2024
172a674
made some code review-related changes
dmarticus Nov 26, 2024
574d4a6
fix some formatting
dmarticus Nov 26, 2024
386ff7b
clip y
dmarticus Nov 26, 2024
9068c0b
more metrics more metrics
dmarticus Nov 26, 2024
ef78f6b
yeah
dmarticus Nov 26, 2024
ffe10d0
fix
dmarticus Nov 26, 2024
6f47ee7
added two tests
dmarticus Nov 26, 2024
0156612
few more things
dmarticus Nov 27, 2024
f255203
merge conflict resolution
dmarticus Nov 27, 2024
21a9e25
will this commit?
dmarticus Nov 27, 2024
94d15e8
Merge branch 'feat/metrics-for-common-flag-paths' into feat/integrati…
dmarticus Nov 27, 2024
d791a03
fixing bugs in group matching
dmarticus Dec 2, 2024
c209683
Merge branch 'master' into feat/integration-tests-for-new-flags
dmarticus Dec 3, 2024
048c4db
so close, but still need to grok group_key
dmarticus Dec 4, 2024
897acd2
Merge branch 'master' into feat/support-groups-and-payloads
dmarticus Dec 6, 2024
bec7fa9
yeesh
dmarticus Dec 10, 2024
53994b2
Merge branch 'master' into feat/support-groups-and-payloads
dmarticus Dec 10, 2024
1da6644
got my house in order t god
dmarticus Dec 10, 2024
61033cb
not idiomatic
dmarticus Dec 10, 2024
464cca8
don't use this
dmarticus Dec 10, 2024
6ef7d9d
naur
dmarticus Dec 10, 2024
f2c6f5e
linting and more self-reviews
dmarticus Dec 10, 2024
e835697
now tests are failing
dmarticus Dec 10, 2024
ad5bad6
okay
dmarticus Dec 10, 2024
0bb7a5c
Merge commit 'ad5bad621a' into feat/support-groups-and-payloads
dmarticus Dec 10, 2024
66ab412
still need to do this
dmarticus Dec 10, 2024
15df5b2
Merge branch 'master' into feat/support-groups-and-payloads
dmarticus Dec 11, 2024
90bfab2
Merge branch 'master' into feat/support-groups-and-payloads
dmarticus Dec 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion rust/feature-flags/src/api/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ pub async fn flags(
let context = RequestContext {
state,
ip,
meta: meta.0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never use this value in the request logic, so I can omit it.

headers,
body,
};
Expand Down
114 changes: 110 additions & 4 deletions rust/feature-flags/src/api/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ pub struct FlagsQueryParams {
pub struct RequestContext {
pub state: State<router::State>,
pub ip: IpAddr,
pub meta: FlagsQueryParams,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above comment.

pub headers: HeaderMap,
pub body: Bytes,
}
Expand All @@ -82,11 +81,24 @@ pub struct FeatureFlagEvaluationContext {
hash_key_override: Option<String>,
}

/// Process a feature flag request and return the evaluated flags
///
/// ## Flow
/// 1. Decodes and validates the request
/// 2. Extracts and verifies the authentication token
/// 3. Retrieves team information
/// 4. Processes person and group properties
/// 5. Retrieves feature flags
/// 6. Evaluates flags based on the context
///
/// ## Error Handling
/// - Returns early if any step fails
/// - Maintains error context through the FlagError enum
/// - Individual flag evaluation failures don't fail the entire request
pub async fn process_request(context: RequestContext) -> Result<FlagsResponse, FlagError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needed a comment since it's an entrypoint method

let RequestContext {
state,
ip,
meta: _, // TODO use this
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol I don't actually use this anywhere outside of recording request metadata, so bye bye meta!

headers,
body,
} = context;
Expand All @@ -95,20 +107,24 @@ pub async fn process_request(context: RequestContext) -> Result<FlagsResponse, F
let token = request
.extract_and_verify_token(state.redis.clone(), state.reader.clone())
.await?;

let team = request
.get_team_from_cache_or_pg(&token, state.redis.clone(), state.reader.clone())
.await?;

let distinct_id = request.extract_distinct_id()?;
let groups = request.groups.clone();
let team_id = team.id;
let person_property_overrides = get_person_property_overrides(
!request.geoip_disable.unwrap_or(false),
request.person_properties.clone(),
&ip,
&state.geoip,
);
let group_property_overrides = request.group_properties.clone();

let groups = request.groups.clone();
let group_property_overrides =
process_group_property_overrides(groups.clone(), request.group_properties.clone());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note, this means every time we have a flags request with groups, there will be a group_property_override that looks like (e.g.)

 Some({"project": {"$group_key": String("test_company_id")}})


let hash_key_override = request.anon_distinct_id.clone();

let feature_flags_from_cache_or_pg = request
Expand Down Expand Up @@ -170,6 +186,39 @@ pub fn get_person_property_overrides(
}
}

/// Processes group property overrides by combining existing overrides with group key overrides
///
/// When groups are provided in the format {"group_type": "group_key"}, we need to ensure these
/// are included in the group property overrides with the special "$group_key" property.
fn process_group_property_overrides(
groups: Option<HashMap<String, Value>>,
existing_overrides: Option<HashMap<String, HashMap<String, Value>>>,
) -> Option<HashMap<String, HashMap<String, Value>>> {
match groups {
Some(groups) => {
let group_key_overrides: HashMap<String, HashMap<String, Value>> = groups
.into_iter()
.map(|(group_type, group_key)| {
let mut properties = existing_overrides
.as_ref()
.and_then(|g| g.get(&group_type))
.cloned()
.unwrap_or_default();

properties.insert("$group_key".to_string(), group_key);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


(group_type, properties)
})
.collect();

let mut result = existing_overrides.unwrap_or_default();
result.extend(group_key_overrides);
Some(result)
}
None => existing_overrides,
}
}

/// Decode a request into a `FlagRequest`
/// - Currently only supports JSON requests
// TODO support all supported content types
Expand Down Expand Up @@ -738,4 +787,61 @@ mod tests {
assert!(!result.error_while_computing_flags);
assert_eq!(result.feature_flags["test_flag"], FlagValue::Boolean(true));
}

#[test]
fn test_process_group_property_overrides() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wanted to make sure I'm correctly overriding the group values with any passed in property overrides.

// Test case 1: Both groups and existing overrides
let groups = HashMap::from([
("project".to_string(), json!("project_123")),
("organization".to_string(), json!("org_456")),
]);

let mut existing_overrides = HashMap::new();
let mut project_props = HashMap::new();
project_props.insert("industry".to_string(), json!("tech"));
existing_overrides.insert("project".to_string(), project_props);

let result =
process_group_property_overrides(Some(groups.clone()), Some(existing_overrides));

assert!(result.is_some());
let result = result.unwrap();

// Check project properties
let project_props = result.get("project").expect("Project properties missing");
assert_eq!(project_props.get("industry"), Some(&json!("tech")));
assert_eq!(project_props.get("$group_key"), Some(&json!("project_123")));

// Check organization properties
let org_props = result
.get("organization")
.expect("Organization properties missing");
assert_eq!(org_props.get("$group_key"), Some(&json!("org_456")));

// Test case 2: Only groups, no existing overrides
let result = process_group_property_overrides(Some(groups.clone()), None);

assert!(result.is_some());
let result = result.unwrap();
assert_eq!(result.len(), 2);
assert_eq!(
result.get("project").unwrap().get("$group_key"),
Some(&json!("project_123"))
);

// Test case 3: No groups, only existing overrides
let mut existing_overrides = HashMap::new();
let mut project_props = HashMap::new();
project_props.insert("industry".to_string(), json!("tech"));
existing_overrides.insert("project".to_string(), project_props);

let result = process_group_property_overrides(None, Some(existing_overrides.clone()));

assert!(result.is_some());
assert_eq!(result.unwrap(), existing_overrides);

// Test case 4: Neither groups nor existing overrides
let result = process_group_property_overrides(None, None);
assert!(result.is_none());
}
}
2 changes: 2 additions & 0 deletions rust/feature-flags/src/api/types.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use serde::{Deserialize, Serialize};
use serde_json::Value;
use std::collections::HashMap;

#[derive(Debug, PartialEq, Eq, Deserialize, Serialize)]
Expand All @@ -18,4 +19,5 @@ pub enum FlagValue {
pub struct FlagsResponse {
pub error_while_computing_flags: bool,
pub feature_flags: HashMap<String, FlagValue>,
pub feature_flag_payloads: HashMap<String, Value>, // flag key -> payload
}
2 changes: 0 additions & 2 deletions rust/feature-flags/src/cohort/cohort_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,6 @@ mod tests {
.find(|c| c.id == main_cohort.id)
.expect("Failed to find main cohort");

println!("fetched_main_cohort: {:?}", fetched_main_cohort);

let dependencies = fetched_main_cohort.extract_dependencies().unwrap();
let expected_dependencies: HashSet<CohortId> =
[dependent_cohort.id].iter().cloned().collect();
Expand Down
Loading