Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556763002:
swarming: fill the rest of SwarmingTaskEvent proto (Closed)
|
Created:
4 years ago by Paweł Hajdan Jr. Modified:
4 years ago Reviewers:
M-A Ruel CC:
chromium-reviews, infra-reviews+luci-py_chromium.org Target Ref:
refs/heads/master Project:
luci-py Visibility:
Public. |
Descriptionswarming: fill the rest of SwarmingTaskEvent proto
BUG=660014
Committed: https://github.com/luci/luci-py/commit/8e285a8f793c3e98218ea994cad186f946482a7e
Patch Set 1 #
Total comments: 9
Patch Set 2 : fixes #Messages
Total messages: 19 (13 generated)
phajdan.jr@chromium.org changed reviewers: + maruel@chromium.org
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Luci-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/32ed485bdb917410)
lgtm with questions and comments https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... File appengine/swarming/event_mon_metrics.py (right): https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:32: def _files_ref_to_proto(files_ref, proto): You think it is useful information to stream? I don't know, just asking. https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:84: if task_properties.execution_timeout_secs: it is always set; no need for condition https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:86: if task_properties.grace_period_secs: same here, and io_timeout https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:94: event.proto.swarming_task_event.state = state_enum['COMPLETED'].number maybe use a string representation instead?
Thanks for review. https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... File appengine/swarming/event_mon_metrics.py (right): https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:32: def _files_ref_to_proto(files_ref, proto): On 2016/12/06 15:40:09, M-A Ruel wrote: > You think it is useful information to stream? I don't know, just asking. I think we already had that discussion. It's part of the proto, and we fill it in. FWIW, especially outputs could be useful. Inputs too, in case we know some input was misgenerated or something and want to find all affected tasks etc. https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:86: if task_properties.grace_period_secs: On 2016/12/06 15:40:09, M-A Ruel wrote: > same here, and io_timeout I'm getting exceptions if I remove. For _some_ of these vars it might be unnecessary, but why not stay on the safe side? I can add a TODO if we really want to change the code somehow. Caught exception while sending event (/base/data/home/apps/s~chromium-swarm-dev/2515-8294f5d-tainted-phajdan.397589610219954810/event_mon_metrics.py:160) Traceback (most recent call last): File "event_mon_metrics.py", line 157, in send_task_event _task_summary_to_proto(summary, event) File "event_mon_metrics.py", line 87, in _task_summary_to_proto properties_proto.io_timeout_s = task_properties.io_timeout_secs File "gae_ts_mon/protobuf/google/protobuf/internal/python_message.py", line 669, in field_setter new_value = type_checker.CheckValue(new_value) File "gae_ts_mon/protobuf/google/protobuf/internal/type_checkers.py", line 132, in CheckValue raise TypeError(message) TypeError: None has type <type 'NoneType'>, but expected one of: (<type 'int'>, <type 'long'>) https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:94: event.proto.swarming_task_event.state = state_enum['COMPLETED'].number On 2016/12/06 15:40:09, M-A Ruel wrote: > maybe use a string representation instead? Do you have a more specific suggestion? Please note proto uses an enum.
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... File appengine/swarming/event_mon_metrics.py (right): https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:86: if task_properties.grace_period_secs: On 2016/12/07 20:27:56, Paweł Hajdan Jr. wrote: > On 2016/12/06 15:40:09, M-A Ruel wrote: > > same here, and io_timeout > > I'm getting exceptions if I remove. For _some_ of these vars it might be > unnecessary, but why not stay on the safe side? I can add a TODO if we really > want to change the code somehow. > > Caught exception while sending event > (/base/data/home/apps/s~chromium-swarm-dev/2515-8294f5d-tainted-phajdan.397589610219954810/event_mon_metrics.py:160) > Traceback (most recent call last): > File "event_mon_metrics.py", line 157, in send_task_event > _task_summary_to_proto(summary, event) > File "event_mon_metrics.py", line 87, in _task_summary_to_proto > properties_proto.io_timeout_s = task_properties.io_timeout_secs > File "gae_ts_mon/protobuf/google/protobuf/internal/python_message.py", line > 669, in field_setter > new_value = type_checker.CheckValue(new_value) > File "gae_ts_mon/protobuf/google/protobuf/internal/type_checkers.py", line > 132, in CheckValue > raise TypeError(message) > TypeError: None has type <type 'NoneType'>, but expected one of: (<type 'int'>, > <type 'long'>) Humm that's weird. ok. https://codereview.chromium.org/2556763002/diff/1/appengine/swarming/event_mo... appengine/swarming/event_mon_metrics.py:94: event.proto.swarming_task_event.state = state_enum['COMPLETED'].number On 2016/12/07 20:27:56, Paweł Hajdan Jr. wrote: > On 2016/12/06 15:40:09, M-A Ruel wrote: > > maybe use a string representation instead? > > Do you have a more specific suggestion? Please note proto uses an enum. Ah, ignore me then
The CQ bit was checked by phajdan.jr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org Link to the patchset: https://codereview.chromium.org/2556763002/#ps20001 (title: "fixes")
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": 20001, "attempt_start_ts": 1481186482288440,
"parent_rev": "e8a3f4173cd03c372299bf7bfb1625a62d1516cd", "commit_rev":
"8e285a8f793c3e98218ea994cad186f946482a7e"}
Message was sent while issue was closed.
Description was changed from ========== swarming: fill the rest of SwarmingTaskEvent proto BUG=660014 ========== to ========== swarming: fill the rest of SwarmingTaskEvent proto BUG=660014 Committed: https://github.com/luci/luci-py/commit/8e285a8f793c3e98218ea994cad186f946482a7e ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://github.com/luci/luci-py/commit/8e285a8f793c3e98218ea994cad186f946482a7e |
