-
Notifications
You must be signed in to change notification settings - Fork 2k
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
UI: Showed host reservations and allocation utilization by task on existing stats charts #10208
Conversation
Ember Asset Size actionAs of c15bf65 Files that got Bigger 🚨:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
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.
agreed re the desirability of having an overly-general shared component, breaking it up for the variations makes sense to me.
I found a significant problem in that tooltips are broken for me on the allocation chart, but apart from a few tiny notes, this looks good 😀
<span class="value">{{datum.formatttedY}}</span> | ||
{{/if}} | ||
</li> | ||
</c.Tooltip> |
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.
Hmm. Looks like this happens for allocs with lifecycle tasks that haven't started yet. Is that when you encountered it too?
} | ||
|
||
get reservedAmount() { | ||
return this.metric === 'cpu' ? this.tracker.reservedCPU : this.tracker.reservedMemory; |
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.
Maybe it’s a fallback that never actually happens but it gives me pause to see a this.metric
-based conditional here with only two branches while the two following ones have three 🤔
await preload(this.store); | ||
|
||
const resource = findResource(this.store); | ||
console.log(resource, this.store.peekAll('allocation').get('firstObject').states); |
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.
Forgotten?
|
||
/** Args | ||
node = null; | ||
metric null; (one of 'cpu' or 'memory' |
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.
tiny: missing )
(repeated elsewhere)
Currently, PrimaryMetric is already overloaded on multiple dimensions: metric and resource type. This refactor will use multiple components as a form of control flow instead spidering conditionals, which are only getting worse as the charts for each resource type diverge.
This binds a function to a target before passing it along to another component. It's normal to expect to get to use `this` within functions on components and controllers, but (sans actions) that doesn't happen automatically.
This leverages the existing pre-processing being done in the allocation-stats-tracker to also create additive percentages relative to the allocation resources vs. the task resources. This can then be used in a chart to create a stacked area representation of consumption.
- Sorting must be done on copies to preserve orders. - Indices should be reversed since rendering is also reversed (the back layer (the tallest) is rendered first to create the stacking effect).
…tion In addition to this computation being wasteful, it introduces a bug where the allocation on a stats tracker can update twice in one render, which isn't allowed in Glimmer (ironically, Glimmmer's lack of auto-memoization introduced the issue).
bf4bccc
to
d9426df
Compare
Okay! I fixed the tooltip bug you caught and another bug with sorting tasks by their lifecycle phase. |
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.
nice, indeed fixed for me 🥳
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This applies the new chart patterns to add more information to the existing memory and CPU utilization charts on the client detail, allocation detail, and task detail pages.
Client Detail
The client detail charts will now show a horizontal annotation (or "high water mark") denoting the reserved capacity for the metric if one has been set.
Allocation Detail
The allocation detail charts will now break down utilization by task for allocations with multiple tasks.
Task Detail
The task detail is unchanged. It shows the same data as the allocation detail charts but narrowed down to the single task.
Implementation
Since each instance of the previous
PrimaryMetric
component has now deviated, it made sense to refactor it into three components, one for each variation. This resulted in some repetition, but that seemed better than adding even more conditionals and overloading the existing implementation.There are also some details that arose from using the
StatsTrackers
with Glimmer components.