Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2040993002:
sandwich: Uplift for production use. (Closed)
|
Created:
4 years, 6 months ago by gabadie Modified:
4 years, 6 months ago CC:
chromium-reviews, mikecase+watch_chromium.org, gabadie+watch_chromium.org, jbudorick+watch_chromium.org, lizeb+watch-android-loading_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionsandwich: Uplift for production use.
This CL contains several change to uplift sandwich:
1) Finds Android DeviceUtil object from device serial only once
per command line to speed up sandwich graph generation.
2) Makes SandwichRunner.output_dir mandatory so it can always dumps
error files in case of error or intermittent failures.
3) Lets AndroidChromeController dismiss crash dialogs when browser
or renderer process crashes.
BUG=582080
Committed: https://crrev.com/cb1fb319fa271c43e982f256c9365f987c4f9d3c
Cr-Commit-Position: refs/heads/master@{#398032}
Patch Set 1 #
Total comments: 8
Messages
Total messages: 17 (6 generated)
gabadie@chromium.org changed reviewers: + pasko@chromium.org
Hey Egor, Here are some uplifts for sandwich. https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/contr... File tools/android/loading/controller.py (right): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/contr... tools/android/loading/controller.py:414: break FYI: copy pasted from https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry...
Description was changed from ========== sandwich: Make some uplift for production use. This CL contains severals change to enhance sandwich: 1) Finds Android DeviceUtil object from device serial only once per command line to speed up sandwich graph generation. 2) Makes SandwichRunner.output_dir mendatory so it can always dumps error files in case of error or intermittent failures. 3) Lets AndroidChromeController dismiss crash dialogs when browser or renderer process crashes. BUG=582080 ========== to ========== sandwich: Uplift for production use. This CL contains several change to uplift sandwich: 1) Finds Android DeviceUtil object from device serial only once per command line to speed up sandwich graph generation. 2) Makes SandwichRunner.output_dir mandatory so it can always dumps error files in case of error or intermittent failures. 3) Lets AndroidChromeController dismiss crash dialogs when browser or renderer process crashes. BUG=582080 ==========
lgtm with questions https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/contr... File tools/android/loading/controller.py (right): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/contr... tools/android/loading/controller.py:414: break On 2016/06/06 13:40:37, gabadie wrote: > FYI: copy pasted from > https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry... Thanks for the pointer. Ugh. https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_runner.py (left): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_runner.py:143: if self.output_dir is not None and run_id is not None: yeah, I always wondered why we needed SandwichRunner.output_dir to be None in the first place. Was it for some sort of integration test? https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_runner.py (right): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_runner.py:203: error.RaiseOriginal() what does not work with raising ChromeControllerError again via "raise" with no args?
https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_runner.py (left): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_runner.py:143: if self.output_dir is not None and run_id is not None: On 2016/06/06 13:58:52, pasko wrote: > yeah, I always wondered why we needed SandwichRunner.output_dir to be None in > the first place. Was it for some sort of integration test? No that was just to not record the trace for WPR recording. But it is actually more important to have error in WPR recording fails rather than not saving the trace. https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_runner.py (right): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_runner.py:203: error.RaiseOriginal() On 2016/06/06 13:58:52, pasko wrote: > what does not work with raising ChromeControllerError again via "raise" with no > args? 'raise' would raise the previously catched exception. So here it would raise the ChromeControllerError. The point of RaiseOriginal() is to raise back the exception that as failed within the with chrome_ctl.Open(). These way we have the full stack trace outputed in the stderr.
thanks! please add mattcary or lizeb to review the controller part
mattcary@chromium.org changed reviewers: + mattcary@chromium.org
lgtm https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_runner.py (right): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_runner.py:203: error.RaiseOriginal() On 2016/06/06 14:03:55, gabadie wrote: > On 2016/06/06 13:58:52, pasko wrote: > > what does not work with raising ChromeControllerError again via "raise" with > no > > args? > > 'raise' would raise the previously catched exception. So here it would raise the > ChromeControllerError. The point of RaiseOriginal() is to raise back the > exception that as failed within the with chrome_ctl.Open(). These way we have > the full stack trace outputed in the stderr. A disadvantage of this approach is that we lose the fact that we handled & reraised the exception as a ChromeControllerError. An alternative might be to extend the ChromeControllerError so that it prints out the whole stack trace. However, doing that reliably I think is difficult. I think re-raising the original can be a little surprising, but will be more useful than just propagating the ChromeControllerError.
Thanks Matt! Landing. https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... File tools/android/loading/sandwich_runner.py (right): https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... tools/android/loading/sandwich_runner.py:203: error.RaiseOriginal() On 2016/06/06 14:37:09, mattcary wrote: > On 2016/06/06 14:03:55, gabadie wrote: > > On 2016/06/06 13:58:52, pasko wrote: > > > what does not work with raising ChromeControllerError again via "raise" with > > no > > > args? > > > > 'raise' would raise the previously catched exception. So here it would raise > the > > ChromeControllerError. The point of RaiseOriginal() is to raise back the > > exception that as failed within the with chrome_ctl.Open(). These way we have > > the full stack trace outputed in the stderr. > > A disadvantage of this approach is that we lose the fact that we handled & > reraised the exception as a ChromeControllerError. An alternative might be to > extend the ChromeControllerError so that it prints out the whole stack trace. > > However, doing that reliably I think is difficult. I think re-raising the > original can be a little surprising, but will be more useful than just > propagating the ChromeControllerError. I don't see where the disadvantage is. The point of the chrome controller was to provide an exception bundling with additional information about chrome such as the chrome log. But the quantity of information its contains is slightly too big, especially with the verbose Android's logcat. At this call time, the chrome controller error has already dumped himself out into a file. Therefore his life goal of providing additional debug data is completed. Then reraising the original exception in case of a non intermittent failure then become completely fine because it also make the upper layers not having to handle the ChromeControllerError border case, and giving a clean stack trace where the original excepetion as been thrown into the stderr, which makes debugging far more friendly.
The CQ bit was checked by gabadie@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2040993002/1
On 2016/06/06 14:55:14, gabadie wrote: > Thanks Matt! Landing. > > https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... > File tools/android/loading/sandwich_runner.py (right): > > https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... > tools/android/loading/sandwich_runner.py:203: error.RaiseOriginal() > On 2016/06/06 14:37:09, mattcary wrote: > > On 2016/06/06 14:03:55, gabadie wrote: > > > On 2016/06/06 13:58:52, pasko wrote: > > > > what does not work with raising ChromeControllerError again via "raise" > with > > > no > > > > args? > > > > > > 'raise' would raise the previously catched exception. So here it would raise > > the > > > ChromeControllerError. The point of RaiseOriginal() is to raise back the > > > exception that as failed within the with chrome_ctl.Open(). These way we > have > > > the full stack trace outputed in the stderr. > > > > A disadvantage of this approach is that we lose the fact that we handled & > > reraised the exception as a ChromeControllerError. An alternative might be to > > extend the ChromeControllerError so that it prints out the whole stack trace. > > > > However, doing that reliably I think is difficult. I think re-raising the > > original can be a little surprising, but will be more useful than just > > propagating the ChromeControllerError. > > I don't see where the disadvantage is. The point of the chrome controller was to > provide an exception bundling with additional information about chrome such as > the chrome log. But the quantity of information its contains is slightly too > big, especially with the verbose Android's logcat. > > At this call time, the chrome controller error has already dumped himself out > into a file. Therefore his life goal of providing additional debug data is > completed. Then reraising the original exception in case of a non intermittent > failure then become completely fine because it also make the upper layers not > having to handle the ChromeControllerError border case, and giving a clean stack > trace where the original excepetion as been thrown into the stderr, which makes > debugging far more friendly. Someone looking at an exception who isn't intimately familiar with sandwich may be confused because the printed stack trace won't match what actually happened (the processing between the original exception, and the re-raising). The fact that full information is dumped somewhere else in the log may not be obvious. This sort of issue is why fiddling with exceptions can be problematic. However, for the reasons I explained I think what you've done is the least worse option.
Message was sent while issue was closed.
Description was changed from ========== sandwich: Uplift for production use. This CL contains several change to uplift sandwich: 1) Finds Android DeviceUtil object from device serial only once per command line to speed up sandwich graph generation. 2) Makes SandwichRunner.output_dir mandatory so it can always dumps error files in case of error or intermittent failures. 3) Lets AndroidChromeController dismiss crash dialogs when browser or renderer process crashes. BUG=582080 ========== to ========== sandwich: Uplift for production use. This CL contains several change to uplift sandwich: 1) Finds Android DeviceUtil object from device serial only once per command line to speed up sandwich graph generation. 2) Makes SandwichRunner.output_dir mandatory so it can always dumps error files in case of error or intermittent failures. 3) Lets AndroidChromeController dismiss crash dialogs when browser or renderer process crashes. BUG=582080 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== sandwich: Uplift for production use. This CL contains several change to uplift sandwich: 1) Finds Android DeviceUtil object from device serial only once per command line to speed up sandwich graph generation. 2) Makes SandwichRunner.output_dir mandatory so it can always dumps error files in case of error or intermittent failures. 3) Lets AndroidChromeController dismiss crash dialogs when browser or renderer process crashes. BUG=582080 ========== to ========== sandwich: Uplift for production use. This CL contains several change to uplift sandwich: 1) Finds Android DeviceUtil object from device serial only once per command line to speed up sandwich graph generation. 2) Makes SandwichRunner.output_dir mandatory so it can always dumps error files in case of error or intermittent failures. 3) Lets AndroidChromeController dismiss crash dialogs when browser or renderer process crashes. BUG=582080 Committed: https://crrev.com/cb1fb319fa271c43e982f256c9365f987c4f9d3c Cr-Commit-Position: refs/heads/master@{#398032} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/cb1fb319fa271c43e982f256c9365f987c4f9d3c Cr-Commit-Position: refs/heads/master@{#398032}
Message was sent while issue was closed.
On 2016/06/06 14:58:57, mattcary wrote: > On 2016/06/06 14:55:14, gabadie wrote: > > Thanks Matt! Landing. > > > > > https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... > > File tools/android/loading/sandwich_runner.py (right): > > > > > https://codereview.chromium.org/2040993002/diff/1/tools/android/loading/sandw... > > tools/android/loading/sandwich_runner.py:203: error.RaiseOriginal() > > On 2016/06/06 14:37:09, mattcary wrote: > > > On 2016/06/06 14:03:55, gabadie wrote: > > > > On 2016/06/06 13:58:52, pasko wrote: > > > > > what does not work with raising ChromeControllerError again via "raise" > > with > > > > no > > > > > args? > > > > > > > > 'raise' would raise the previously catched exception. So here it would > raise > > > the > > > > ChromeControllerError. The point of RaiseOriginal() is to raise back the > > > > exception that as failed within the with chrome_ctl.Open(). These way we > > have > > > > the full stack trace outputed in the stderr. > > > > > > A disadvantage of this approach is that we lose the fact that we handled & > > > reraised the exception as a ChromeControllerError. An alternative might be > to > > > extend the ChromeControllerError so that it prints out the whole stack > trace. > > > > > > However, doing that reliably I think is difficult. I think re-raising the > > > original can be a little surprising, but will be more useful than just > > > propagating the ChromeControllerError. > > > > I don't see where the disadvantage is. The point of the chrome controller was > to > > provide an exception bundling with additional information about chrome such as > > the chrome log. But the quantity of information its contains is slightly too > > big, especially with the verbose Android's logcat. > > > > At this call time, the chrome controller error has already dumped himself out > > into a file. Therefore his life goal of providing additional debug data is > > completed. Then reraising the original exception in case of a non intermittent > > failure then become completely fine because it also make the upper layers not > > having to handle the ChromeControllerError border case, and giving a clean > stack > > trace where the original excepetion as been thrown into the stderr, which > makes > > debugging far more friendly. > > Someone looking at an exception who isn't intimately familiar with sandwich may > be confused because the printed stack trace won't match what actually happened > (the processing between the original exception, and the re-raising). The fact > that full information is dumped somewhere else in the log may not be obvious. > > This sort of issue is why fiddling with exceptions can be problematic. However, > for the reasons I explained I think what you've done is the least worse option. Oh I see. Thanks for the explanation! =) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
