Bug 160585 - Some EWS console logs doesn't go to log file
Summary: Some EWS console logs doesn't go to log file
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
Depends on:
Reported: 2016-08-04 18:51 PDT by Aakash Jain
Modified: 2016-08-22 11:31 PDT (History)
6 users (show)

See Also:

Proposed patch (4.01 KB, patch)
2016-08-04 19:10 PDT, Aakash Jain
dbates: review-
dbates: commit-queue-
Details | Formatted Diff | Diff
Updated patch (6.06 KB, patch)
2016-08-05 17:32 PDT, Aakash Jain
dbates: review+
dbates: commit-queue-
Details | Formatted Diff | Diff
Added the comment, also moved logging initialization to queueengine.py from queues.py (20.29 KB, patch)
2016-08-08 16:07 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2016-08-04 18:51:40 PDT
After the fix of https://bugs.webkit.org/show_bug.cgi?id=159539, some of the logs started going to log files. However all the console logs doesn't go to the log file. Only logs from the log statements in webkitpy/tool/commands/queues.py are going to log file. This is because the logger from only that module is configured to log to file. We should ensure that all the console logs go to logfile.
Comment 1 Aakash Jain 2016-08-04 19:10:31 PDT
Created attachment 285393 [details]
Proposed patch
Comment 2 Daniel Bates 2016-08-05 16:51:50 PDT
Comment on attachment 285393 [details]
Proposed patch

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

Please add tests to Tools/Scripts/webkitpy/common/system/filesystem_unittest.py for this change.

> Tools/Scripts/webkitpy/common/system/filesystem.py:238
> +    def open_text_file_for_writing(self, path, mode='w'):

Expose the mode argument of codes.open() is error prone because it allows a person to violate the purpose of this function - to open a file for writing. For example, suppose a person makes the illogical call open_text_file_for_writing("a.txt", "r") then the file a.txt will be opened in read-only mode. In general when designing an interface we want to make it easy to use correctly and hard to use incorrectly. Clearly, the proposed change makes it easy to use this function incorrectly. I suggest that we add new boolean parameter to this function, called shouldAppend or append, that determines whether we open the specified file in write-only mode or in append-mode. This parameter should default to False - open the file in write-only mode.
Comment 3 Aakash Jain 2016-08-05 17:32:55 PDT
Created attachment 285469 [details]
Updated patch
Comment 4 Daniel Bates 2016-08-08 13:59:17 PDT
Comment on attachment 285469 [details]
Updated patch

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

> Tools/Scripts/webkitpy/tool/commands/queues.py:126
> +        logutils.configure_logger_to_log_to_file(logging.getLogger("webkitpy"), self.queue_log_path(), self.host.filesystem)

I suggest that we add a comment in this function to explain that we explicitly pass 'logging.getLogger("webkitpy")' instead of _log because we want to capture all messages logged from webkitpy modules.
Comment 5 Aakash Jain 2016-08-08 16:07:12 PDT
Created attachment 285598 [details]
Added the comment, also moved logging initialization to queueengine.py from queues.py
Comment 6 Daniel Bates 2016-08-08 16:46:13 PDT
Comment on attachment 285598 [details]
Added the comment, also moved logging initialization to queueengine.py from queues.py

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

> Tools/ChangeLog:11
> +        (FileSystemHandler._open): Ensure that we open logfile in append mode in order to avoid

Nit: logfile => log file

> Tools/ChangeLog:21
> +        (QueueEngine._begin_logging): Configure the python logger to log to file.

Nit: python => Python

> Tools/Scripts/webkitpy/common/system/logutils.py:226
>      logger.addHandler(handler)
> +    return handler

Instead of modifying this function so as to make its behavior even more similar to logutils.configure_logging() I suggest we take this opportunity to remove this function entirely and have the caller call logutils.configure_logging() directly.

> Tools/Scripts/webkitpy/common/system/logutils.py:230
> +def remove_handler_from_logger(logger, handler):
> +    logger.removeHandler(handler)

I do not see the need for this convenience function. The caller should just call logger.removeHandler(handler) directly.
Comment 7 Aakash Jain 2016-08-09 10:43:24 PDT
This was landed manually in https://trac.webkit.org/changeset/204275. 
Discussed with Dan and split it into two patches. The other portion is in https://bugs.webkit.org/show_bug.cgi?id=160698
Comment 8 Brent Fulgham 2016-08-22 11:31:34 PDT
Comment on attachment 285598 [details]
Added the comment, also moved logging initialization to queueengine.py from queues.py

Clearing flags since this patch has been landed.