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)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: markh, Assigned: lina)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1451152 - Record the bookmarks mirror database size in telemetry.
59 bytes, text/x-review-board-request
tcsc
: review+
Details
Bug 1451152 - Record timing for merging, updating Places, and staging records in the bookmarks mirror.
59 bytes, text/x-review-board-request
tcsc
: review+
Details 
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
Priority: -- → P2
Priority: P2 → P1
Assignee: nobody → kit
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+
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.
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 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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/16d084813aa7
https://hg.mozilla.org/mozilla-central/rev/72c16dae1e7f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: