Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1484253003:
[Android] Add record functionality to logcat monitor. (Closed)
|
Created:
5 years ago by mikecase (-- gone --) Modified:
5 years ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, mikecase+watch_chromium.org, yfriedman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Add record functionality to logcat monitor.
Because Android Chromium infra may be switching over to swarming,
we will need logic to record logcats that is triggered by
individual scripts and not the bots. This is because in the future
the bot that trigger the tests may not have the devices attached to
them.
BUG=
Committed: https://crrev.com/fde7a9ac09e8d509c77f0ee20fbb6b93ab93979d
Cr-Commit-Position: refs/heads/master@{#365878}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Removed lots of stuff. Addressed jbudorick's comments. #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 8
Patch Set 6 : Addressed comments. #Patch Set 7 : Use FindAll and WaitFor use the logcat file. #Patch Set 8 : #
Total comments: 6
Patch Set 9 : Fixed some issues tests found. Added file flushing logic and use unbuffered logcat output. #Patch Set 10 : Cleanup #
Total comments: 1
Patch Set 11 : #Patch Set 12 : Fixed behavior of WaitFor #
Total comments: 13
Patch Set 13 : Remove assertions. Raise Exceptions. Add Close() function. #
Total comments: 4
Patch Set 14 : Fixed nits. Use reraiser_thread now. #Patch Set 15 : Pass device serials to LogcatMonitorCommandErrors. #
Messages
Total messages: 27 (9 generated)
mikecase@chromium.org changed reviewers: + jbudorick@chromium.org, rnephew@chromium.org
tempdir.py was lifted straight from https://codereview.chromium.org/1415533007 since I haven't been able to land that yet.. PTAL. Using parallelizer pMap as suggested.
https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... File build/android/devil/android/logcat_recorder.py (right): https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:73: device.adb.Logcat(clear=True) Need to probably move this logcat clear call to Record() and call it synchronously. Otherwise, I guess it could be possible this call happens after a test starts or something.
https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... File build/android/devil/android/logcat_recorder.py (right): https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:15: from pylib import constants No pylib imports in devil. If you need something from constants, pull it over to an appropriate constants module in devil. https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:19: import download_from_google_storage Actually, if this is your use of constants, you should use tools/telemetry/catapult_base/cloud_storage.py instead. https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:21: class LogcatRecorder(object): I'm really not crazy about this being a separate module from LogcatMonitor. They're too close in intended functionality. We may need to rework the LogcatMonitor to handle this case, but that'd be ok; I think that module is subtly bugged anyway. Also, I'm not crazy about how this object runs across multiple devices. I think there should be one LogcatMonitorRecorderThing per device, and we should parallelize over that. This would imply building the merge logic on top of that layer, which should make it easier to throw away if we switch to per-device files. https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:33: assert output_file or output_gs_file, ('No output specified for ' nit: drop the entire string to the next line https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:70: def _Record(self): This function is doing _way_ too much: - recording the logcat from all devices - merging the logcats into one file - optionally writing that file to a provided location - optionally uploading that file to gs only the first of those should be done in LogcatMonitorRecorderThing https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:76: prefix = ('\n%s: ' % str(device)[-5:]) nit: no parens
Description was changed from ========== Add a LogcatRecorder class to record logcats. Because Android Chromium infra may be switching over to swarming, we will need logic to record logcats that is triggered by individual scripts and not the bots. This is because in the future the bot that trigger the tests may not have the devices attached to them. This adds a LogcatRecorder class to record logcats to files and/or upload them to Google Storage. BUG= ========== to ========== Add a record functionality to logcat monitor. Because Android Chromium infra may be switching over to swarming, we will need logic to record logcats that is triggered by individual scripts and not the bots. This is because in the future the bot that trigger the tests may not have the devices attached to them. BUG= ==========
Removed lots of stuff. This CL pretty much just adds functionality for logcat_monitor.py to write the logcat to a file. Also merged logcat_recorder.py into logcat_monitor.py https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... File build/android/devil/android/logcat_recorder.py (right): https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:15: from pylib import constants On 2015/12/01 at 03:48:12, jbudorick wrote: > No pylib imports in devil. If you need something from constants, pull it over to an appropriate constants module in devil. Removed all GS stuff from cl. So this is gone too. https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:19: import download_from_google_storage On 2015/12/01 at 03:48:12, jbudorick wrote: > Actually, if this is your use of constants, you should use tools/telemetry/catapult_base/cloud_storage.py instead. Removed all GS stuff from cl. So this is gone too. https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:21: class LogcatRecorder(object): On 2015/12/01 at 03:48:12, jbudorick wrote: > I'm really not crazy about this being a separate module from LogcatMonitor. They're too close in intended functionality. We may need to rework the LogcatMonitor to handle this case, but that'd be ok; I think that module is subtly bugged anyway. > > Also, I'm not crazy about how this object runs across multiple devices. I think there should be one LogcatMonitorRecorderThing per device, and we should parallelize over that. This would imply building the merge logic on top of that layer, which should make it easier to throw away if we switch to per-device files. Merged with to logcat_monitor (this means it is per device). https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:33: assert output_file or output_gs_file, ('No output specified for ' On 2015/12/01 at 03:48:12, jbudorick wrote: > nit: drop the entire string to the next line Removed all GS stuff from cl. So this is gone too. https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:70: def _Record(self): On 2015/12/01 at 03:48:12, jbudorick wrote: > This function is doing _way_ too much: > - recording the logcat from all devices > - merging the logcats into one file > - optionally writing that file to a provided location > - optionally uploading that file to gs > > only the first of those should be done in LogcatMonitorRecorderThing Now it just simply records the logcat to a file. https://codereview.chromium.org/1484253003/diff/1/build/android/devil/android... build/android/devil/android/logcat_recorder.py:76: prefix = ('\n%s: ' % str(device)[-5:]) On 2015/12/01 at 03:48:12, jbudorick wrote: > nit: no parens Done
ping. Pretty short review. Shouldn't take more than 10 mins. Thanks :D :D :D
https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:32: self.adb = adb Why self._adb to self.adb? https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:132: prefix = '\n%s: ' % str(self.adb)[-5:] Don't worry about the device leader, just dump the logcat to the file. https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:143: self.adb.Logcat(clear=True) This shouldn't be responsible for clearing the logcat; the logic that handles self._clear should be. https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:173: def RecordLogcats(cls, devices, logcat_dir): As noted offline, this should be nixed. However, having this save-to-file functionality as a context manager is a good idea. What about adding a kwargs to the constructor for a file to save the logcat to?
Addressed comments. Also took the liberty of removing the retry decorator from GetLogcatMonitor (since it doesnt do anything with the device). https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:32: self.adb = adb On 2015/12/09 at 15:33:15, jbudorick wrote: > Why self._adb to self.adb? Yeah, used monitor.adb in the classmethod that I am deleting. Changing this back to _adb. https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:132: prefix = '\n%s: ' % str(self.adb)[-5:] On 2015/12/09 at 15:33:16, jbudorick wrote: > Don't worry about the device leader, just dump the logcat to the file. k https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:143: self.adb.Logcat(clear=True) On 2015/12/09 at 15:33:15, jbudorick wrote: > This shouldn't be responsible for clearing the logcat; the logic that handles self._clear should be. good point. fixed https://codereview.chromium.org/1484253003/diff/80001/build/android/devil/and... build/android/devil/android/logcat_monitor.py:173: def RecordLogcats(cls, devices, logcat_dir): On 2015/12/09 at 15:33:16, jbudorick wrote: > As noted offline, this should be nixed. However, having this save-to-file functionality as a context manager is a good idea. What about adding a kwargs to the constructor for a file to save the logcat to? Deleting. This CL is getting smaller and smaller.
https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:87: f.seek(0, 2) Having this file open in two places seems ... hairy. https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:131: with open(self._record_file.name, 'r') as f: As written, we can only call FindAll while the logcat monitor is running. I think that makes sense for WaitFor, but I'm not sure it does for FindAll. https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:185: self._record_file.close() these two lines should be in a finally block in case shutil.copy fails.
Please take a look. Some changes from before... Now flushing logcat file at the beginning of calls to WaitFor and FindAll. This is because of problems tests found where if something was logged and then instantly called FindAll to search for it, it wouldnt be written to the file yet. Changed adb_wrapper.Logcat to grab all current output from Logcat command instead of line by line. This is because, like before, if something was logged and instantly called FindAll to search for it, there was a change the line wasn't even ever seen by the logcat_monitor yet. This at least greatly decreases the chance for that. Get file size of the recorded logcat file at the beginning of WaitFor and FindAll. This, probably won't make much difference in practice, but I mean theoretically you could write to the file really fast and FindAll would be searching through all the new logs that were written after it was initially called. Changed tests for WaitFor to run WaitFor async. Previously, WaitFor would look through logs logged before it was even called. So the test could log stuff, then "wait" for it to appear. Changed the temp file in logcat monitor to be closed only in __del__() to allow you to call FindAll to search through the logs even after the logcat monitor was stopped. So, you can do stuff like monitor = LogcatMonitor() with monitor: # do stuff while recording logcat with monitor: # do other stuff which also gets recorded. monitor.FindAll() https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:87: f.seek(0, 2) On 2015/12/15 at 01:33:03, jbudorick wrote: > Having this file open in two places seems ... hairy. Ack https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:131: with open(self._record_file.name, 'r') as f: On 2015/12/15 at 01:33:03, jbudorick wrote: > As written, we can only call FindAll while the logcat monitor is running. I think that makes sense for WaitFor, but I'm not sure it does for FindAll. Well, two ways I can do this I guess. (1) Have it look at the self._output_file if you specified one and stopped recording. Or (2) don't close the tempfile except in __del__ which will let it live even past a call to monitor.Stop(). I went with (2). This has the added benefit of being able to Start and Stop the same monitor multiple and just write to a single temp file. https://codereview.chromium.org/1484253003/diff/140001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:185: self._record_file.close() On 2015/12/15 at 01:33:03, jbudorick wrote: > these two lines should be in a finally block in case shutil.copy fails. Done https://codereview.chromium.org/1484253003/diff/180001/build/android/devil/ut... File build/android/devil/utils/cmd_helper.py (right): https://codereview.chromium.org/1484253003/diff/180001/build/android/devil/ut... build/android/devil/utils/cmd_helper.py:314: def IterCmdOutput(args, timeout=None, cwd=None, shell=False, Need the logcat output as soon as I can get it to make the logcat monitor work better. Getting it line by line causes trouble if you log something to the device and then instantly call FindAll() or something. Sometimes the logcat monitor hasnt processed enough lines in time.
Fixed behavior of WaitFor. Code can now be much simpler :D
https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:87: with open(self._record_file.name, 'r') as f: WaitFor now starts looking from the start of the file instead of seeking to the end. https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... File build/android/devil/android/logcat_monitor_test.py (right): https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor_test.py:91: test_log.WaitFor(r'.*last line.*', None) These WaitFor is to stop test flakiness. Otherwise, there could be a chance the logcat monitor is stopped before anything is written to the logcat file.
Description was changed from ========== Add a record functionality to logcat monitor. Because Android Chromium infra may be switching over to swarming, we will need logic to record logcats that is triggered by individual scripts and not the bots. This is because in the future the bot that trigger the tests may not have the devices attached to them. BUG= ========== to ========== [Android] Add record functionality to logcat monitor. Because Android Chromium infra may be switching over to swarming, we will need logic to record logcats that is triggered by individual scripts and not the bots. This is because in the future the bot that trigger the tests may not have the devices attached to them. BUG= ==========
I'm still concerned about the multiple file handles, especially writing to one and reading to the other. https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:11: import threading nit: import order https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:44: self._flush_event = threading.Event() nit: alpha https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:73: 'Must be currently recording logcat.') see below re assertions https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:117: assert self._record_file is not None, ( see below re assertions https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:154: assert self._record_thread is None, ( I don't think these should be assertions. Raise an exception (and document it) if you want, but this state is incorrect use of a public interface and should therefore not be an assertion. https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:199: if self._record_file: I think this should be moved into a separate public function that a client can call if it wants to cooperate. __del__ should then call that function and log a warning if the file was still open.
Fixed everything you mentioned (except for the having same file open multiple times. That seems to be fine. But Im open to alternatives). https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:44: self._flush_event = threading.Event() On 2015/12/17 at 01:49:05, jbudorick wrote: > nit: alpha Done (if alpha means "you forgot to delete this") https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:73: 'Must be currently recording logcat.') On 2015/12/17 at 01:49:05, jbudorick wrote: > see below re assertions now raise a LogcatMonitorCommandError https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:117: assert self._record_file is not None, ( On 2015/12/17 at 01:49:05, jbudorick wrote: > see below re assertions now raise a LogcatMonitorCommandError https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:154: assert self._record_thread is None, ( On 2015/12/17 at 01:49:05, jbudorick wrote: > I don't think these should be assertions. Raise an exception (and document it) if you want, but this state is incorrect use of a public interface and should therefore not be an assertion. Done. I just added an "if not self._record_thread" to make _StartRecording a noop if someone calls Start twice before Stop. https://codereview.chromium.org/1484253003/diff/220001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:199: if self._record_file: On 2015/12/17 at 01:49:05, jbudorick wrote: > I think this should be moved into a separate public function that a client can call if it wants to cooperate. __del__ should then call that function and log a warning if the file was still open. Added a Close() method that you should call. Still have this logic in __del__ as well as a logging warning asking people to call Close() if they are not doing it.
lgtm w/ nits https://codereview.chromium.org/1484253003/diff/240001/build/android/devil/an... File build/android/devil/android/logcat_monitor.py (right): https://codereview.chromium.org/1484253003/diff/240001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:12: import shutil nit: this is still out of order https://codereview.chromium.org/1484253003/diff/240001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:160: self._record_thread = threading.Thread(target=record_to_file) nit: use a reraiser_thread here, and call ReraiseIfException after joining the thread. https://codereview.chromium.org/1484253003/diff/240001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:197: self._record_file.close() set self._record_file to None here. https://codereview.chromium.org/1484253003/diff/240001/build/android/devil/an... build/android/devil/android/logcat_monitor.py:214: class LogcatMonitorCommandError(Exception): nit: I think this should inherit from device_errors.CommandFailedError.
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1484253003/#ps260001 (title: "Fixed nits. Use reraiser_thread now.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1484253003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1484253003/260001
The CQ bit was unchecked by mikecase@chromium.org
The CQ bit was checked by mikecase@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1484253003/#ps280001 (title: "Pass device serials to LogcatMonitorCommandErrors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1484253003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1484253003/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Add record functionality to logcat monitor. Because Android Chromium infra may be switching over to swarming, we will need logic to record logcats that is triggered by individual scripts and not the bots. This is because in the future the bot that trigger the tests may not have the devices attached to them. BUG= ========== to ========== [Android] Add record functionality to logcat monitor. Because Android Chromium infra may be switching over to swarming, we will need logic to record logcats that is triggered by individual scripts and not the bots. This is because in the future the bot that trigger the tests may not have the devices attached to them. BUG= Committed: https://crrev.com/fde7a9ac09e8d509c77f0ee20fbb6b93ab93979d Cr-Commit-Position: refs/heads/master@{#365878} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/fde7a9ac09e8d509c77f0ee20fbb6b93ab93979d Cr-Commit-Position: refs/heads/master@{#365878} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
