workflow-execution-service-schemas icon indicating copy to clipboard operation
workflow-execution-service-schemas copied to clipboard

Proposal: Add timestamps to RunStatus

Open patmagee opened this issue 4 years ago • 4 comments

Currently the RunStatus Object is quite minimal, comprised only of two keys: state and run_id. As a user, this provides me with very little context of the runs and makes creating UI's or CLI's awkward and hard to read.

I propose we add two keys start_time and end_time to the RunStatus. This information is readily available already as part of the RunLog and should not pose a significant challenge for implementors to retrieve. While this does not provide complete context to the user it is a vast improvement over what we currently have and at least they will be able to see:

  1. When a run was submitted
  2. How long a run has been executing for
  3. Provide an easy way of sorting runs by start or end time.
{
  "run_id":"ff07e7a8-4da5-4e4f-b6a7-164cf4d6b45e",
  "status": "RUNNING",
  "start_time": "2021-01-07T00:00:000Z",
  "end_time":  "2021-01-07T07:21:000Z"
}

patmagee avatar Jan 07 '21 14:01 patmagee

I think this is a great addition -- and I'm guessing this is info that is already passed onto many users -- possibly through the same endpoint or another one. Adding in some reviewers and @aniewielska I'd love your take from doing something similar for TES. This feels like a great candidate for a PR and discussion

@wshands @jaeddy @aniewielska

ruchim avatar Jan 26 '21 13:01 ruchim

@ruchim is there anything I should do to further this discussion? I am more then happy to make a PR to update the specification

patmagee avatar Jan 27 '21 14:01 patmagee

Do we know if RunStatus and RunLog appeared somewhere during WES evolution or was this distinction present in WES from the very beginning? TES is different in that it offers a parameter view: MINIMAL, BASIC, FULL and with it the requestor decides on the level of detail visible both in the list and in the get one task payload. The spec implements all levels of detail by just one common type tesTask. So the contract of what is present in which level of visibility is not well expressed in the spec, but the advantage is that MINIMAL and BASIC have the same properties under exactly the same paths as FULL. At the moment MINIMAL does not contain times, either.

In the solution proposed for WES, I like the idea of returning start and end time in the RunStatus, especially for the list (having played with WES implementations I felt a bit lost without that information). I am worried, however, that RunStatus is starting to diverge from RunLog.

The equivalent of proposed RunStatus information looks like that for RunLog with start_time and end_time being encapsulated in run_log:

{
  "run_id": "string",
 "state": "UNKNOWN",
 "run_log": {
    "start_time": "string",
    "end_time": "string",
 }
}

I would like to see consistency between those 2 types.

aniewielska avatar Feb 01 '21 21:02 aniewielska

@aniewielska thanks for the input. I would push back on nesting them under the run-log field. For a few reasons.

Firstly, The RunStatus object is not synonymous with the Log object. The latter contains information on how to retrieve Stderr and stdout if relevant, and a whole slew of other properties. Unless you are proposing we also include those properties, I would argue that we are already breaking convention by leaving them blank.

The motivation here is to provide a bit more context that will allow api consumers (and implementors) the ability to easily filter, order and understand the content that is being returned. To that end, I dont believe introducing a two level hierarchy for just the start and end time fully accomplishes this goal. Operations such as filtering and ordering are now more challenging (albeit only slightly).

As an example, if I were to implement a cli, I would always flatten the start and end time into the parent object when displaying the results of a list operation. If this is the way users will interact with it, I dont see why we should make it harder for them.

While embedding the star and end time at the root object diverges from the log, I think the use case is different enough that it is warranted.

patmagee avatar Feb 02 '21 02:02 patmagee