Skip to content

Move nimbus to new RS api #6662

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ import kotlinx.coroutines.Job
import kotlinx.coroutines.NonCancellable
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import mozilla.appservices.remotesettings.RemoteSettingsConfig
import mozilla.appservices.remotesettings.RemoteSettingsServer
import mozilla.appservices.remotesettings.RemoteSettingsService
import mozilla.telemetry.glean.Glean
import org.json.JSONObject
import org.mozilla.experiments.nimbus.GleanMetrics.NimbusEvents
Expand Down Expand Up @@ -68,11 +67,13 @@ open class Nimbus(
override val prefs: SharedPreferences? = null,
appInfo: NimbusAppInfo,
coenrollingFeatureIds: List<String>,
server: NimbusServerSettings?,
remoteSettingsService: RemoteSettingsService,
Copy link
Member

Choose a reason for hiding this comment

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

This will require an associated change in the constructor use here

override fun newNimbus(appInfo: NimbusAppInfo, serverSettings: NimbusServerSettings?) =
Nimbus(
context,
appInfo = appInfo,
prefs = sharedPreferences,
coenrollingFeatureIds = getCoenrollingFeatureIds(),
server = serverSettings,
deviceInfo = createDeviceInfo(),
delegate = createDelegate(),
observer = createObserver(),
recordedContext = recordedContext,
)

As well as in a number of tests

Copy link
Contributor

Choose a reason for hiding this comment

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

What's you're feeling on how hard it's going to be to get a RemoteSettingsService to pass to the constructor on the firefox-android side?

If I traced through the code correctly, I think it just means moving the RemoteSettingsService val above the Nimbus components val. That seems easy enough, but I'm slightly worried that there are special requirements for Nimbus startup that could cause problems.

Similarly, do you foresee any issues on focus or ios?

deviceInfo: NimbusDeviceInfo,
private val observer: NimbusInterface.Observer? = null,
delegate: NimbusDelegate,
private val recordedContext: RecordedContext? = null,
collectionName: String,
remoteSettingsService: RemoteSettingsService? = null,
) : NimbusInterface {
// An I/O scope is used for reading or writing from the Nimbus's RKV database.
private val dbScope: CoroutineScope = delegate.dbScope
Expand Down Expand Up @@ -160,20 +161,14 @@ open class Nimbus(
val experimentContext = buildExperimentContext(context, appInfo, deviceInfo)

// Initialize Nimbus
val remoteSettingsConfig = server?.let {
RemoteSettingsConfig(
server = RemoteSettingsServer.Custom(it.url.toString()),
collectionName = it.collection,
)
}

nimbusClient = NimbusClient(
experimentContext,
recordedContext,
coenrollingFeatureIds,
dataDir.path,
remoteSettingsConfig,
metricsHandler,
remoteSettingsService,
collectionName,
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import androidx.annotation.RawRes
import kotlinx.coroutines.runBlocking
import org.mozilla.experiments.nimbus.internal.FeatureManifestInterface
import org.mozilla.experiments.nimbus.internal.RecordedContext
import mozilla.appservices.remotesettings.RemoteSettingsService

private const val TIME_OUT_LOADING_EXPERIMENT_FROM_DISK_MS = 200L

Expand Down Expand Up @@ -122,7 +123,7 @@ abstract class AbstractNimbusBuilder<T : NimbusInterface>(val context: Context)

@Suppress("TooGenericExceptionCaught")
return try {
newNimbus(appInfo, serverSettings).apply {
newNimbus(appInfo, serverSettings, remoteSettingsService, collectionName).apply {
// Apply any experiment recipes we downloaded last time, or
// if this is the first time, we load the ones bundled in the res/raw
// directory.
Expand Down Expand Up @@ -164,6 +165,8 @@ abstract class AbstractNimbusBuilder<T : NimbusInterface>(val context: Context)
protected abstract fun newNimbus(
appInfo: NimbusAppInfo,
serverSettings: NimbusServerSettings?,
remoteSettingsService: RemoteSettingsService?

): T

/**
Expand Down Expand Up @@ -218,7 +221,7 @@ private class Observer(
}

class DefaultNimbusBuilder(context: Context) : AbstractNimbusBuilder<NimbusInterface>(context) {
override fun newNimbus(appInfo: NimbusAppInfo, serverSettings: NimbusServerSettings?) =
override fun newNimbus(appInfo: NimbusAppInfo, serverSettings: NimbusServerSettings?, remoteSettingsService: remoteSettingsService, collectionName: String) =
Nimbus(
context,
appInfo = appInfo,
Expand All @@ -229,6 +232,8 @@ class DefaultNimbusBuilder(context: Context) : AbstractNimbusBuilder<NimbusInter
delegate = createDelegate(),
observer = createObserver(),
recordedContext = recordedContext,
collectionName = serverSettings?.collection,
remoteSettingsService = remoteSettingsService,
)

override fun newNimbusDisabled() = NullNimbus(context)
Expand Down
19 changes: 10 additions & 9 deletions components/nimbus/examples/experiment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,11 @@ fn main() -> Result<()> {
MetricsHandler,
},
AppContext, AvailableRandomizationUnits, EnrollmentStatus, NimbusClient,
NimbusTargetingHelper, RemoteSettingsConfig, RemoteSettingsServer,
NimbusTargetingHelper,
};
use std::collections::HashMap;
use remote_settings::{RemoteSettingsConfig2, RemoteSettingsService};
use std::io::prelude::*;
use std::{collections::HashMap, sync::Arc};

pub struct NoopMetricsHandler;

Expand Down Expand Up @@ -217,23 +218,23 @@ fn main() -> Result<()> {
log::info!("Database directory is {}", db_path);

// initiate the optional config
let config = RemoteSettingsConfig {
server: Some(RemoteSettingsServer::Custom {
url: server_url.to_string(),
}),
server_url: None,
let config = RemoteSettingsConfig2 {
server: None,
bucket_name: None,
collection_name: collection_name.to_string(),
app_context: None,
};

let remote_settings_services = RemoteSettingsService::new("nimbus".to_owned(), config)?;

// Here we initialize our main `NimbusClient` struct
let nimbus_client = NimbusClient::new(
context.clone(),
Default::default(),
Default::default(),
db_path,
Some(config),
Box::new(NoopMetricsHandler),
Some(collection_name.to_string()),
Some(Arc::new(remote_settings_services)),
)?;
log::info!("Nimbus ID is {}", nimbus_client.nimbus_id()?);

Expand Down
27 changes: 18 additions & 9 deletions components/nimbus/ios/Nimbus/NimbusBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import Foundation
import class MozillaAppServices.RemoteSettingsService

/**
* A builder for [Nimbus] singleton objects, parameterized in a declarative class.
Expand Down Expand Up @@ -259,15 +260,23 @@ public class NimbusBuilder {
featureManifest?.getCoenrollingFeatureIds() ?? []
}

func newNimbus(_ appInfo: NimbusAppSettings, serverSettings: NimbusServerSettings?) throws -> NimbusInterface {
try Nimbus.create(serverSettings,
appSettings: appInfo,
coenrollingFeatureIds: getCoenrollingFeatureIds(),
dbPath: dbFilePath,
resourceBundles: resourceBundles,
userDefaults: userDefaults,
errorReporter: errorReporter,
recordedContext: recordedContext)
func newNimbus(
_ appInfo: NimbusAppSettings,
serverSettings: NimbusServerSettings?,
remoteSettingsService: RemoteSettingsService?
) throws -> NimbusInterface {
try Nimbus.create(
serverSettings,
appSettings: appInfo,
coenrollingFeatureIds: getCoenrollingFeatureIds(),
dbPath: dbFilePath,
resourceBundles: resourceBundles,
userDefaults: userDefaults,
errorReporter: errorReporter,
recordedContext: recordedContext,
collectionName: serverSettings?.collection,
remoteSettingsService: RemoteSettingsService,
)
}

func newNimbusDisabled() -> NimbusInterface {
Expand Down
17 changes: 7 additions & 10 deletions components/nimbus/ios/Nimbus/NimbusCreate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -77,34 +77,31 @@ public extension Nimbus {
/// - Throws `NimbusError` if anything goes wrong with the Rust FFI or in the `NimbusClient` constructor.
///
static func create(
_ server: NimbusServerSettings?,
appSettings: NimbusAppSettings,
coenrollingFeatureIds: [String] = [],
dbPath: String,
resourceBundles: [Bundle] = [Bundle.main],
enabled: Bool = true,
userDefaults: UserDefaults? = nil,
errorReporter: @escaping NimbusErrorReporter = defaultErrorReporter,
recordedContext: RecordedContext? = nil
recordedContext: RecordedContext? = nil,
remoteSettingsService: RemoteSettingsService? = nil,
collectionName: String? = nil
Copy link
Member

Choose a reason for hiding this comment

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

This will require an associated change here

func newNimbus(_ appInfo: NimbusAppSettings, serverSettings: NimbusServerSettings?) throws -> NimbusInterface {
try Nimbus.create(serverSettings,
appSettings: appInfo,
coenrollingFeatureIds: getCoenrollingFeatureIds(),
dbPath: dbFilePath,
resourceBundles: resourceBundles,
userDefaults: userDefaults,
errorReporter: errorReporter,
recordedContext: recordedContext)
}

) throws -> NimbusInterface {
guard enabled else {
return NimbusDisabled.shared
}

let context = Nimbus.buildExperimentContext(appSettings)
let remoteSettings = server.map { server -> RemoteSettingsConfig in
RemoteSettingsConfig(
collectionName: server.collection,
server: .custom(url: server.url.absoluteString)
)
}

let nimbusClient = try NimbusClient(
appCtx: context,
recordedContext: recordedContext,
coenrollingFeatureIds: coenrollingFeatureIds,
dbpath: dbPath,
remoteSettingsConfig: remoteSettings,
metricsHandler: GleanMetricsHandler()
metricsHandler: GleanMetricsHandler(),
remoteSettingsService: remoteSettingsService,
collectionName: collectionName
)

return Nimbus(
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 @@ -20,6 +20,7 @@ pub use enrollment::{EnrolledFeature, EnrollmentStatus};
pub use error::{NimbusError, Result};
#[cfg(debug_assertions)]
pub use evaluator::evaluate_enrollment;
pub use remote_settings::RemoteSettingsRecord;
pub use schema::*;
pub use targeting::NimbusTargetingHelper;

Expand All @@ -30,7 +31,6 @@ cfg_if::cfg_if! {

pub use stateful::nimbus_client::*;
pub use stateful::matcher::AppContext;
pub use remote_settings::{RemoteSettingsConfig, RemoteSettingsServer};
} else {
pub mod stateless;

Expand Down
10 changes: 7 additions & 3 deletions components/nimbus/src/nimbus.udl
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
[External="remote_settings"]
typedef record RemoteSettingsConfig;
typedef interface RemoteSettingsService;

[External="remote_settings"]
typedef enum RemoteSettingsServer;

[External="remote_settings"]
typedef record RemoteSettingsRecord;

dictionary CalculatedAttributes {
i32? days_since_install;
i32? days_since_update;
Expand Down Expand Up @@ -147,8 +150,9 @@ interface NimbusClient {
RecordedContext? recorded_context,
sequence<string> coenrolling_feature_ids,
string dbpath,
RemoteSettingsConfig? remote_settings_config,
MetricsHandler metrics_handler
MetricsHandler metrics_handler,
string? collection_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the collection name could be live in the nimbus code, but I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion would be that it lives in the client code, actually.

RemoteSettingsService? remote_settings_service
);

/// Initializes the database and caches enough information so that the
Expand Down
53 changes: 21 additions & 32 deletions components/nimbus/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// file, You can obtain one at https://mozilla.org/MPL/2.0/.

use crate::{defaults::Defaults, enrollment::ExperimentMetadata, NimbusError, Result};
use remote_settings::RemoteSettingsRecord;
use serde_derive::*;
use serde_json::{Map, Value};
use std::collections::HashSet;
Expand Down Expand Up @@ -102,40 +103,28 @@ impl ExperimentMetadata for Experiment {
}
}

pub fn parse_experiments(payload: &str) -> Result<Vec<Experiment>> {
// We first encode the response into a `serde_json::Value`
// to allow us to deserialize each experiment individually,
// omitting any malformed experiments
let value: Value = match serde_json::from_str(payload) {
Ok(v) => v,
Err(e) => {
return Err(NimbusError::JSONError(
"value = nimbus::schema::parse_experiments::serde_json::from_str".into(),
e.to_string(),
))
}
};
let data = value
.get("data")
.ok_or(NimbusError::InvalidExperimentFormat)?;
pub fn parse_experiments(records: Vec<RemoteSettingsRecord>) -> Result<Vec<Experiment>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can continue to input a string and parse it like before. That should reduce the number of test changes needed. Something like this should work:

pub fn parse_experiments(payload: &str) -> Result<Vec<Experiment>> {
  let records: Vec<RemoteSettingsRecord> = match serde_json::from_str(records) {
    Ok(v) => v,
    Err(e) => {
        return Err(NimbusError::JSONError(
            "value = nimbus::schema::parse_experiments::serde_json::from_str".into(),
            e.to_string(),
        ))
  };
  // .. same as before
}

// XXX: In the future it would be nice if this lxived in its own versioned crate so that
// the schema could be decoupled from the sdk so that it can be iterated on while the
// sdk depends on a particular version of the schema through the Cargo.toml.
let mut res = Vec::new();
for exp in data
.as_array()
.ok_or(NimbusError::InvalidExperimentFormat)?
{
// XXX: In the future it would be nice if this lived in its own versioned crate so that
// the schema could be decoupled from the sdk so that it can be iterated on while the
// sdk depends on a particular version of the schema through the Cargo.toml.
match serde_json::from_value::<Experiment>(exp.clone()) {
Ok(exp) => res.push(exp),
Err(e) => {
log::trace!("Malformed experiment data: {:#?}", exp);
log::warn!(
"Malformed experiment found! Experiment {}, Error: {}",
exp.get("id").unwrap_or(&serde_json::json!("ID_NOT_FOUND")),
e
);
for record in records {
if let Some(Value::Array(exp_data)) = record.fields.get("data") {
for entry in exp_data {
match serde_json::from_value::<Experiment>(entry.clone()) {
Ok(exp) => res.push(exp),
Err(e) => {
log::trace!("Malformed experiment data: {:#?}", record.fields);
log::warn!(
"Malformed experiment found! Experiment {}, Error: {}",
record.id,
e
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should return NimbusError::InvalidExperimentFormat if the if let doesn't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: tif this is about the array, ref 122

}
} else {
return Err(NimbusError::InvalidExperimentFormat);
}
}
Ok(res)
Expand Down
12 changes: 8 additions & 4 deletions components/nimbus/src/stateful/client/http_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,22 @@
//!
//! But the simple subset implemented here meets our needs for now.

use std::sync::Arc;

use crate::error::Result;
use crate::schema::parse_experiments;
use crate::stateful::client::{Experiment, SettingsClient};
use remote_settings::RemoteSettings;
use remote_settings::{RemoteSettingsClient, RemoteSettingsError};

impl SettingsClient for RemoteSettings {
impl SettingsClient for Arc<RemoteSettingsClient> {
fn get_experiments_metadata(&self) -> Result<String> {
unimplemented!();
}

fn fetch_experiments(&self) -> Result<Vec<Experiment>> {
let resp = self.get_records_raw()?;
parse_experiments(&resp.text())
let records = self.get_records(false).ok_or(RemoteSettingsError::Other {
reason: "Unable to fetch experiment records".to_owned(),
})?;
parse_experiments(records)
}
}
Loading