Closed
Bug 1451152
Opened 6 years ago
Closed 6 years ago
Add telemetry for the size of the mirror database
Categories
(Firefox :: Sync, enhancement, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: markh, Assigned: lina)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
From bug 1450797 comment 1: We should probably have telemetry about [the mirror database] size first, like we do for places.sqlite and favicons.sqlite
Updated•6 years ago
|
Priority: -- → P2
Updated•6 years ago
|
Priority: P2 → P1
Updated•6 years ago
|
Assignee: nobody → kit
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
Comment on attachment 8969139 [details] Bug 1451152 - Record the bookmarks mirror database size in telemetry. https://reviewboard.mozilla.org/r/237854/#review243578 Looks good, although I'd say it should either always be absolute or always relative probably. ::: toolkit/components/places/SyncedBookmarksMirror.jsm:167 (Diff revision 1) > * Sets up the mirror database connection and upgrades the mirror to the > * newest schema version. Automatically recreates the mirror if it's corrupt; > * throws on failure. > * > * @param {String} options.path > - * The full path to the mirror database file. > + * The path to the mirror database file, either absolute or relative Why do we want to take both? ::: toolkit/components/places/SyncedBookmarksMirror.jsm:184 (Diff revision 1) > static async open(options) { > let db = await Sqlite.cloneStorageConnection({ > connection: PlacesUtils.history.DBConnection, > readOnly: false, > }); > + let path = OS.Path.join(OS.Constants.Path.profileDir, options.path); Does this behave the same on all platforms when options.path is absolute (which we now support?).
Attachment #8969139 -
Flags: review?(tchiovoloni) → review+
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969139 [details] Bug 1451152 - Record the bookmarks mirror database size in telemetry. https://reviewboard.mozilla.org/r/237854/#review243578 I think we always pass absolute paths now. I changed this to bring us in line with Sqlite.jsm (https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/toolkit/modules/Sqlite.jsm#940-941), and so that we're not accidentally relying on `OS.File.stat` normalizing the path in one case, and SQLite's `ATTACH` expanding the path in the other case.
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969139 [details] Bug 1451152 - Record the bookmarks mirror database size in telemetry. https://reviewboard.mozilla.org/r/237854/#review243578 > Does this behave the same on all platforms when options.path is absolute (which we now support?). Yep. `join(absolute1, absolute2) == absolute2`.
Comment 6•6 years ago
|
||
mozreview-review |
Comment on attachment 8969140 [details] Bug 1451152 - Record timing for merging, updating Places, and staging records in the bookmarks mirror. https://reviewboard.mozilla.org/r/237852/#review243584 Two suggestions which I'll leave it up to you whether or not to do. ::: toolkit/components/places/SyncedBookmarksMirror.jsm:401 (Diff revision 1) > let reasons = ignoreCounts[kind]; > let extra = {}; > for (let reason in reasons) { > let count = reasons[reason]; > if (count > 0) { > - extra[reason] = String(count); > + extra[reason] = count.toString(10); Was this returning a non-base-10 string? FWIW String(x) guarantees the result is a string, whereas x.toString can be overridden to return a non-string. That said, `.toString(10)` is probably more explicit and it shouldn't matter, since nobody should be overriding Number.prototype.toString like that in Firefox (outside of the js test suite or w/e). Honestly we might just want to move all the number => string conversions into `this.recordTelemetryEvent`, so that we don't have to do it across so many lines... There might be a reason we don't want to do that though, your call. ::: toolkit/components/places/SyncedBookmarksMirror.jsm:2778 (Diff revision 1) > } > > +// Executes a function and returns a `{ result, time }` tuple, where `result` is > +// the function's return value, and `time` is the time taken to execute the > +// function. > +async function withTiming(func) { I almost feel like it might make sense to a) have withTiming take a name of the thing it's timing, and b) log the time that took when trace logging is on. WDYT?
Attachment #8969140 -
Flags: review?(tchiovoloni) → review+
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969140 [details] Bug 1451152 - Record timing for merging, updating Places, and staging records in the bookmarks mirror. https://reviewboard.mozilla.org/r/237852/#review243584 > Was this returning a non-base-10 string? > > FWIW String(x) guarantees the result is a string, whereas x.toString can be overridden to return a non-string. That said, `.toString(10)` is probably more explicit and it shouldn't matter, since nobody should be overriding Number.prototype.toString like that in Firefox (outside of the js test suite or w/e). > > Honestly we might just want to move all the number => string conversions into `this.recordTelemetryEvent`, so that we don't have to do it across so many lines... There might be a reason we don't want to do that though, your call. Yeah, I thought it was a bit more explicit, to match `.toFixed(...)`...but there's no technical reason for the change, so I'm happy to revert. Moving the conversions and checks into `recordTelemetryEvent` is a good idea, thanks! > I almost feel like it might make sense to > > a) have withTiming take a name of the thing it's timing, and > b) log the time that took when trace logging is on. > > WDYT? Yes! I think we could even log this at the debug level, since it might help spot perf problems when folks upload logs, and we don't have many callers.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•6 years ago
|
||
Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/16d084813aa7 Record the bookmarks mirror database size in telemetry. r=tcsc https://hg.mozilla.org/integration/autoland/rev/72c16dae1e7f Record timing for merging, updating Places, and staging records in the bookmarks mirror. r=tcsc
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/16d084813aa7 https://hg.mozilla.org/mozilla-central/rev/72c16dae1e7f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in
before you can comment on or make changes to this bug.
Description
•