Skip to content
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

Fix all Age, LastSeen, and FirstSeen displays #4990

Merged
merged 3 commits into from
Mar 17, 2022
Merged

Fix all Age, LastSeen, and FirstSeen displays #4990

merged 3 commits into from
Mar 17, 2022

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Mar 10, 2022

  • Sorting based on non-changing values without using Date.now()

  • Displaying base on "mobx-utils".now() with some optimization for
    updates of very long durations

  • Deprecating KubeObject.getTimeDiffFromNow and KubeObject.getAge
    methods as they are not reactive

  • Export ReactiveDuration to the extension API.

Signed-off-by: Sebastian Malton sebastian@malton.name

- Sorting based on non-changing values without using Date.now()

- Displaying base on "mobx-utils".now() with some optimization for
  updates of very long durations

- Deprecating KubeObject.getTimeDiffFromNow and KubeObject.getAge
  methods as they are not reactive

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 added bug Something isn't working area/ui labels Mar 10, 2022
@Nokel81 Nokel81 added this to the 5.4.2 milestone Mar 10, 2022
@Nokel81 Nokel81 requested a review from a team as a code owner March 10, 2022 17:04
@Nokel81 Nokel81 requested review from jweak and DmitriyNoa and removed request for a team March 10, 2022 17:04
@jim-docker jim-docker modified the milestones: 5.4.2, 5.4.3 Mar 15, 2022
@jim-docker
Copy link
Contributor

Looks like it's too reactive now:

Screen.Recording.2022-03-15.at.3.04.04.PM.mov

Also noticed Last seen time for newly created pod of 52y, but might be an old bug.

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 15, 2022

newly created pod of 52y

Sounds like the unix epoch

Looks like it's too reactive now:

Do you think it should be slower then? Because those updates are all synchronized (namely they all react against the same observable clock instance)

@jim-docker
Copy link
Contributor

newly created pod of 52y

Sounds like the unix epoch

Looks like it's too reactive now:

Do you think it should be slower then? Because those updates are all synchronized (namely they all react against the same observable clock instance)

Updating the item list every second seems too much(?) I guess if it's not making kube api calls every update it's good.

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 15, 2022

Updating the item list every second seems too much(?) I guess if it's not making kube api calls every update it's good.

For something like age I think updating every second (while seconds are displayed) and then only updating every minute is fine.

And yes this is a completely local calculation. It is based on the metadata.creationTimestamp field which should never change.

jim-docker
jim-docker previously approved these changes Mar 15, 2022
Copy link
Contributor

@jim-docker jim-docker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.
The sort direction for Age seems backwards to me:
Screen Shot 2022-03-15 at 6 12 56 PM
Shouldn't the values go from small to large reading down the list for this indicator?

<>
<p>Failed to load {crd.getPluralName()}</p>
{!version.served && (
<p>Prefered version ({crd.getGroup()}/{version.name}) is not served</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p>Prefered version ({crd.getGroup()}/{version.name}) is not served</p>
<p>Preferred version ({crd.getGroup()}/{version.name}) is not served</p>

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 15, 2022

Shouldn't the values go from small to large reading down the list for this indicator?

I guess it depends on what you think the default sorting means. I took it as the chevron points in the direction of sort from smallest to largest.

So an up chevron would mean that the largest age is near the top of the table.


return (
<>
{formatDuration(now(computeUpdateInterval(creationTimestamp)) - creationTimestamp, compact)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we need to tweak formatDuration (or use other formatter) now that we update/compute the duration. Updating things like 2m3s every second will be annoying. My suggestion is to drop seconds completely when over 1m and then update duration only once per minute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized that current formatDuration mimics kubectl output. Maybe it's ok to keep using that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can tweak it if you like...

I could slow down the updates to every 5s if you think it is too fast, and thus distracting.

jakolehm
jakolehm previously approved these changes Mar 16, 2022
Copy link
Contributor

@jakolehm jakolehm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if updating age/duration every second for 10 minutes is a good idea but we can tweak it after we get some real usage.

@jim-docker
Copy link
Contributor

Shouldn't the values go from small to large reading down the list for this indicator?

I guess it depends on what you think the default sorting means. I took it as the chevron points in the direction of sort from smallest to largest.

So an up chevron would mean that the largest age is near the top of the table.

I agree with how you interpret what it should mean but I don't think that is consistent with other fields. For names the chevron pointing up has the "smallest" name at the top.

I assumed how it works is chevron up means ascending, and since the natural direction for a list is downward, then ascending means the values go up as you read the list (down). But I agree it seems more natural for the chevron to point in the direction of the ordering in the list.

Anyway I think what you have here for age is inconsistent with other columns (like restarts on pods)

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 16, 2022

Anyway I think what you have here for age is inconsistent with other columns (like restarts on pods)

That is very convincing, we should be consistent with out UI. Will change.

Nokel81 added 2 commits March 16, 2022 08:47
Signed-off-by: Sebastian Malton <sebastian@malton.name>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 dismissed stale reviews from jakolehm and jim-docker via 4716e83 March 16, 2022 12:47
@Nokel81 Nokel81 requested review from jim-docker and jakolehm March 16, 2022 12:47
Copy link
Contributor

@jim-docker jim-docker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still want it for 5.4.3? I guess we can decide when we go to release 5.4.3. I think it should be 5.5, since it changes behaviour (updates every second), which may be a bit surprising to see in a patch.

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 16, 2022

There seems to be more consensus for it being 5.5 so let's do that.

@Nokel81 Nokel81 modified the milestones: 5.4.3, 5.5.0 Mar 16, 2022
@Nokel81 Nokel81 merged commit b08daa8 into master Mar 17, 2022
@Nokel81 Nokel81 deleted the issue-4901 branch March 17, 2022 19:17
@Nokel81 Nokel81 mentioned this pull request May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Resource age sort not working Sorting pod disruption budgets by Age use alphabetic sort
3 participants