Starting new Remote Settings API #6378

bendk · 2024-09-16T20:23:23Z

Added the RemoteSettingsService and RemoteSettingsClient types. These provide the new remote settings API that we're hoping to implement. The goal is to match the API of the official client more closely. The main changes are:

Downloaded data is stored locally
Getting/syncing records are split up into different operations to avoid hitting the network when getting settings values during startup.
Application-wide configuration is managed by RemoteSettingsService, which uses that to construct the clients.

This code is still a WIP. The hope is that this outlines the general shape of the new API and the work that needs to be done.

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.

Added the `RemoteSettingsService` and `RemoteSettingsClient` types. These provide the new remote settings API that we're hoping to implement. The goal is to match the API of the official client more closely. The main changes are: - Downloaded data is stored locally - Getting/syncing records are split up into different operations to avoid hitting the network when getting settings values during startup. - Application-wide configuration is managed by `RemoteSettingsService`, which uses that to construct the clients. This code is still a WIP. The hope is that this outlines the general shape of the new API and the work that needs to be done.

bendk · 2024-09-16T20:34:12Z

components/remote_settings/src/remote_settings.udl

+    /// Construct a new client with the configuration passed to the constructor
+    [Throws=RemoteSettingsError]
+    RemoteSettingsClient make_client(string collection_name);
+};


This seems like a nice idea to me, but it's not totally clear that we need it. How do others feel about this?

I agree, most consumers won't need to to specify more than collection name.

This could be a place where we gather references to existing clients, for example in order to switch their server (stage, dev) or bucket (main, main-preview) when needed

I'm not quite sure I understand the discussion here. Would this fit with the following use case?

iOS (or Android) code (swift/Kotlin) sets up the remote settings server to use stage + preview.

Rust based search client calls RemoteSettingsClient("search-config-v2").get().

Here I'd want the client to return the configuration according to the stage+preview server.

In other words, I would like the Rust based search client to be able to get the collection without having to worry about passing through the remote settings configuration.

I think we want the search client to be able to go directly to remote settings client as it'll avoid having to pass the configuration back and forth across the Uniffi interfaces.

And further clarification, I think the Remote Settings server settings should be app-global rather than per-collection, hence I don't really want the search client to have to deal with those as well.

Thanks for pointing this out. My proposal was that the consumer app would create a RemoteSettingsService and use that as a factory for all the clients/collections. RemoteSettingsService stores the server config and avoids the need for a global variable.

However, I can definitely see why that would be annoying for your use-case. In order for you to create remote settings clients that used the correct server, the consumer app would need to pass the RemoteSettingsService to you.

A global variable could simplify things, but then you have to worry about startup order. What happens if you constructed your client before the consumer app configured the server? I'm not sure if this is a big deal or not, but I like the fact that the RemoteSettingsService makes this more explicit.

I don't have strong feelings either way, what do others think?

codecov-commenter · 2024-09-16T20:39:57Z

Codecov Report

Attention: Patch coverage is 0% with 66 lines in your changes missing coverage. Please review.

Project coverage is 49.91%. Comparing base (b8b598c) to head (ec8a706).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
components/remote_settings/src/lib.rs	0.00%	43 Missing ⚠️
components/remote_settings/src/storage.rs	0.00%	16 Missing ⚠️
components/remote_settings/src/client.rs	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6378      +/-   ##
==========================================
+ Coverage   47.71%   49.91%   +2.19%     
==========================================
  Files         162      140      -22     
  Lines       14164    13541     -623     
==========================================
  Hits         6759     6759              
+ Misses       7405     6782     -623

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bendk · 2024-09-16T20:40:20Z

components/remote_settings/src/storage.rs

+    /// Get attachment data from storage
+    pub fn get_attachment_data(&self, _location: &str) -> Result<Option<Vec<u8>>> {
+        // Q: should we store attachment data directly in SQLite or put it in the filesystem?
+        todo!()


I'm personally kind of interested in just storing this in SQLite. AFAIK, attachment data is not that large. I think Suggest has some of the larger attachments, our biggest is 6m, then 2m, then <1m. It doesn't seem that bad to stick this in the database and we get things like atomicity for free.

I agree, using a DB gives us several benefits when it comes to deleting attachments of obsolete collections. (pruneAttachments() on desktop)

Presumably that data would then have to be returned across the Uniffi interface. Are we likely to hit performance issues with serialising that data vs saving it to a file?

Passing data across the FFI requires a couple extra copies, but so would saving/reading it to a file. I think it should be fine for the use-cases we're talking about, but should probably double-check that.

leplatrem

This is great 💯 Thank you Ben 👌

leplatrem · 2024-09-17T08:18:33Z

components/remote_settings/src/lib.rs

+}
+
+pub struct RemoteSettingsClient {
+    client: Client,


nit: it could be called httpClient

leplatrem · 2024-09-17T08:21:33Z

components/remote_settings/src/remote_settings.udl

+    /// Construct a new client with the configuration passed to the constructor
+    [Throws=RemoteSettingsError]
+    RemoteSettingsClient make_client(string collection_name);
+};


I agree, most consumers won't need to to specify more than collection name.

This could be a place where we gather references to existing clients, for example in order to switch their server (stage, dev) or bucket (main, main-preview) when needed

leplatrem · 2024-09-17T08:22:05Z

components/remote_settings/src/remote_settings.udl

+    RemoteSettingsServer server;
+    /// Directory to store data in.  Must be writable by the client.
+    string storage_dir;
+    /// Override bucket name ("main" by default, "preview" for preview collections)


nit:

Suggested change

/// Override bucket name ("main" by default, "preview" for preview collections)

/// Override bucket name ("main" by default, "main-preview" for preview collections)

leplatrem · 2024-09-17T08:24:52Z

components/remote_settings/src/storage.rs

+    /// Get attachment data from storage
+    pub fn get_attachment_data(&self, _location: &str) -> Result<Option<Vec<u8>>> {
+        // Q: should we store attachment data directly in SQLite or put it in the filesystem?
+        todo!()


I agree, using a DB gives us several benefits when it comes to deleting attachments of obsolete collections. (pruneAttachments() on desktop)

leplatrem · 2024-09-17T08:28:06Z

components/remote_settings/src/storage.rs

+    }
+
+    /// Store attachment data in storage
+    pub fn store_attachment_data(&self, _location: &str, _data: &[u8]) -> Result<()> {


This could maybe take a list of attachments in order to support bulk writes.
On desktop we have a situation where a consumer has to store 1300 tiny .pem files on first sync.

But we can also always add a new method later store_attachment_bulk() or whatever.

Standard8 · 2024-09-20T11:19:33Z

components/remote_settings/src/remote_settings.udl

+    /// Get the current set of records.
+    ///
+    /// If no records have been downloaded, this will return null.  Use `sync` to fetch the current
+    /// records from the server.
+    [Throws=RemoteSettingsError]
+    sequence<RemoteSettingsRecord>? get();


Are we going to include the dumps in remote settings as well?

This would avoid the application having to load them separately, or pass them in. We can also include some of the logic that the desktop client uses to determine when the local records should be updated from the dump or not.

If so, then I'd expect this to be more If no records have been downloaded, this will load from the local dumps, if found. Otherwise this will return null.

Yes, but I plan to add this later on. I'm wondering if that should be a different class though, since if you know there's a local dump then get() doesn't need to return a nullable value.

Standard8 · 2024-09-20T11:22:00Z

components/remote_settings/src/remote_settings.udl

+    /// Sync records with the server.
+    [Throws=RemoteSettingsError]
+    void sync();


Are we going to also periodically check with the server for updates? If so, it might be worth making clear that this is only needed in the force-sync case.

This is a great question. I think the periodic syncing should be driven by the embedding application, since they have a closer integration with the OS and can create background workers, etc. However, it seems annoying to make each remote settings client consumer have to call sync() for their client.

How about this:

We rename this method to force_sync() and documentation about that.

We add a RemoteSettingsService::sync() method, which is what the embedding application will call. That updates the remote settings data for all the clients that it knows about (or something similar, I'm not sure exactly how this should work). This also doesn't need to be tied to the RemoteSettingsService idea, it could just be a global function.

Standard8 · 2024-09-20T11:29:35Z

components/remote_settings/src/remote_settings.udl

+    /// Download an attachment with the provided id to the provided path.
+    [Throws=RemoteSettingsError]
+    void download_attachment_to_path(string attachment_id, string path);


Desktop currently has a slightly different interface. It downloads the attachment to the database (if not included in dumps locally), and then either allows loading from the dump, the database cache or to actually download it.

It also passes in the attachment record, which I believe means it can check hashes and out-of-date.

What's the advantage of downloading this to application specified file? Would it be better to have remote settings handle the file paths/cache, and then maybe tell the app where the path of the file is?

The desktop api currently returns blobs, but maybe storing the file under storage_path and returning the path within that would be better?

On Desktop we deprecated the file-base API, because it was tedious to purge old files etc.
If there are uniffi concerns, maybe we could have a hybrid approach, where downloaded files are tracked in a db and referenced as paths?

Let's just remove this method for now, I don't know of a real use-case for it.

Standard8 · 2024-09-20T11:32:36Z

components/remote_settings/src/storage.rs

+    /// Get attachment data from storage
+    pub fn get_attachment_data(&self, _location: &str) -> Result<Option<Vec<u8>>> {
+        // Q: should we store attachment data directly in SQLite or put it in the filesystem?
+        todo!()


Presumably that data would then have to be returned across the Uniffi interface. Are we likely to hit performance issues with serialising that data vs saving it to a file?

bendk added the WIP Work in progress label Sep 16, 2024

bendk force-pushed the push-xqlomszvusvu branch from 31d7958 to ec8a706 Compare September 16, 2024 20:30

bendk mentioned this pull request Sep 16, 2024

Remote settings caching support #6328

Closed

5 tasks

bendk commented Sep 16, 2024

View reviewed changes

leplatrem reviewed Sep 17, 2024

View reviewed changes

Standard8 reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Starting new Remote Settings API #6378

Starting new Remote Settings API #6378

bendk commented Sep 16, 2024

bendk Sep 16, 2024

leplatrem Sep 17, 2024

Standard8 Sep 20, 2024

Standard8 Sep 20, 2024

bendk Oct 3, 2024

codecov-commenter commented Sep 16, 2024

bendk Sep 16, 2024

leplatrem Sep 17, 2024

Standard8 Sep 20, 2024

bendk Oct 3, 2024

leplatrem left a comment

leplatrem Sep 17, 2024

leplatrem Sep 17, 2024

leplatrem Sep 17, 2024

leplatrem Sep 17, 2024

leplatrem Sep 17, 2024

Standard8 Sep 20, 2024

bendk Oct 3, 2024

Standard8 Sep 20, 2024

bendk Oct 3, 2024 •

edited

Loading

Standard8 Sep 20, 2024

leplatrem Sep 30, 2024

bendk Oct 3, 2024

Standard8 Sep 20, 2024

	/// Override bucket name ("main" by default, "preview" for preview collections)
	/// Override bucket name ("main" by default, "main-preview" for preview collections)

Starting new Remote Settings API #6378

Are you sure you want to change the base?

Starting new Remote Settings API #6378

Conversation

bendk commented Sep 16, 2024

Pull Request checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 16, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk Oct 3, 2024 •

edited

Loading