This would be handy to visually distinguish errors and success if one has logOnSuccess enabled. It would also be handy for the clean up (bug 663181) so that errors and success logs could be cleaned up separately (and potentially at a different pace)

added prefixes "success-" and "err-" to successful/erroneous sync logs. Also added unit-tests to check output filenames.

Comment on attachment 547825 [details] [diff] [review] success-/err- logfile prefixes Review of attachment 547825 [details] [diff] [review]: ----------------------------------------------------------------- Note "User" in hg patch header. See <http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed>. r- to take care of minor points below. ::: services/sync/modules/service.js @@ +478,5 @@ > fapp.level = Log4Moz.Level[Svc.Prefs.get("log.appender.file.level")]; > root.addAppender(fapp); > }, > > + _resetFileLog: function resetFileLog(flushToFile, isError) { Nit: try to make the two names agree. @@ +483,5 @@ > let inStream = this._logAppender.getInputStream(); > this._logAppender.reset(); > if (flushToFile && inStream) { > try { > + let filename = ((isError) ? "err-" : "success-") + Date.now() + ".log"; I'd prefer: _resetFileLog: function(flushToFile, filenamePrefix) { and then calling it with LOG_PREFIX_ERROR or LOG_PREFIX_SUCCESS, defined at the top of the file. Boolean args are usually a red flag to me (pun intended): you can't tell from an invocation of _resetFileLog what the second argument is supposed to do, and it doesn't generalize at all. http://mark-story.com/posts/view/the-argument-for-flag-arguments @@ +531,5 @@ > !Services.io.offline) { > this._ignorableErrorCount += 1; > } else { > + this._resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"), > + true); Nit: indentation. Align your arguments. @@ +546,5 @@ > this.logout(); > break; > default: > + this._resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"), > + true); Ditto. @@ +551,5 @@ > break; > } > break; > case "weave:service:sync:finish": > + this._resetFileLog(Svc.Prefs.get("log.appender.file.logOnSuccess"), Ditto. ::: services/sync/tests/unit/test_service_filelog.js @@ +71,5 @@ > let entries = logsdir.directoryEntries; > do_check_true(entries.hasMoreElements()); > let logfile = entries.getNext().QueryInterface(Ci.nsILocalFile); > do_check_eq(logfile.leafName.slice(-4), ".log"); > + do_check_eq(logfile.leafName.slice(0,8), "success-"); Update these to use the constants, of course.

> _resetFileLog: function(flushToFile, filenamePrefix) { > > and then calling it with LOG_PREFIX_ERROR or LOG_PREFIX_SUCCESS, defined at > the top of the file. > > Boolean args are usually a red flag to me (pun intended): you can't tell > from an invocation of _resetFileLog what the second argument is supposed to > do, and it doesn't generalize at all. I agree about const'ing the values. I would also like to use "error-..." instead of "err-". However, I still think that the logic which prefix should be used should live in _resetFileLog, not in the observers. > @@ +531,5 @@ > > !Services.io.offline) { > > this._ignorableErrorCount += 1; > > } else { > > + this._resetFileLog(Svc.Prefs.get("log.appender.file.logOnError"), > > + true); > > Nit: indentation. Align your arguments. Also, never indent with tabs please. Ever. :)

(In reply to comment #3) > I agree about const'ing the values. I would also like to use "error-..." > instead of "err-". However, I still think that the logic which prefix should > be used should live in _resetFileLog, not in the observers. Sure; the important point is that it's not "true" or "false". (Using consts which happen to be the actual prefixes just saves some work inside _resetFileLog -- i.e., "the logic" is "the identity function" :))

Comment on attachment 547825 [details] [diff] [review] success-/err- logfile prefixes ># HG changeset patch ># Parent 5d1799e16fe9991cf7c468a6dc35e0049fdb3543 ># User Philipp von Weitershausen <philipp@weitershausen.de> Also, you probably want to update your .hgrc file, unless you want me to take credit for your work ;)

username filled from .hgrc, fixed some indentation, .log changed to .txt

Comment on attachment 547854 [details] [diff] [review] username, indentation, .log->.txt r=philikon

STR To enable logging for successful sync, toggle services.sync.log.appender.file.logOnSuccess to true. Erroneous sync should always cause logging. Sync using Tools->Sync Now. If sync is successful, about:sync-log should have a log file of the form "success-<#>.txt" (e.g. success-13913898319.txt). To test error sync, somehow cause an erroneous sync. about:sync-log should contain a log file of the form "error-<#>.txt"

verified on nightly s-c builds of 20110725

Comment on attachment 547854 [details] [diff] [review] username, indentation, .log->.txt Requesting approval for Aurora because apparently Fennec isn't too happy about '.log' files (see bug 673674). To cope with that Fennec bug, we can simply transplant this patch which makes Sync use the '.txt' file ending. Patch has minimal risk and is already verified by QA.

Comment on attachment 547854 [details] [diff] [review] username, indentation, .log->.txt We're going to pass on this for aurora. We'll let it bubble up through the normal process

(In reply to comment #14) > We're going to pass on this for aurora. We'll let it bubble up through the > normal process Thanks Christian. Please do note that unless bug 673674 landed for Fennec 7, this would mean that Fennec 7 users would not be able to view Sync logs.

Yep, we're waiting for bug 673674 to be nominated