Closed
Bug 1371398
Opened 7 years ago
Closed 7 years ago
Add telemetry for time spent getting and setting browser.storage.local
Categories
(WebExtensions :: General, enhancement, P2)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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
Assignee | ||
Comment 2•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-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 ::: 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+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
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 8•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-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/#review160292 data-r=me
Attachment #8879690 -
Flags: review+
Comment 12•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8879690 -
Flags: feedback?(benjamin)
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a05f39ab6781
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•