chore(flags): Make dependency graph function generic #33567
Conversation
There was a problem hiding this comment.
PR Summary
Refactors dependency graph handling in Rust code to introduce a generic DependencyProvider trait, enabling reuse across feature flags and cohorts.
- Adds new
graph_utils.rswith genericDependencyProvidertrait and graph building utilities for improved code reuse - Implements
DependencyProviderfor bothCohortandFeatureFlagtypes to standardize dependency extraction - Replaces cohort-specific
build_cohort_dependency_graphwith genericbuild_dependency_graphimplementation - Adds support for handling feature flag dependencies in property filters with proper cycle detection
4 files reviewed, 3 comments
Edit PR Review Bot Settings | Greptile
And make `DependencyNotFound` take an i64.
f003eee to
f96fd6e
Compare
There was a problem hiding this comment.
PR Summary
This update refactors error handling and dependency types to support a more generic dependency system across the codebase.
- Introduces
DependencyTypeenum and replaces cohort-specific errors with genericDependencyNotFoundandDependencyCycleerrors inerrors.rs - Updates error handling in
flag_matching.rsto differentiate between deleted dependencies (warning) vs actual errors - Makes
FromFeatureAndMatchtrait more generic by addingcondition_indexparameter to track which dependency condition failed - Changes error code from 'cohort_not_found' to 'dependency_not_found_cohort' for consistency in
tests.rs
9 files reviewed, 4 comments
Edit PR Review Bot Settings | Greptile
| if let FlagError::DependencyNotFound(dependency_type, dependency_id) = &e { | ||
| warn!( | ||
| "Feature flag '{}' targeting deleted {} with id {} for distinct_id '{}': {:?}", | ||
| flag.key, dependency_type, dependency_id, self.distinct_id, e |
There was a problem hiding this comment.
make sure to also add parsing for the DependencyNotFound error type to our prometheus label exporter
pub fn parse_exception_for_prometheus_label(err: &FlagError) -> &'static str {
match err {
FlagError::DatabaseError(msg) => {
if msg.contains("statement timeout") {
"timeout"
} else if msg.contains("no more connections") {
"no_more_connections"
} else if msg.contains("Failed to fetch conditions") {
"flag_condition_retry"
} else if msg.contains("Failed to fetch group") {
"group_mapping_retry"
} else if msg.contains("Database healthcheck failed") {
"healthcheck_failed"
} else if msg.contains("query_wait_timeout") {
"query_wait_timeout"
} else {
"database_error"
}
}
FlagError::DatabaseUnavailable => "database_unavailable",
FlagError::RedisUnavailable => "redis_unavailable",
FlagError::TimeoutError => "timeout_error",
FlagError::NoGroupTypeMappings => "no_group_type_mappings",
FlagError::CohortNotFound(_) => "cohort_not_found",
FlagError::CohortFiltersParsingError => "cohort_filters_parsing_error",
_ => "unknown",
}
```
otherwise this will be an unknown error in our metrics (e.g. https://grafana.prod-us.posthog.dev/d/eehu20mrbkbuof/new-feature-flags?var-reason=$__all&var-team_id=$__all&var-timing_quartile=0.99&from=now-6h&to=now&timezone=utc), which is no bueno.
| FlagError::DependencyNotFound(dependency_type, _) => match dependency_type { | ||
| DependencyType::Cohort => "dependency_not_found_cohort", | ||
| DependencyType::Flag => "dependency_not_found_flag", | ||
| }, |
There was a problem hiding this comment.
@dmarticus, how do I add these labels to the prometheus label exporter?
There was a problem hiding this comment.
just add them as names in parse_exception_for_prometheus_label method, like this
FlagError::DatabaseUnavailable => "database_unavailable",
FlagError::RedisUnavailable => "redis_unavailable",
FlagError::TimeoutError => "timeout_error",
FlagError::NoGroupTypeMappings => "no_group_type_mappings",
FlagError::CohortNotFound(_) => "cohort_not_found",
FlagError::CohortFiltersParsingError => "cohort_filters_parsing_error",
+ FlagError::DependencyNotFound(dependency_type, _) => match dependency_type {
+ DependencyType::Cohort => "dependency_not_found_cohort",
+ DependencyType::Flag => "dependency_not_found_flag",
+ },
should work i think.
There was a problem hiding this comment.
As far as I can tell, that code is already there. Am I missing something?
There was a problem hiding this comment.
Did you mean for DependencyCycle? I don't see that error type represented in this function.
There was a problem hiding this comment.
Ok, I added some for DependencyCycle in case that's what you meant.
Important
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Problem
Related to #29016
This is a refactoring to make a generic trait for structures with dependencies such as Cohorts and Feature Flags.
Changes
No public facing changes. Code just gets better.
Did you write or update any docs for this change?
How did you test this code?
Unit tests