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

add enrollment status metric for cirrus #5891

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 16 additions & 2 deletions components/nimbus/src/cirrus.udl
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,26 @@ enum NimbusError {
"UuidError", "InvalidExperimentFormat",
"InvalidPath", "InternalError", "NoSuchExperiment", "NoSuchBranch",
"DatabaseNotReady", "VersionParsingError", "TryFromIntError",
"ParseIntError", "TransformParameterError", "CirrusError",
"ParseIntError", "TransformParameterError", "CirrusError", "UniFFICallbackError"
};

callback interface MetricsHandler {
void record_enrollment_statuses(sequence<EnrollmentStatusExtraDef> enrollment_status_extras);
};

dictionary EnrollmentStatusExtraDef {
string? branch;
string? conflict_slug;
string? error_string;
string? reason;
string? slug;
string? status;
string? user_id;
};

interface CirrusClient {
[Throws=NimbusError]
constructor(string app_context, optional sequence<string> coenrolling_feature_ids = []);
constructor(string app_context, MetricsHandler metrics_handler, optional sequence<string> coenrolling_feature_ids = []);

// Handles an enrollment request string and returns an enrollment response string.
[Throws=NimbusError]
Expand Down
1 change: 0 additions & 1 deletion components/nimbus/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ pub enum NimbusError {
#[cfg(not(feature = "stateful"))]
#[error("Error in Cirrus: {0}")]
CirrusError(#[from] CirrusClientError),
#[cfg(feature = "stateful")]
#[error("UniFFI callback error: {0}")]
UniFFICallbackError(#[from] uniffi::UnexpectedUniFFICallbackError),
}
Expand Down
2 changes: 1 addition & 1 deletion components/nimbus/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ mod strings;
mod targeting;

pub mod error;
pub mod metrics;
pub mod schema;
pub mod versioning;

Expand All @@ -25,7 +26,6 @@ pub use targeting::NimbusTargetingHelper;

cfg_if::cfg_if! {
if #[cfg(feature = "stateful")] {
pub mod metrics;

pub mod stateful;

Expand Down
9 changes: 9 additions & 0 deletions components/nimbus/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ pub struct EnrollmentStatusExtraDef {
pub reason: Option<String>,
pub slug: Option<String>,
pub status: Option<String>,
#[cfg(not(feature = "stateful"))]
pub user_id: Option<String>,
}

#[cfg(test)]
Expand Down Expand Up @@ -43,6 +45,11 @@ impl EnrollmentStatusExtraDef {
pub fn status(&self) -> &str {
self.status.as_ref().unwrap()
}

#[cfg(not(feature = "stateful"))]
pub fn user_id(&self) -> &str {
self.user_id.as_ref().unwrap()
}
brennie marked this conversation as resolved.
Show resolved Hide resolved
}

impl From<ExperimentEnrollment> for EnrollmentStatusExtraDef {
Expand Down Expand Up @@ -74,6 +81,8 @@ impl From<ExperimentEnrollment> for EnrollmentStatusExtraDef {
reason: reason_value,
slug: Some(enrollment.slug),
status: Some(enrollment.status.name()),
#[cfg(not(feature = "stateful"))]
user_id: None,
}
}
}
4 changes: 2 additions & 2 deletions components/nimbus/src/stateful/nimbus_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ impl NimbusClient {
db_path: P,
config: Option<RemoteSettingsConfig>,
available_randomization_units: AvailableRandomizationUnits,
host_metrics: Box<dyn MetricsHandler>,
metrics_handler: Box<dyn MetricsHandler>,
jeddai marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<Self> {
let settings_client = Mutex::new(create_client(config)?);

Expand All @@ -99,7 +99,7 @@ impl NimbusClient {
coenrolling_feature_ids,
db: OnceCell::default(),
event_store: Arc::default(),
metrics_handler: Arc::new(host_metrics),
metrics_handler: Arc::new(metrics_handler),
})
}

Expand Down
24 changes: 21 additions & 3 deletions components/nimbus/src/stateless/cirrus_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ use crate::{
EnrollmentsEvolver, ExperimentEnrollment,
},
error::CirrusClientError,
metrics::{EnrollmentStatusExtraDef, MetricsHandler},
parse_experiments, AppContext, AvailableRandomizationUnits, Experiment, NimbusError,
NimbusTargetingHelper, Result, TargetingAttributes,
};
use serde_derive::*;
use serde_json::{Map, Value};
use std::collections::HashMap;
use std::fmt;
use std::sync::Mutex;
use std::sync::{Arc, Mutex};

/// EnrollmentResponse is a DTO for the response from handling enrollment for a given client.
///
Expand Down Expand Up @@ -76,20 +77,25 @@ pub struct CirrusMutableState {
experiments: Vec<Experiment>,
}

#[derive(Default)]
pub struct CirrusClient {
app_context: AppContext,
coenrolling_feature_ids: Vec<String>,
state: Mutex<CirrusMutableState>,
metrics_handler: Arc<Box<dyn MetricsHandler>>,
jeddai marked this conversation as resolved.
Show resolved Hide resolved
}

impl CirrusClient {
pub fn new(app_context: String, coenrolling_feature_ids: Vec<String>) -> Result<Self> {
pub fn new(
app_context: String,
metrics_handler: Box<dyn MetricsHandler>,
coenrolling_feature_ids: Vec<String>,
) -> Result<Self> {
let app_context: AppContext = serde_json::from_str(&app_context)?;
Ok(Self {
app_context,
coenrolling_feature_ids,
state: Default::default(),
metrics_handler: Arc::new(metrics_handler),
})
}

Expand Down Expand Up @@ -151,6 +157,18 @@ impl CirrusClient {
prev_enrollments,
)?;

self.metrics_handler.record_enrollment_statuses(
enrollments
.iter()
.cloned()
.map(|e| {
let mut extra: EnrollmentStatusExtraDef = e.into();
extra.user_id = Some(user_id.clone());
extra
})
.collect(),
);

let enrolled_feature_config_map =
map_features_by_feature_id(&enrollments, &state.experiments, &coenrolling_ids);

Expand Down
56 changes: 51 additions & 5 deletions components/nimbus/src/tests/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,20 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/. */

use crate::enrollment::{
EnrolledFeatureConfig, EnrolledReason, ExperimentEnrollment, NotEnrolledReason,
};
use crate::{
enrollment::{EnrolledFeatureConfig, EnrolledReason, ExperimentEnrollment, NotEnrolledReason},
metrics::{EnrollmentStatusExtraDef, MetricsHandler},
AppContext, EnrollmentStatus, Experiment, FeatureConfig, NimbusTargetingHelper,
TargetingAttributes,
};
use serde::Serialize;
use serde_json::{json, Value};
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};
use std::sync::{Arc, Mutex};

cfg_if::cfg_if! {
if #[cfg(feature = "stateful")] {
use crate::stateful::behavior::EventStore;
use std::sync::{Arc, Mutex};
}
}

Expand Down Expand Up @@ -48,6 +47,53 @@ impl Default for NimbusTargetingHelper {
}
}

/// A Rust implementation of the MetricsHandler trait
/// Used to test recording of Glean metrics across the FFI within Rust
///
/// *NOTE: Use this struct's `new` method when instantiating it to lock the Glean store*
#[derive(Clone)]
pub struct TestMetrics {
state: Arc<Mutex<HashMap<String, serde_json::Value>>>,
}

impl TestMetrics {
pub fn new() -> Self {
TestMetrics {
state: Default::default(),
}
}

pub fn assert_get_vec_value(&self, key: &str) -> serde_json::Value {
self.state.lock().unwrap().get(key).unwrap().clone()
}
}

impl MetricsHandler for TestMetrics {
/// In actual implementations of the MetricsHandler trait, this method would record the
/// supplied EnrollmentStatusExtraDefs into Glean.
///
/// This implementation is explicitly used for testing, and does the following:
/// 1. It locks the TestMetrics instance's state
/// 2. It looks up the key for `enrollment_status` in the state, extends it if it already
/// exists and inserts it if it does not exist.
///
/// This then allows for us to use the `assert_get_vec_value` method above in tests to fetch the
/// list of metrics that have been recorded during a given test.
fn record_enrollment_statuses(&self, enrollment_status_extras: Vec<EnrollmentStatusExtraDef>) {
jeddai marked this conversation as resolved.
Show resolved Hide resolved
let key = "enrollment_status".to_string();
let mut state = self.state.lock().unwrap();
let new = serde_json::to_value(enrollment_status_extras).unwrap();
state
.entry(key)
.and_modify(|v| {
v.as_array_mut()
.unwrap()
.extend(new.as_array().unwrap().iter().cloned());
})
.or_insert(new);
}
}

pub(crate) fn get_test_experiments() -> Vec<Experiment> {
vec![
serde_json::from_value(json!({
Expand Down
1 change: 0 additions & 1 deletion components/nimbus/src/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ mod test_versioning;

#[cfg(feature = "stateful")]
mod stateful {
mod helpers;
mod test_behavior;
mod test_enrollment;
mod test_evaluator;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#![allow(unused_imports)]

use crate::error::Result;
use crate::tests::stateful::helpers::TestMetrics;
use crate::tests::helpers::TestMetrics;

#[cfg(feature = "rkv-safe-mode")]
#[test]
Expand Down
50 changes: 0 additions & 50 deletions components/nimbus/src/tests/stateful/helpers.rs

This file was deleted.

9 changes: 3 additions & 6 deletions components/nimbus/src/tests/stateful/test_nimbus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,9 @@ use crate::{
},
persistence::{Database, StoreId},
},
tests::{
helpers::{
get_bucketed_rollout, get_ios_rollout_experiment, get_targeted_experiment,
to_local_experiments_string,
},
stateful::helpers::TestMetrics,
tests::helpers::{
get_bucketed_rollout, get_ios_rollout_experiment, get_targeted_experiment,
to_local_experiments_string, TestMetrics,
},
AppContext, AvailableRandomizationUnits, Experiment, NimbusClient, TargetingAttributes,
DB_KEY_APP_VERSION, DB_KEY_UPDATE_DATE,
Expand Down
Loading