Skip to content

feat(site): add workspace timings #15068

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

Merged
merged 54 commits into from
Oct 23, 2024
Merged

feat(site): add workspace timings #15068

merged 54 commits into from
Oct 23, 2024

Conversation

BrunoQuaresma
Copy link
Collaborator

@BrunoQuaresma BrunoQuaresma commented Oct 14, 2024

Demo:

Screen.Recording.2024-10-18.at.15.48.28.mp4

@BrunoQuaresma BrunoQuaresma marked this pull request as ready for review October 18, 2024 19:00
@BrunoQuaresma
Copy link
Collaborator Author

Related to coder/internal#44

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

Fantastic work! I'll leave the frontend review to the folks who know best. I'm really excited to see this be added.

Question: when a script occurs, does the error indicator sit to the left of it in the timeline overview view, or is it always aligned to the far left?

<Tooltip
title={
<TooltipTitle>
Script exited with {t.exitCode} code
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
Script exited with {t.exitCode} code
Script exited with code {t.exitCode}

Is there a way to make the exit code more visually distinct? Perhaps bold, with a different colour, monospace font, etc?

Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

This is really cool!!


export const timings = (workspaceId: string) => {
return {
queryKey: ["workspaces", workspaceId, "timings"],
Copy link
Member

Choose a reason for hiding this comment

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

Should this include the build ID or something? I think this means if a workspace gets rebuilt it could cache the same timings because the workspace ID is the same, and I suppose you might have to refresh the page to get the new timings?

Somewhat related, when I create a new workspace I have to refresh or navigate away and back again before the timings load.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch!

scale: number;
};

export const XAxis: FC<XAxisProps> = ({ ticks, scale, ...htmlProps }) => {
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if this component is related, but I found that if I:

  1. Stop a running workspace, wait for it to stop
  2. Click "Workspaces"
  3. Go back into my now-stopped workspace

Then the timings run off edge of the component and I am not able to see them. It seems to fix itself if I navigate away and come back again. Weirdly, the same issue does not appear to happen if I start a stopped workspace then navigate away and back again, it only happens when I stop my workspace.

screenshot

Copy link
Member

Choose a reason for hiding this comment

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

Another thing is if I resize my screen then the timings and legend get cut off, idk if it is worth adding resizing logic but maybe we can just throw in a horizontal scrollbar.

Comment on lines 15 to 17
// The X axis should occupy all available space. If there is extra space,
// increase the column width accordingly. Use a CSS variable to propagate the
// value to the child components.
Copy link
Member

Choose a reason for hiding this comment

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

Not important, but this comment has a bunch of space at the beginning 😄

minWidth: 24,
// Increase hover area
position: "relative",
"&::after": {
Copy link
Member

Choose a reason for hiding this comment

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

Neat trick!

Comment on lines +72 to +73
overflow: "hidden",
textOverflow: "ellipsis",
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a hover, maybe a tooltip, in case someone wants to see the full name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The user can already do it by hovering over the bar.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh I see. My instinct was to hover the name.

const scales = [100, 500, 5_000];

const pickScale = (totalTime: number): number => {
const reversedScales = scales.slice().reverse();
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is the only place where scales is used? We could just define scales already reversed.

return reversedScales[0];
};

export const makeTicks = (time: number) => {
Copy link
Member

@code-asher code-asher Oct 18, 2024

Choose a reason for hiding this comment

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

Do we need to be worried about small screens? These ticks can get really cramped in a small screen. I wonder if we should just set a minimum width and add horizontal scrollbars.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, it should use horizontal scrollbars already, but I will investigate that. We should be concerned about tablet viewports.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Found the issue and it is fixed!

Comment on lines +35 to +37
{Array.from({ length: nOfBlocks }, (_, i) => i + 1).map((n) => (
<div key={n} css={styles.block} style={{ minWidth: blockSize }} />
))}
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully this is the right spot, but one thing that threw me off is that my template has five resources, and I saw five blue blocks, but only three of them show up in the chart when I click into the bar. Edit: I think because we filter Coder resources in the sub-chart but not the top-level chart.

It was also a little surprising that the blue blocks were all even width instead of matching up with their timings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

Comment on lines +20 to +21
* Color scheme for the bar. If not passed the default gray color will be
* used.
Copy link
Member

Choose a reason for hiding this comment

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

I kept thinking the gray box was a checkbox 😆

I wonder if we could change the shape or default color a bit, but then again I might be the only one dumb enough not to realize it was the legend color.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol, @chrifro wdyt?

Copy link

Choose a reason for hiding this comment

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

You are referring to the "state refresh" color right? Screenshot 2024-10-23 at 09 15 19

We could totally change that color. Either to orange if we also want to draw more attention to the "state refresh"
orange-bg: #431407
orange-stroke: #FDBA74 (we use the same color already on different views)

or if we want to keep it more subtle to grey
grey-bg: #18181B
grey-stroke: #71717A

something to keep in mind: the colors will likely slightly change as part of #14780. Then the difference between the newly introduced grey colors and the bg will be more obvious (see screenshot).
Screenshot 2024-10-23 at 09 41 58

Copy link
Member

@code-asher code-asher Oct 29, 2024

Choose a reason for hiding this comment

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

Sorry for the late reply, yes that is what I meant! When there are two or more, it is pretty obvious they are a legend, but with just one I thought it was supposed to be a checkbox, in particular the dark or grey ones. This might just be me being dumb though.

>
{/** We only want to expand stages with more than one resource */}
{t.resources > 1 ? (
<ClickableBar
Copy link
Member

Choose a reason for hiding this comment

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

I am seeing a case where my plan stage is clickable, but there are no timings or rows when I click in. The resources are data.coder_provisioner.me, data.coder_workspace_owner.me, and data.coder_workspace.me.

I think because we are filtering out Coder resources in the resource chart, but not in the top-level chart. Not sure if that means we should also filter them out in the top-level, or if we should include rows for them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@BrunoQuaresma BrunoQuaresma requested review from code-asher, dannykopping, a team and jaaydenh and removed request for code-asher and a team October 21, 2024 20:11
Copy link

@chrifro chrifro left a comment

Choose a reason for hiding this comment

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

Looks great @BrunoQuaresma 👏 ✨

One comment non-blocking:

  • what do you think of adjusting the height of the whole drop-down to fit the content? Currently, there is a lot more bottom spacing on the initial view vs the different stages (see screenshots).
    Consequences: either the box height gets smaller for the stage views as well or the box height changes for the stage views. Thoughts?
Screenshot 2024-10-23 at 09 43 28 Screenshot 2024-10-23 at 09 53 24

@BrunoQuaresma
Copy link
Collaborator Author

@chrifro Hm... I see what you’re trying to achieve, but I usually prefer to have elements with dynamic data and unpredictable sizes set to a fixed height to avoid layout shifting. In my opinion, we could release the component as it is and see if we receive any feedback from users on this. Does that sound like a good plan?

@BrunoQuaresma BrunoQuaresma merged commit d89eceb into main Oct 23, 2024
27 checks passed
@BrunoQuaresma BrunoQuaresma deleted the bq/workspace-timings-ui branch October 23, 2024 13:09
@github-actions github-actions bot locked and limited conversation to collaborators Oct 23, 2024
) : (
<KeyboardArrowDown css={{ fontSize: 16, marginRight: 16 }} />
)}
<span>Provisioning time</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this is not just provisioning time since it also includes agent timings.
We should use a more descriptive name, like "Build timeline".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants