1453692 - Add inspect method to Remote settings

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, enhancement)

Description

[Mathieu Leplatre [:leplatrem]]

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

Comment 1

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1453692 - Add RemoteSettings inspect() method

Review commit: https://reviewboard.mozilla.org/r/250296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250296/

Comment 2

[:Gijs (out for now; he/him)]

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?

Comment 3

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8984470 [details]
Bug 1453692 - Add RemoteSettings inspect() method

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250296/diff/1-2/

Comment 4

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8984470 [details]
Bug 1453692 - Add RemoteSettings inspect() method

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250296/diff/2-3/

Comment 5

[:Gijs (out for now; he/him)]

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

Comment 6

[Mathieu Leplatre [:leplatrem]]

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.

Comment 7

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8984470 [details]
Bug 1453692 - Add RemoteSettings inspect() method

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250296/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/693ff336db30
Add RemoteSettings inspect() method r=Gijs

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/693ff336db30