Closed Bug 1453692 Opened 2 years ago Closed 2 years ago

Add inspect method to Remote settings

Categories

(Firefox :: Remote Settings Client, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Future
Tracking Status
firefox62 --- fixed

People

(Reporter: leplatrem, Assigned: leplatrem)

References

Details

Attachments

(1 file)

In order to avoid the about:remotesettings extension (see Bug 1406036) to be too coupled to the RemoteSettings internal, I propose that we introduce an `inspect()` method, that would return:

Global:
- server url
- polling endpoint
- list of registered settings collections
- local timestamp
- server timestamp
- last check
- server backoff
- clock skew
- root hash

For each collection:
- collection name
- bucket name
- local timestamp
- server timestamp
- collection metadata
- last poll date
- signer name
Depends on: 1454970
Assignee: nobody → mathieu
Version: 57 Branch → Trunk
Comment on attachment 8984470 [details]
Bug 1453692 - Add RemoteSettings inspect() method

https://reviewboard.mozilla.org/r/250296/#review257000

This is mostly there but I wonder if we can parallelize the operations to get the last modified data etc. from the different collections.

::: services/common/tests/unit/test_blocklist_clients.js:310
(Diff revision 1)
> +  equal(localTimestamp, null); // never synchronized.
> +  equal(lastCheck, 0); // never synchronized.

This is a bit surprising because it's in the list of data in comment #0. Are consumers supposed to get this information some other way? Or does this only happen if we actually sync to the server, and in that case, why don't we do this here to actually get this data?

::: services/common/tests/unit/test_blocklist_clients.js:314
(Diff revision 1)
> +  equal(serverTimestamp, '"3000"');
> +  equal(localTimestamp, null); // never synchronized.
> +  equal(lastCheck, 0); // never synchronized.
> +  equal(mainBucket, "main");
> +  equal(serverURL, `http://localhost:${server.identity.primaryPort}/v1`);
> +  equal(pollingEndpoint, "/buckets/monitor/collections/changes/records");

How useful is this, given it's basically in the definition of the collection? Is there currently no way to get the definition options of the collection back out?

::: services/settings/remote-settings.js:627
(Diff revision 1)
> +   */
> +  async function _client(bucketName, collectionName) {
> +    // Check if a client was registered for this bucket/collection. Potentially
> +    // with some specific options like signer, filter function etc.
> +    const key = `${bucketName}/${collectionName}`;
> +    if (_clients.has(key)) {

Couldn't we just call `_clients.get(key)` and just check it's non-null (instead of doing the map lookup twice)? We will never store null entries into the Map, right?

::: services/settings/remote-settings.js:639
(Diff revision 1)
> +    } else if (bucketName == mainBucket && (await databaseExists(bucketName, collectionName) ||
> +                                            await hasLocalDump(bucketName, collectionName))) {

Could we race the databaseExists/hasLocalDump calls? Or do we have to run the former first? Or would it be a bad idea because we'd always do twice the IO and the general case is that the DB will exist except on first run?

::: services/settings/remote-settings.js:758
(Diff revision 1)
> +    const kintoServer = Services.prefs.getCharPref(PREF_SETTINGS_SERVER);
> +    const changesEndpoint = kintoServer + changesPath;

We do this in more than 1 place but the result can't differ unless `PREF_SETTINGS_SERVER` changes. Would it make sense to create a lazy getter instead?

::: services/settings/remote-settings.js:763
(Diff revision 1)
> +    for (const change of changes) {
> +      const { bucket, collection, last_modified: serverTimestamp } = change;
> +      const client = await _client(bucket, collection);
> +      if (!client) {
> +        continue;
> +      }
> +      const kintoCol = await client.openCollection();
> +      const localTimestamp = await kintoCol.db.getLastModified();
> +      const lastCheck = Services.prefs.getIntPref(client.lastCheckTimePref, 0);
> +      collections.push({
> +        bucket: client.bucketName,
> +        collection: client.collectionName,
> +        localTimestamp,
> +        serverTimestamp,
> +        lastCheck,
> +        signerName: client.signerName
> +      });
> +    }

These are async indexeddb operations, right? So rather than doing them linearly, could we race them by mapping the changes to individual promises and then awaiting `Promise.all()` on them instead?
Attachment #8984470 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8984470 [details]
Bug 1453692 - Add RemoteSettings inspect() method

https://reviewboard.mozilla.org/r/250296/#review257688

::: services/settings/remote-settings.js:616
(Diff revisions 1 - 3)
> +  Object.defineProperty(remoteSettings, "pollingEndpoint", {
> +    get() {
> +      const kintoServer = Services.prefs.getCharPref(PREF_SETTINGS_SERVER);
> +      const changesPath = Services.prefs.getCharPref(PREF_SETTINGS_CHANGES_PATH);
> +      return kintoServer + changesPath;
> +    }
> +  });

OK, but this isn't a lazy getter, just a getter... ;-)

Do these prefs change in tests or something? If they do not, I guess we could use XPCOMUtils.defineLazyGetter() instead (though then you have to import that file again...).
Attachment #8984470 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8984470 [details]
Bug 1453692 - Add RemoteSettings inspect() method

https://reviewboard.mozilla.org/r/250296/#review257688

> OK, but this isn't a lazy getter, just a getter... ;-)
> 
> Do these prefs change in tests or something? If they do not, I guess we could use XPCOMUtils.defineLazyGetter() instead (though then you have to import that file again...).

Yes, I change the server URL from the tests in order to test network errors and Telemetry. 

Also I would like our about:remotesettings extension to be able to change that preference in order to switch from PROD to STAGE etc.
Keywords: checkin-needed
Target Milestone: --- → Future
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/693ff336db30
Add RemoteSettings inspect() method r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/693ff336db30
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.