Closed Bug 1371398 Opened 4 years ago Closed 4 years ago

Add telemetry for time spent getting and setting browser.storage.local

Categories

(WebExtensions :: General, enhancement, P2)

51 Branch
enhancement

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

(Whiteboard: [metrics] triaged)

Attachments

(1 file)

We'd like to know how much time is spent during local.storage.get and local.storage.set, so we will introduce a couple of temporary (i.e., for the next 6 months) histograms to measure those.
We're talking about browser.storage.local here, despite my original typos.
Summary: Add telemetry for time spent getting and setting local storage → Add telemetry for time spent getting and setting browser.storage.local
There was some discussion about this on IRC, but nobody has added any comments to this bug, so, as per the discussion in today's meeting, I am going to proceed as planned.
Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158594

::: toolkit/components/extensions/ExtensionStorage.jsm:152
(Diff revision 1)
> +    let extensionId = context.extension.id;
> +    let extData = await this.read(extensionId);
> -      let changes = {};
> +    let changes = {};
> -      for (let prop in items) {
> +    for (let prop in items) {
> -        let item = items[prop];
> +      let item = items[prop];
> -        changes[prop] = {oldValue: extData[prop], newValue: item};
> +      changes[prop] = {oldValue: extData[prop], newValue: item};
> -        extData[prop] = item;
> +      extData[prop] = item;
> -      }
> +    }
>  
> -      this.notifyListeners(extensionId, changes);
> +    this.notifyListeners(extensionId, changes);
>  
> -      return this.write(extensionId);
> +    let result = await this.write(extensionId);

Some issues here:

1) We need to catch exceptions between the start() and finish().
2) We need to do this from ext-storage.js so we capture the overhead from messaging, serializing, and so forth.
3) The context is not a good enough context object for this, since we can have many parallel storage operations for the same context.
Attachment #8879690 - Flags: review?(kmaglione+bmo)
Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158948

::: toolkit/components/extensions/ext-c-storage.js:37
(Diff revision 2)
>      }
>      return {
>        storage: {
>          local: {
> -          get: function(keys) {
> +          get: async function(keys) {
> +            const stopwatchKey = {context, keys};

This can just be an empty object.

::: toolkit/components/extensions/ext-c-storage.js:47
(Diff revision 2)
> -              keys,
> +                keys,
> -            ]);
> +              ]);
> +              TelemetryStopwatch.finish(storageGetHistogram, stopwatchKey);
> +              return result;
> +            } catch (e) {
> +              TelemetryStopwatch.cancel(storageGetHistogram, stopwatchKey);

We need to re-throw the exception here.

::: toolkit/components/extensions/ext-c-storage.js:61
(Diff revision 2)
> -              items,
> +                items,
> -            ]);
> +              ]);
> +              TelemetryStopwatch.finish(storageSetHistogram, stopwatchKey);
> +              return result;
> +            } catch (e) {
> +              TelemetryStopwatch.cancel(storageSetHistogram, stopwatchKey);

We need to re-throw the exception here.
Attachment #8879690 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158948

> This can just be an empty object.

You mean I can just set it to an empty object? Is so, why would that work? I thought it was meant to be something unique? Or is it because an object created here will be unique each time it is created? If that's the case can I make the same change to the `set` method?
Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review158948

> You mean I can just set it to an empty object? Is so, why would that work? I thought it was meant to be something unique? Or is it because an object created here will be unique each time it is created? If that's the case can I make the same change to the `set` method?

A new object is created each time the method is called, and all that matters is the exact identity of the object, not what the object contains.
Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

Requesting data review for a temporary telemetry probe which will gather information about the performance of browser.storage.local in the wild, which we hope to use to make decisions about changing the implementation of the API.
Attachment #8879690 - Flags: feedback?(benjamin)
Comment on attachment 8879690 [details]
Bug 1371398 - Add telemetry for time spent getting and setting browser.storage.local,

https://reviewboard.mozilla.org/r/151040/#review160292

data-r=me
Attachment #8879690 - Flags: review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a05f39ab6781
Add telemetry for time spent getting and setting browser.storage.local, r=bsmedberg,kmag
Attachment #8879690 - Flags: feedback?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/a05f39ab6781
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1425957
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.