|
|
Created:
5 years, 1 month ago by Michael Achenbach Modified:
5 years ago Reviewers:
Michael Hablich CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[release] Add monitoring state to auto-roller json output.
The option --json-output will make the auto-roller dump a
json file with a monitoring_state key. This can be one of:
started, up_to_date, success.
BUG=chromium:559141
LOG=n
NOTRY=true
Committed: https://crrev.com/8cd3cf297287e581a49e487067f5cbd991b27123
Cr-Commit-Position: refs/heads/master@{#32201}
Patch Set 1 #Patch Set 2 : Fix state and add test #
Total comments: 7
Patch Set 3 : Review #
Created: 5 years ago
Messages
Total messages: 14 (5 generated)
Description was changed from ========== [release] Add monitoring statue to auto-roller json output. BUG= ========== to ========== [release] Add monitoring statue to auto-roller json output. BUG=chromium:559141 LOG=n NOTRY=true ==========
machenbach@chromium.org changed reviewers: + hablich@chromium.org
Description was changed from ========== [release] Add monitoring statue to auto-roller json output. BUG=chromium:559141 LOG=n NOTRY=true ========== to ========== [release] Add monitoring statue to auto-roller json output. The option --json-output will make the auto-roller dump a json file with a monitoring_state key. This can be one of: started, up_to_date, success. BUG=chromium:559141 LOG=n NOTRY=true ==========
PTAL
It seems you want to report the step which created an error (or the success) if something goes wrong, isn't it? https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll.py File tools/release/auto_roll.py (right): https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... tools/release/auto_roll.py:27: self['json_output']['monitoring_state'] = 'started' It seems the state is reported when the script is winded down. This means that 'started' can only happen when there was an error. So, how about calling this not 'started' but 'error'? https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... tools/release/auto_roll.py:82: self['json_output']['monitoring_state'] = 'up_to_date' Why is this not a 'success'?
https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll.py File tools/release/auto_roll.py (right): https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... tools/release/auto_roll.py:27: self['json_output']['monitoring_state'] = 'started' On 2015/11/24 09:32:54, Hablich wrote: > It seems the state is reported when the script is winded down. This means that > 'started' can only happen when there was an error. So, how about calling this > not 'started' but 'error'? That'd be the meaning behind started. I didn't wanted to call it error as I still want to indicate that the actual script started. There would also be other error states where it doesn't start. WDYT? https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... tools/release/auto_roll.py:82: self['json_output']['monitoring_state'] = 'up_to_date' On 2015/11/24 09:32:54, Hablich wrote: > Why is this not a 'success'? Because I wan't to be able to monitor this case differently. If it claims it's up-to-date for too long it might actually be a problem. I.e. it is stale...
Description was changed from ========== [release] Add monitoring statue to auto-roller json output. The option --json-output will make the auto-roller dump a json file with a monitoring_state key. This can be one of: started, up_to_date, success. BUG=chromium:559141 LOG=n NOTRY=true ========== to ========== [release] Add monitoring state to auto-roller json output. The option --json-output will make the auto-roller dump a json file with a monitoring_state key. This can be one of: started, up_to_date, success. BUG=chromium:559141 LOG=n NOTRY=true ==========
https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll.py File tools/release/auto_roll.py (right): https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... tools/release/auto_roll.py:27: self['json_output']['monitoring_state'] = 'started' On 2015/11/24 10:08:35, Michael Achenbach wrote: > On 2015/11/24 09:32:54, Hablich wrote: > > It seems the state is reported when the script is winded down. This means that > > 'started' can only happen when there was an error. So, how about calling this > > not 'started' but 'error'? > > That'd be the meaning behind started. I didn't wanted to call it error as I > still want to indicate that the actual script started. There would also be other > error states where it doesn't start. WDYT? How about making it more verbose: Set 'monitoring_state' in each step. 'Started' IMO leaves too much open for interpretation. https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... tools/release/auto_roll.py:82: self['json_output']['monitoring_state'] = 'up_to_date' On 2015/11/24 10:08:35, Michael Achenbach wrote: > On 2015/11/24 09:32:54, Hablich wrote: > > Why is this not a 'success'? > > Because I wan't to be able to monitor this case differently. If it claims it's > up-to-date for too long it might actually be a problem. I.e. it is stale... See above comment. Interpretation of each exit state can than be done by the monitor and not the script itself anymore.
PTAL https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll.py File tools/release/auto_roll.py (right): https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... tools/release/auto_roll.py:27: self['json_output']['monitoring_state'] = 'started' On 2015/11/24 10:30:55, Hablich wrote: > On 2015/11/24 10:08:35, Michael Achenbach wrote: > > On 2015/11/24 09:32:54, Hablich wrote: > > > It seems the state is reported when the script is winded down. This means > that > > > 'started' can only happen when there was an error. So, how about calling > this > > > not 'started' but 'error'? > > > > That'd be the meaning behind started. I didn't wanted to call it error as I > > still want to indicate that the actual script started. There would also be > other > > error states where it doesn't start. WDYT? > > How about making it more verbose: Set 'monitoring_state' in each step. 'Started' > IMO leaves too much open for interpretation. OK. Done. Not sure if that verbosity helps, but it probably can't hurt (despite more code).
On 2015/11/24 10:44:48, Michael Achenbach wrote: > PTAL > > https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll.py > File tools/release/auto_roll.py (right): > > https://codereview.chromium.org/1465413002/diff/20001/tools/release/auto_roll... > tools/release/auto_roll.py:27: self['json_output']['monitoring_state'] = > 'started' > On 2015/11/24 10:30:55, Hablich wrote: > > On 2015/11/24 10:08:35, Michael Achenbach wrote: > > > On 2015/11/24 09:32:54, Hablich wrote: > > > > It seems the state is reported when the script is winded down. This means > > that > > > > 'started' can only happen when there was an error. So, how about calling > > this > > > > not 'started' but 'error'? > > > > > > That'd be the meaning behind started. I didn't wanted to call it error as I > > > still want to indicate that the actual script started. There would also be > > other > > > error states where it doesn't start. WDYT? > > > > How about making it more verbose: Set 'monitoring_state' in each step. > 'Started' > > IMO leaves too much open for interpretation. > > OK. Done. Not sure if that verbosity helps, but it probably can't hurt (despite > more code). lgtm
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1465413002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1465413002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8cd3cf297287e581a49e487067f5cbd991b27123 Cr-Commit-Position: refs/heads/master@{#32201} |