-
Notifications
You must be signed in to change notification settings - Fork 69
Refine stage progress details #1033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Previously, the run polling updated the progress every 3s. Well, it used to every 3s before debouncing it. After debouncing it, the progress was only updated on changes to the run, which is likely only happening when the current stage transitions from "running" to "success" or "failure".
- totalDurationMillis is unset for "running" stages. The merging of the previous stage details resulted in picking up the total duration of the stage in the previous run. With the total taking precedent in the rendering of "LiveTotal", it current stage duration was never displayed. Fix: Do not use the total from the previous run. - With the above fixed, another bug was surfaced: startTimeMillis is picked up from previous runs as well. The steps polling updates every second compared to the run/stages polling every three seconds. This allows the steps polling to surface steps from stages that are not yet refreshed and use skeleton stages from the previous run. With startTimeMillis coming from these skeleton stages, the "Started x ago" display was off for up-to 3s. It showed the time difference for the stage in the previous run. Fix: For skeleton stages, use the start of the first step. - Without a total or start timestamp, the time durations were not displayed. Their icons were rendered regardless. Fix: Only show the icons when either is set. - Very specific durations were displayed, like 0.42s or 9.99s. That seems too specific to me. The sub-second part seems distracting. When stages are "running", the durations are rounded to seconds. When stages are done, the time is updated once to a specific value. The "Started x ago" display is updated every minute thereafter. E.g. total duration: 1s -> 2s -> 3s -> [done] -> 3.5s E.g. started x ago: 1s -> 2s -> 3s -> [done] -> 4s -> 1m -> 2min ...
| const ms = Math.max(1_000, Date.now() - since); | ||
| // "9.99s ago" is not useful either. By the time you read it, it is already "10s ago". | ||
| // Round everything to the next second. | ||
| setDuration(1_000 * Math.round(ms / 1_000)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about instead of rounding it using <1s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see also #942
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rounding needs to stay. I can remove the max(1s, now-since) and let the rendering act on duration<1_000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 86ed4d3
| const resolution = live ? 1_000 : 60_000; | ||
| const update = () => { | ||
| // Round with 1s/1min precision. | ||
| setDuration(resolution * Math.round((Date.now() - since) / resolution)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did you see this, on the right side of the video? That would be from the UI on the main branch. The left side (this PR) would round it and not render .0s for a "running"/blue stage.
I can see a "success"/green variant with 8.0s. I am not rounding after completion. The LiveTotal renders a Total instead: https://github.com/das7pad/jenkins-pipeline-graph-view-plugin/blob/e2f4d6511024ff903198fa6fdc83bb1c563eccab/src/main/frontend/common/utils/live-total.tsx#L10-L11
Either way, it looks like an issue with the rendering of times via humanise, maybe it does not truncate trailing zeros after rounding?
| function humanise(duration: number, locale: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 667f2da
|
(Love the demo comparison video ❤️) |
| // 1712ms -> 1.7sec | ||
| expect(getTotal(1_712).getByText("1.7s")).toBeInTheDocument(); | ||
| // 1100ms -> 1.1sec | ||
| expect(getTotal(1_100).getByText("1.1s")).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need decimal seconds at all? blue ocean doesn't render them and neither does github actions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decimals are only shown for durations under 10 seconds. I personally prefer keeping them.
pipeline-graph-view-plugin/src/main/frontend/common/utils/timings.tsx
Lines 72 to 77 in 86ed4d3
| } else if (seconds >= 10) { | |
| durationParts["seconds"] = seconds; | |
| } else if (seconds >= 1) { | |
| durationParts["seconds"] = seconds; | |
| if (millis >= 100) { | |
| durationParts["milliseconds"] = millis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed a couple of pipelines at work and I think it will actually be fine without the decimals. Do you want to round, ceil or floor?
What do others prefer? cc @janfaracik @lewisbirks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think floor but ok with any
|
|
||
| useEffect(() => { | ||
| // Update every second while in progress. Update every minute when done. | ||
| const resolution = live ? 1_000 : 60_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is updating too often.
There's a bug in this implementation where the timer goes above the actual time and it jumps down.
time-jump.mov
See right at the end it jumps from 4 down to 3.2.
In another run I had it go from 4 to 2.6.
I think we saw similar when this was initially implemented and decreasing the update interval was enough to smooth it out,
cc @lewisbirks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember rightly we made the update of something, probably the live time update, just out of sync with the polling to prevent that. It was like the poll was every 3s and then the timer was updated every 3.001s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the computed duration based on the stage/step startTimeMillis switching to the final stage/step totalDurationMillis after the next stage/step polling result arrives.
We could let the stage times tail behind by 2s (run/stage polling interval = 3s vs step polling interval = 1s vs ui update interval = 1s), but that might look strange as Started x ago is not in sync with the total duration and the sum of all steps does not equal the stage total (obvious when there is only one step). Updating the times in the UI every 3s seemed quite strange to me.
I wonder if we can increase the run/stage polling interval to 1s. Perhaps we can do so after #997 has optimized the network traffic for "nothing changed since last poll"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably increase the polling a bit if necessary it was just set where it was to reduce the cost of calculating it so often.

This PR is refining the rendering of stage progress details: total duration, started x ago, icon animation.
Update stage progress in StatusIcon every second
Previously, the run polling updated the progress every 3s.
Well, it used to every 3s before debouncing it (#993). After debouncing it, the
progress was only updated on changes to the run, which is likely only
happening when the current stage transitions from "running" to "success"
or "failure".
This PR is fixing the regression and improving the update cycle from 3s to 1s.
Update stage durations every second while running and every minute after
previous stage details resulted in picking up the total duration of
the stage in the previous run. With the total taking precedent in the
rendering of "LiveTotal", it current stage duration was never
displayed.
Fix: Do not use the total from the previous run.
picked up from previous runs as well. The steps polling updates every
second compared to the run/stages polling every three seconds.
This allows the steps polling to surface steps from stages that are
not yet refreshed and use skeleton stages from the previous run.
With startTimeMillis coming from these skeleton stages, the
"Started x ago" display was off for up-to 3s. It showed the time
difference for the stage in the previous run.
Fix: For skeleton stages, use the start of the first step.
displayed. Their icons were rendered regardless.
Fix: Only show the icons when either is set.
That seems too specific to me. The sub-second part seems distracting.
When stages are "running", the durations are rounded to seconds.
When stages are done, the time is updated once to a specific value.
The "Started x ago" display is updated every minute thereafter.
E.g. total duration: 1s -> 2s -> 3s -> [done] -> 3.5s
E.g. started x ago: 1s -> 2s -> 3s -> [done] -> 4s -> 1m -> 2min ...
Testing done
I've added unit tests for the new rendering.
jenkins-stage-progress-details.mp4
Left: This PR; right: HEAD at 6143c97
Side note: The logs are not properly tailed. I'll fixed that shortly with a reworked tailing state as part of #909.
Submitter checklist