1552382 - Don't log "0 outgoing items pre-reconciliation" in the new bookmarks engine

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Lina Butler (ex-Mozilla)]

We'll log this for every sync with the new engine:

1558001468439 Sync.Engine.Bookmarks INFO 0 outgoing items pre-reconciliation

Since we don't use _reconcile, we don't know how many outgoing items we have until we've finished applying and merging everything—at which point we fill out outgoing changeset. In contrast, the old engine fills the changeset before downloading, so it'll have the right count.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:markh, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

This is a bit painful - the message is logged in the "base class" of the engines at https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/services/sync/modules/engines.js#1194. You will notice it ends up coming from the result of pullChanges() but looking in bookmarks.js you will see that engine always returns an empty object.

Just logging when there are non-zero changes doesn't seem correct - we probably want to keep logging for other engines even when there are zero. I had an idea how we could do some hacks so only bookmarks doesn't log this, but on reflection, I don't think the "pre reconcile" part actually matters and that it's reasonable for all engines to just log this post-reconcile.

And engines do that - but at trace level, which isn't ideal. So I think this really is just:

• delete that log entry above.
• change the log entry at https://searchfox.org/mozilla-central/rev/5e70cd673a0ba0ad19b662c1cf656e0823781596/services/sync/modules/engines.js#1840 so that it:
  ** Is before the branch - ie, so it always logs, even when zero entries.
  ** For fun, use js template strings to format the integer.
  ** Change the log level to .info()
  ** Change the message to something a bit better - s/Preparing/Uploading/ might be all that's necessary, but I'm open to anything :)

New tests will not be necessary for this change and it's extremely unlikely any tests would fail, but you should (a) run tests in services/sync locally and (b) push a "try" run (which might turn into its own adventure of getting hg access etc)

Comment 3

[Ben Dean-Kawamura [:bdk]]

Attachment: Bug 1552382: Updating sync engine logging. r=markh

• Don't log pre-reconciliation outgoing item count. Always outputs 0 for bookmarks and
  it's not needed for other engines.
• Updated the post-reconciliation outgoing item count log so that it can
  be used instead of the pre-reconciliation log item:
  • Always log, even when the count is 0
  • Change log level to INFO
  • Changed wording "Preparing" -> "Uploading"

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd0af92f57a8
Updating sync engine logging.  r=markh

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/fd0af92f57a8