Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2514413003:
[Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon (Closed)
|
Created:
4 years, 1 month ago by Sharu Jiang Modified:
4 years ago CC:
chromium-reviews, infra-reviews+infra_chromium.org, chanli, lijeffrey, mbarbella (wrong one) Target Ref:
refs/heads/master Project:
infra Visibility:
Public. |
Description[Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon
BUG=605365
Committed: https://chromium.googlesource.com/infra/infra/+/55d43e66cf63ee1c9f7101333184a8911b810c76
Patch Set 1 : . #
Total comments: 4
Patch Set 2 : Update names of metrics. #
Total comments: 2
Patch Set 3 : Update docs. #
Total comments: 2
Patch Set 4 : Update doc. #Patch Set 5 : Rebase #
Messages
Total messages: 22 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon BUG= ========== to ========== [Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon BUG= ==========
katesonia@chromium.org changed reviewers: + stgao@chromium.org, wrengr@chromium.org
PTAL :)
https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/crash_pipeline.py (right): https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... appengine/findit/crash/crash_pipeline.py:166: metric.increment({tag_name: tag_value, What's the tag value? How many different values it could have? For a field in a metric, the value of the field should be bounded. https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/monitoring.py (right): https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... appengine/findit/crash/monitoring.py:7: found_suspects = gae_ts_mon.CounterMetric( Could we make the names match the metrics?
https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/crash_pipeline.py (right): https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... appengine/findit/crash/crash_pipeline.py:166: metric.increment({tag_name: tag_value, On 2016/11/22 00:16:30, stgao (slow on Monday) wrote: > What's the tag value? How many different values it could have? For a field in a > metric, the value of the field should be bounded. The monitored tag_value has only True and False. https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... File appengine/findit/crash/monitoring.py (right): https://codereview.chromium.org/2514413003/diff/40001/appengine/findit/crash/... appengine/findit/crash/monitoring.py:7: found_suspects = gae_ts_mon.CounterMetric( On 2016/11/22 00:16:30, stgao (slow on Monday) wrote: > Could we make the names match the metrics? Done.
Description was changed from ========== [Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon BUG= ========== to ========== [Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon BUG=605365 ==========
Looks good, modulo my comment https://codereview.chromium.org/2514413003/diff/60001/appengine/findit/crash/... File appengine/findit/crash/monitoring.py (right): https://codereview.chromium.org/2514413003/diff/60001/appengine/findit/crash/... appengine/findit/crash/monitoring.py:9: description='Predator detection of suspected cls of a crash.') I'm not sure what exactly ts_mon stores, but these descriptions aren't very helpful/descriptive. In general (because I've noticed this elsewhere in the code), rephrasing method/function/field/variable names into sentence format is not proper documentation. Docstrings etc serve a different purpose and are intended for a different audience than variable names are, so just reiterating the variable name is unhelpful and in many cases misleading.
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/2514413003/diff/60001/appengine/findit/crash/... File appengine/findit/crash/monitoring.py (right): https://codereview.chromium.org/2514413003/diff/60001/appengine/findit/crash/... appengine/findit/crash/monitoring.py:9: description='Predator detection of suspected cls of a crash.') On 2016/11/22 19:34:18, wrengr wrote: > I'm not sure what exactly ts_mon stores, but these descriptions aren't very > helpful/descriptive. In general (because I've noticed this elsewhere in the > code), rephrasing method/function/field/variable names into sentence format is > not proper documentation. Docstrings etc serve a different purpose and are > intended for a different audience than variable names are, so just reiterating > the variable name is unhelpful and in many cases misleading. You are right, I changed the description to be more descriptive.
lgtm https://codereview.chromium.org/2514413003/diff/100001/appengine/findit/crash... File appengine/findit/crash/monitoring.py (right): https://codereview.chromium.org/2514413003/diff/100001/appengine/findit/crash... appengine/findit/crash/monitoring.py:10: 'crash analysis of Predator. This metric has fields like:' Much better :) The "crash analysis of Predator" part is still a bit awkward though. I'd rephrase to something like "Metric monitoring whether Predator found CLs for the crash reports". Similarly for the others.
https://codereview.chromium.org/2514413003/diff/100001/appengine/findit/crash... File appengine/findit/crash/monitoring.py (right): https://codereview.chromium.org/2514413003/diff/100001/appengine/findit/crash... appengine/findit/crash/monitoring.py:10: 'crash analysis of Predator. This metric has fields like:' On 2016/11/23 22:04:55, wrengr wrote: > Much better :) > > The "crash analysis of Predator" part is still a bit awkward though. I'd > rephrase to something like "Metric monitoring whether Predator found CLs for the > crash reports". Similarly for the others. My bad documenting... thanks for the advises!
lgtm You may talk to infra team for authentication to push metrics to their service. Detail is in https://chromium.googlesource.com/infra/infra/+/master/appengine_module/gae_t...
On 2016/11/29 18:39:45, stgao (slow on Monday) wrote: > lgtm > > You may talk to infra team for authentication to push metrics to their service. > Detail is in > https://chromium.googlesource.com/infra/infra/+/master/appengine_module/gae_t... Thanks for the instruction, I already asked Sergey for access during the code review and he granted me :)
On 2016/11/29 19:18:57, Sharu Jiang wrote: > On 2016/11/29 18:39:45, stgao (slow on Monday) wrote: > > lgtm > > > > You may talk to infra team for authentication to push metrics to their > service. > > Detail is in > > > https://chromium.googlesource.com/infra/infra/+/master/appengine_module/gae_t... > > Thanks for the instruction, I already asked Sergey for access during the code > review and he granted me :) Nice!
The CQ bit was checked by katesonia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wrengr@chromium.org, stgao@chromium.org Link to the patchset: https://codereview.chromium.org/2514413003/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480453059443910,
"parent_rev": "5e7566542a434b83e64acff65b5739d0114b7eba", "commit_rev":
"55d43e66cf63ee1c9f7101333184a8911b810c76"}
Message was sent while issue was closed.
Description was changed from ========== [Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon BUG=605365 ========== to ========== [Predator] Monitor found_suspects, has_regression_range and found_components using ts_mon BUG=605365 Committed: https://chromium.googlesource.com/infra/infra/+/55d43e66cf63ee1c9f7101333184a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/infra/infra/+/55d43e66cf63ee1c9f7101333184a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
