Bug 159539 – EWS console logs doesn't go to log file

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

When we run an ews slave locally, it generates some logs on console. These logs are very useful for debugging issues with ews. However these logs are lost when EWS runs from a script/launch-agent. These logs should go to a log file.

see <rdar://problem/24464570>.

[Aakash Jain]

Created attachment 283095 [details]
Proposed patch

Sample output (from running test-webkitpy)

[~/code/webkit/Tools/mac-ews-logs]$cat mac-ews_2016-07-07_17-21-06.log 
2016-07-07 17:21:06,529 - CAUTION: mac-ews will discard all local changes in "/mock-checkout"
2016-07-07 17:21:06,530 - Running WebKit mac-ews.
2016-07-07 17:21:06,532 - Running: webkit-patch --status-host=example.com clean --port=mac
2016-07-07 17:21:06,533 - Running: webkit-patch --status-host=example.com update --port=mac
2016-07-07 17:21:06,534 - Running: webkit-patch --status-host=example.com apply-attachment --no-update --non-interactive 10000 --port=mac
2016-07-07 17:21:06,535 - Running: webkit-patch --status-host=example.com build --no-clean --no-update --build-style=release --port=mac
2016-07-07 17:21:06,537 - Running: webkit-patch --status-host=example.com build-and-test --no-clean --no-update --test --non-interactive --build-style=release --port=Mac

[Alexey Proskuryakov]

Comment on attachment 283095 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283095&action=review

> Tools/ChangeLog:9
> +        (AbstractQueue.queue_log_path): Append timestamp in file name so that file doesn't get overwritten.

When do these log files get overwritten?

I do not quite understand how this patch works. There are already some logs saved to dial, but these have very little data. What mechanism is used for those logs? Can/should these be merged to use a shared mechanism?

> Tools/Scripts/webkitpy/tool/commands/queues.py:117
> -        return os.path.join(self._log_directory(), "%s.log" % self.name)
> +        return os.path.join(self._log_directory(), "{}_{:%Y-%m-%d_%H-%M-%S}.log".format(self.name, datetime.now()))

start-queue-mac.sh already has log rotation logic that renames old log files. We should delete it if doing it here is preferable.

[Alexey Proskuryakov]

> saved to dial

saved to disk

[Aakash Jain]

Created attachment 283199 [details]
Updated patch

Thanks for pointing me to start-queue-mac.sh, I was searching for that logic in webkitpy.

There were two logging mechanisms being used in the ews code, one is using _log statements, and other was using OutputTee. _log messages were only going to console(not to log file), as file handler was not configured. I am configuring the file handler for the logs in this patch. Also, I am removing OutputTee logic and making everything use the common _log mechanism.

Also, since we do not want two separate log files for queue logs and patch logs, I am removing work_item_log_path from everywhere, so that there is no confusion about log paths.

[Alexey Proskuryakov]

Comment on attachment 283199 [details]
Updated patch

Looks mostly good to me, however we found that this pollutes local file system every time test-webkitpy is run.

It seems nice to have separate logs for queue operation and for specific patches. Can that be retained?

[Aakash Jain]

Created attachment 283868 [details]
Updated patch

Used MockHost(and MockFileSystem) in unit tests, so that test-webkitpy doesn't pollutes actual filesystem. Added new logging class to supports logging to MockFileSystem.

Also, removed the changes related to OutputTee and removing work_item_log_path, will do it in a separate patch.

[David Kilzer (:ddkilzer)]

<rdar://problem/24464570>

[David Kilzer (:ddkilzer)]

Comment on attachment 283868 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283868&action=review

r=me.  I'll let you decide if you want to fix up the comments.

> Tools/Scripts/webkitpy/common/system/logutils.py:217
> +    # create log directory if required

Comments start with a capital letter and end with a period.
This comment could probably be removed since it's obvious what the code is doing as well.

> Tools/Scripts/webkitpy/common/system/logutils.py:222
> +    # configure the logger

Ditto.

[Aakash Jain]

Created attachment 283962 [details]
Updated patch

Thanks for the review.

Removed the extra comments.

[WebKit Commit Bot]

Comment on attachment 283962 [details]
Updated patch

Clearing flags on attachment: 283962

Committed r203386: <http://trac.webkit.org/changeset/203386>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke iOS EWS for some reason. Whenever test-webkitpy is run, something changes that prevent webkit-patch from loading EWS classes. Deleting .pyc files makes it work again.

Landed that as a temporary workaround in <http://trac.webkit.org/r203417>. This needs to be fixed for real, of course.