Move nimbus to new RS api #6662

tiftran · 2025-03-24T16:49:18Z

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

bendk

Looking good overall, I left a few suggestions mostly just to try to reduce the amount of changes needed. The biggest question in my mind is how easy it is for consumers to pass the RemoteSettingsService that they're passing to the suggest/relavency constructors. In theory it should be pretty easy, but I know consumers often want to initialize nimbus very early on in the process which could complicate things.

bendk · 2025-03-24T17:17:15Z

components/nimbus/src/schema.rs

+                            e
+                        );
+                    }
+                }


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

todo: tif this is about the array, ref 122

bendk · 2025-03-24T17:27:23Z

components/nimbus/src/nimbus.udl

-        RemoteSettingsConfig? remote_settings_config,
-        MetricsHandler metrics_handler
+        MetricsHandler metrics_handler,
+        string? collection_name,


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

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

bendk · 2025-03-24T17:38:22Z

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt

            )
        }

+        val remoteSettingsService = RemoteSettingsService(dataDir, remoteSettingsConfig)


We should input the RemoteSettingsService in the constructor. That way all the remote-settings consumers can share the same service.

You may want to do the same dance we did with the other components, where we add an optional argument in the constructor in a separate patch. I'm not so sure that's needed though now that we've already done a few of these. Maybe we could just introduce the breaking change as long as there's a PR to fix it ready.

bendk · 2025-03-24T17:43:21Z

components/nimbus/src/schema.rs

-    let data = value
-        .get("data")
-        .ok_or(NimbusError::InvalidExperimentFormat)?;
+pub fn parse_experiments(records: Vec<RemoteSettingsRecord>) -> Result<Vec<Experiment>> {


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 }

jeddai

I've done a cursory review, and this is a great start! There's quite a bit more that will need to be added for the Kotlin/Swift side of things. If you'd like any further guidance on that I'm more than happy to help — you're welcome to throw something on my calendar or I can provide answers to any questions you have here.

jeddai · 2025-04-10T17:15:46Z

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt

-            )
-        }
+
+        val collectionName = it.collection


Since the call to server?.let has been removed, it no longer refers to an object. I think the collectionName would need to be a parameter of the constructor moving forward

jeddai · 2025-04-10T18:20:44Z

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/Nimbus.kt

@@ -68,7 +67,7 @@ open class Nimbus(
    override val prefs: SharedPreferences? = null,
    appInfo: NimbusAppInfo,
    coenrollingFeatureIds: List<String>,
-    server: NimbusServerSettings?,
+    remoteSettingsService: RemoteSettingsService,


This will require an associated change in the constructor use here

application-services/components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusBuilder.kt

Lines 221 to 232 in b56ecd3

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

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?

jeddai · 2025-04-10T18:25:54Z

components/nimbus/ios/Nimbus/NimbusCreate.swift

-        recordedContext: RecordedContext? = nil
+        recordedContext: RecordedContext? = nil,
+        remoteSettingsService: RemoteSettingsService? = nil,
+        collectionName: String? = nil


This will require an associated change here

application-services/components/nimbus/ios/Nimbus/NimbusBuilder.swift

Lines 262 to 271 in b56ecd3

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)

}

jeddai · 2025-04-10T18:27:21Z

components/nimbus/src/nimbus.udl

-        RemoteSettingsConfig? remote_settings_config,
-        MetricsHandler metrics_handler
+        MetricsHandler metrics_handler,
+        string? collection_name,


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

jeddai · 2025-04-10T18:31:53Z

components/nimbus/src/stateful/evaluator.rs

@@ -3,12 +3,12 @@
 * file, You can obtain one at https://mozilla.org/MPL/2.0/. */

 use crate::stateful::persistence::{Database, StoreId};
+use crate::AppContext;


Was this change necessary?

tiftran changed the title ~~Move to new RS api~~ Move nimbus to new RS api Mar 24, 2025

bendk reviewed Mar 24, 2025

View reviewed changes

tiftran force-pushed the nimbus-rs branch 4 times, most recently from ef7bd07 to 990d9e2 Compare April 1, 2025 06:24

tiftran force-pushed the nimbus-rs branch 4 times, most recently from a1279cb to 06dd7dc Compare April 9, 2025 19:21

Move nimbus to new RS api

636c4cd

tiftran force-pushed the nimbus-rs branch from 06dd7dc to 636c4cd Compare April 10, 2025 02:07

jeddai reviewed Apr 10, 2025

View reviewed changes

tiftran force-pushed the nimbus-rs branch from 2f1ddf9 to 5d046ac Compare April 22, 2025 17:22

feedback changes

eb5c7b5

tiftran force-pushed the nimbus-rs branch from 5d046ac to eb5c7b5 Compare April 22, 2025 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Move nimbus to new RS api #6662

Move nimbus to new RS api #6662

Uh oh!

tiftran commented Mar 24, 2025

Uh oh!

bendk left a comment

Uh oh!

bendk Mar 24, 2025

Uh oh!

tiftran Apr 3, 2025

Uh oh!

bendk Mar 24, 2025

Uh oh!

jeddai Apr 10, 2025

Uh oh!

bendk Mar 24, 2025

Uh oh!

bendk Mar 24, 2025

Uh oh!

jeddai left a comment

Uh oh!

jeddai Apr 10, 2025

Uh oh!

jeddai Apr 10, 2025

Uh oh!

bendk Apr 10, 2025

Uh oh!

jeddai Apr 10, 2025

Uh oh!

jeddai Apr 10, 2025

Uh oh!

jeddai Apr 10, 2025

Uh oh!

Uh oh!

	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,
	)

	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)
	}

Move nimbus to new RS api #6662

Are you sure you want to change the base?

Move nimbus to new RS api #6662

Uh oh!

Conversation

tiftran commented Mar 24, 2025

Pull Request checklist

Uh oh!

bendk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeddai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!