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

FF-2678 flag evaluation details #13

Merged
merged 37 commits into from
Aug 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
deaeefc
refactor: split off FlagEvaluationError type
rasendubi Jul 17, 2024
de274be
refactor: add flag evaluation visitor (noop for now)
rasendubi Jul 17, 2024
6ea91f5
refactor: add allocation eval visitor (noop)
rasendubi Jul 17, 2024
07ba6a3
feat: add simple evaluation details
rasendubi Jul 17, 2024
59c13d7
refactor: add FlagEvaluationError.is_normal()
rasendubi Jul 17, 2024
eecc1c0
fix(msrv): use super scope for visitors
rasendubi Jul 17, 2024
52fc976
refactor: remove Sharder trait
rasendubi Jul 18, 2024
023d261
feat: add timestamp to evaluation details
rasendubi Jul 18, 2024
85d2253
feat: parse created_at and environment field in UFC response
rasendubi Jul 18, 2024
b81d2b5
feat: store fetched_at in Configuration
rasendubi Jul 18, 2024
77c2919
feat: add configuration details to eval EvalFlagDetails
rasendubi Jul 18, 2024
b5df8b9
feat: add variation_key and variation_value to EvalFlagDetails
rasendubi Jul 18, 2024
cd05630
feat: rule-level evaluation details
rasendubi Jul 18, 2024
ba779b9
feat: split result into result and error for prettier json
rasendubi Jul 18, 2024
67ff954
refactor: serialize AssignmentValue with type/value fields
rasendubi Jul 18, 2024
f884b0a
feat: integrate assignment details into rust sdk
rasendubi Jul 18, 2024
8d8a254
feat: add shard-level evaluation details
rasendubi Jul 18, 2024
871bd4a
feat: re-export details types from Rust SDK
rasendubi Jul 18, 2024
006ba76
refactor: move details builder into its own file
rasendubi Jul 18, 2024
28f9823
doc: a bit more docs
rasendubi Jul 18, 2024
d12785f
feat: skip serializing variation key/value if not present
rasendubi Jul 18, 2024
58d72ad
bench: add benchmark for evaluation details
rasendubi Jul 19, 2024
28f8da3
fix: add eppo_core override in main workspace
rasendubi Jul 19, 2024
a9adac1
perf: don't clone assignment event in details builder
rasendubi Jul 19, 2024
88c0fc5
feat: add evaluation_details to AssignmentEvent
rasendubi Jul 19, 2024
1240cb6
feat: bring EvaluationDetails type closer to JS implementation
rasendubi Jul 22, 2024
78a0670
feat: add flag_evaluation_description
rasendubi Jul 23, 2024
2cb7a4c
test: test evaluation details against sdk-test-data
rasendubi Jul 29, 2024
932d753
chore(ruby): add --verbose to Cargo.lock check
rasendubi Jul 29, 2024
23ebf95
chore: override eppo_core in root workspace, not config
rasendubi Jul 29, 2024
e241d6d
chore: check MSRV with local eppo_core
rasendubi Jul 29, 2024
aa35e5b
refactor: move all events into a separate file
rasendubi Aug 1, 2024
6aefe39
refactor: move all evaluation into its own module
rasendubi Aug 1, 2024
00f161d
refactor: inline get_assignment_inner
rasendubi Aug 1, 2024
b1a95fd
feat: add ConfigurationMissing to FlagEvaluationCode
rasendubi Aug 1, 2024
d02f430
feat(eppo_core): evaluation details for bandits
rasendubi Aug 2, 2024
393482e
feat(ruby-sdk): expose evaluation details
rasendubi Aug 2, 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
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ jobs:

- name: Check Cargo.lock
# Ensure that Cargo.lock matches Cargo.toml
run: cargo update --workspace --locked
run: cargo update --workspace --locked --verbose
working-directory: ruby-sdk

- name: Override eppo_core for testing
Expand Down
7 changes: 7 additions & 0 deletions .github/workflows/msrv.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ jobs:
then
cargo install cargo-msrv --locked
fi

- name: Override eppo_core for testing against local version
run: |
mkdir -p ~/.cargo/
echo "[patch.crates-io.eppo_core]" >> "${CARGO_HOME:-$HOME/.cargo}/config.toml"
echo "path = '$PWD/eppo_core'" >> "${CARGO_HOME:-$HOME/.cargo}/config.toml"

- name: Verify MSRV for each package
run: |
for dir in eppo_core rust-sdk ruby-sdk/ext/eppo_client; do
Expand Down
5 changes: 5 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,9 @@ resolver = "2"
members = [
"eppo_core",
"rust-sdk",
"ruby-sdk/ext/eppo_client",
]

[patch.crates-io]
# Local override for development.
eppo_core = { path = './eppo_core' }
5 changes: 5 additions & 0 deletions eppo_core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,9 @@ thiserror = "1.0.60"
url = "2.5.0"

[dev-dependencies]
criterion = { version = "0.4", features = ["html_reports"] }
env_logger = "0.11.3"

[[bench]]
name = "evaluation_details"
harness = false
137 changes: 137 additions & 0 deletions eppo_core/benches/evaluation_details.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
use std::collections::HashMap;
use std::fs::File;

use criterion::{black_box, criterion_group, criterion_main, Criterion, Throughput};

use eppo_core::{
ufc::{get_assignment, get_assignment_details},
Configuration,
};

fn criterion_benchmark(c: &mut Criterion) {
let flags =
serde_json::from_reader(File::open("../sdk-test-data/ufc/flags-v1.json").unwrap()).unwrap();
let configuration = Configuration::from_server_response(flags, None);

{
let mut group = c.benchmark_group("new-user-onboarding");
group.throughput(Throughput::Elements(1));
let attributes = HashMap::new();
group.bench_function("get_assignment", |b| {
b.iter(|| {
get_assignment(
black_box(Some(&configuration)),
black_box("new-user-onboarding"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.bench_function("get_assignment_details", |b| {
b.iter(|| {
get_assignment_details(
black_box(Some(&configuration)),
black_box("new-user-onboarding"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.finish();
}

{
let mut group = c.benchmark_group("rollout");
group.throughput(Throughput::Elements(1));
let attributes = [("country".to_owned(), "US".into())].into();
group.bench_function("get_assignment", |b| {
b.iter(|| {
get_assignment(
black_box(Some(&configuration)),
black_box("new-user-onboarding"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.bench_function("get_assignment_details", |b| {
b.iter(|| {
get_assignment_details(
black_box(Some(&configuration)),
black_box("new-user-onboarding"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.finish();
}

{
let mut group = c.benchmark_group("json-config-flag");
group.throughput(Throughput::Elements(1));
let attributes = [].into();
group.bench_function("get_assignment", |b| {
b.iter(|| {
get_assignment(
black_box(Some(&configuration)),
black_box("json-config-flag"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.bench_function("get_assignment_details", |b| {
b.iter(|| {
get_assignment_details(
black_box(Some(&configuration)),
black_box("json-config-flag"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.finish();
}

{
let mut group = c.benchmark_group("numeric-one-of");
group.throughput(Throughput::Elements(1));
let attributes = [("number".to_owned(), 2.0.into())].into();
group.bench_function("get_assignment", |b| {
b.iter(|| {
get_assignment(
black_box(Some(&configuration)),
black_box("numeric-one-of"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.bench_function("get_assignment_details", |b| {
b.iter(|| {
get_assignment_details(
black_box(Some(&configuration)),
black_box("numeric-one-of"),
black_box("subject1"),
black_box(&attributes),
black_box(None),
)
})
});
group.finish();
}
}

criterion_group!(
name = benches;
config = Criterion::default().noise_threshold(0.02);
targets = criterion_benchmark);
criterion_main!(benches);
File renamed without changes.
22 changes: 0 additions & 22 deletions eppo_core/src/bandits/event.rs

This file was deleted.

7 changes: 0 additions & 7 deletions eppo_core/src/bandits/mod.rs

This file was deleted.

34 changes: 19 additions & 15 deletions eppo_core/src/configuration.rs
Original file line number Diff line number Diff line change
@@ -1,42 +1,46 @@
use std::collections::HashMap;

use chrono::{DateTime, Utc};

use crate::{
bandits::{BanditConfiguration, BanditResponse},
ufc::{BanditVariation, TryParse, UniversalFlagConfig},
};

/// Remote configuration for the eppo client. It's a central piece that defines client behavior.
#[derive(Default, Clone)]
#[derive(Clone)]
pub struct Configuration {
/// Timestamp when configuration was fetched by the SDK.
pub fetched_at: DateTime<Utc>,
/// Flags configuration.
pub flags: Option<UniversalFlagConfig>,
pub flags: UniversalFlagConfig,
/// Bandits configuration.
pub bandits: Option<BanditResponse>,
/// Mapping from flag key to flag variation value to bandit variation.
/// Mapping from flag key to flag variation value to bandit variation. Cached from
/// `self.flags.bandits`.
pub flag_to_bandit_associations:
HashMap</* flag_key: */ String, HashMap</* variation_key: */ String, BanditVariation>>,
}

impl Configuration {
/// Create a new configuration from server responses.
pub fn new(
config: Option<UniversalFlagConfig>,
pub fn from_server_response(
config: UniversalFlagConfig,
bandits: Option<BanditResponse>,
) -> Configuration {
if let Some(config) = &config {
// warn if some flags failed to parse
for (name, flag) in &config.flags {
if let TryParse::ParseFailed(_value) = flag {
log::warn!(target: "eppo", "failed to parse flag configuration: {name:?}");
}
let now = Utc::now();

// warn if some flags failed to parse
for (name, flag) in &config.flags {
if let TryParse::ParseFailed(_value) = flag {
log::warn!(target: "eppo", "failed to parse flag configuration: {name:?}");
}
}

let flag_to_bandit_associations = config
.as_ref()
.map(get_flag_to_bandit_associations)
.unwrap_or_default();
let flag_to_bandit_associations = get_flag_to_bandit_associations(&config);

Configuration {
fetched_at: now,
flags: config,
bandits,
flag_to_bandit_associations,
Expand Down
4 changes: 1 addition & 3 deletions eppo_core/src/configuration_fetcher.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
//! An HTTP client that fetches configuration from the server.
use std::sync::Arc;

use reqwest::{StatusCode, Url};

use crate::{bandits::BanditResponse, ufc::UniversalFlagConfig, Configuration, Error, Result};
Expand Down Expand Up @@ -55,7 +53,7 @@ impl ConfigurationFetcher {
Some(self.fetch_bandits_configuration()?)
};

Ok(Configuration::new(Some(ufc), bandits))
Ok(Configuration::from_server_response(ufc, bandits))
}

fn fetch_ufc_configuration(&mut self) -> Result<UniversalFlagConfig> {
Expand Down
32 changes: 23 additions & 9 deletions eppo_core/src/configuration_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,21 @@ use crate::Configuration;
/// `ConfigurationStore` provides a thread-safe (`Sync`) storage for Eppo configuration that allows
/// concurrent access for readers and writers.
///
/// `Configuration` itself is always immutable and can only be replaced fully.
/// `Configuration` itself is always immutable and can only be replaced completely.
#[derive(Default)]
pub struct ConfigurationStore {
configuration: RwLock<Arc<Configuration>>,
configuration: RwLock<Option<Arc<Configuration>>>,
}

impl ConfigurationStore {
/// Create a new empty configuration store.
pub fn new() -> Self {
ConfigurationStore::default()
}

pub fn get_configuration(&self) -> Arc<Configuration> {
/// Get currently-active configuration. Returns None if configuration hasn't been fetched/stored
/// yet.
pub fn get_configuration(&self) -> Option<Arc<Configuration>> {
// self.configuration.read() should always return Ok(). Err() is possible only if the lock
// is poisoned (writer panicked while holding the lock), which should never happen.
let configuration = self
Expand All @@ -38,35 +41,46 @@ impl ConfigurationStore {
.write()
.expect("thread holding configuration lock should not panic");

*configuration_slot = config;
*configuration_slot = Some(config);
}
}

#[cfg(test)]
mod tests {
use std::{collections::HashMap, sync::Arc};

use chrono::Utc;

use super::ConfigurationStore;
use crate::{ufc::UniversalFlagConfig, Configuration};
use crate::{
ufc::{Environment, UniversalFlagConfig},
Configuration,
};

#[test]
fn can_set_configuration_from_another_thread() {
let store = Arc::new(ConfigurationStore::new());

assert!(store.get_configuration().is_none());

{
let store = store.clone();
let _ = std::thread::spawn(move || {
store.set_configuration(Configuration::new(
Some(UniversalFlagConfig {
store.set_configuration(Configuration::from_server_response(
UniversalFlagConfig {
created_at: Utc::now(),
environment: Environment {
name: "test".to_owned(),
},
flags: HashMap::new(),
bandits: HashMap::new(),
}),
},
None,
))
})
.join();
}

assert!(store.get_configuration().flags.is_some());
assert!(store.get_configuration().is_some());
}
}
Loading
Loading