Cpu and memory sparklines #387

joeatwork · 2016-02-17T01:07:09Z

The changes proposed here depend on the new API information in #386 - comment on this pull request can probably wait until those changes are revised and accepted or declined.

This pull request proposes adding a sparkline-like visualization of the resource usage history for individual pods in a replication controller detail screen, fixing #366

joeatwork · 2016-02-17T01:07:55Z

There are a few product questions associated with this feature, in addition to issues with the implementation -

Right now, the scale of each chart is independent of the others - the maximum point in history will always be at the top of the chart, and even if the chart is showing only one data point the line will cross the whole chart. It's possible that these charts should all share a common scale, either on a single page or in general - if so, please comment here.
As proposed here, the charts are always rendered, no matter how wide or narrow the window is, and are never wrapped below the text measures. Should they disappear or wrap at some point?

bryk · 2016-02-17T16:49:56Z

Just tested. The UI always shows graphs in the same shape. Did you have this problem? Tested on gulp serve and in-cluster heapster.

bryk · 2016-02-17T16:50:42Z

bryk · 2016-02-17T16:51:14Z

I'll investigate this tomorrow.

joeatwork · 2016-02-17T17:22:29Z

I see those shapes for very young processes - it looks like your CPU only has two data points, and your memory history only has three. I'd love to see what you're getting back from the API. It's possible that our Heapsters are configured differently (and if so, I should probably account for that before showing the charts)

bryk · 2016-02-18T15:07:38Z

@floreks Can you help with review?

bryk · 2016-02-18T15:08:51Z

src/app/frontend/common/components/sparkline/sparkline.html

@@ -0,0 +1,8 @@
+<svg viewBox="0,0 1,1"


Nit: please add the Copyright ... boilerplate to html files. See how it is done in others.

No worries, it is stripped and minified during build process.

Fixed in 3062264

bryk · 2016-02-18T16:05:36Z

This is awesome @joeatwork :) I'm really happy with the shape of this PR. A few comments, PTAL

joeatwork · 2016-02-18T18:40:38Z

Thanks for all of your help! I believe I've addressed your comments - please take another look when you get the time.

floreks · 2016-02-19T08:19:27Z

@floreks Can you help with review?

I wanted to this today morning but I see you've done it all :) I'll take a look anyway.

floreks · 2016-02-19T09:05:09Z

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html

                <span ng-if="::ctrl.hasCpuUsage(pod)">
                  {{::(pod.metrics.cpuUsage | kdCores)}}
                </span>
-                <span ng-if="::!ctrl.hasCpuUsage(pod)">
-                  -
-                </span>


Without that when there are no metrics to display columns are empty.

This doesn't look very good. Sometimes we have to wait a bit for metrics to appear so i guess this is needed.

fixed in 3074c8f and 937893d

bryk · 2016-02-19T13:54:20Z

Reviewed 7 of 12 files at r1, 5 of 5 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 29 [r2] (raw file):
You dont need this input param anymore.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 30 [r2] (raw file):
Is timeseries optional? What will happen if this is not defined? I think the Javascript will fail. Mark this as non-optional?

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 158 [r2] (raw file):
Hmm... How about we put this in the same column as current usage? So that the sparkline wraps as needed. The reason is that the table is already pretty wide and forcing it to be even wider worries me.

@floreks ?

Comments from the review on Reviewable.io

bryk · 2016-02-19T14:03:30Z

(bear in mind that I'm testing reviewable.io, so might miss something)

floreks · 2016-02-19T14:29:18Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 30 [r2] (raw file):
Do we actually need 2 way binding here? Is '@' not sufficient?

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 158 [r2] (raw file):
I agree. I'm also worried how introducing additional column would affect row to table mapping on smaller screens. Same column for both sounds good.

Comments from the review on Reviewable.io

bryk · 2016-02-19T15:09:04Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 30 [r2] (raw file):
Timeseries is an object. You usually dont want to bind objects with @ which converts everything to a string.

Comments from the review on Reviewable.io

bryk · 2016-02-19T15:09:45Z

Btw, there's also a gulp format-javascript task.

joeatwork · 2016-02-19T21:07:33Z

Review status: 4 of 9 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 29 [r2] (raw file):
Fixed in 0ae5a0a

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 30 [r2] (raw file):
I've made timeseries non-optional in 205a215 - for now, I'm leaving the '=' binding, if that's ok.

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 158 [r2] (raw file):
We should probably explicitly wrap the column at certain widths with media-queries. Otherwise it will be really tough to get a column that looks like the mock - the mock right now is right justified, but the cell shrinks to so that the left edge of the longest cell in the column lines up with the header (I've included an image with what I think is the desired behavior - the content of the widest cell lines up on the left with the header, but all of the other cells are right-aligned, so their charts line up)

Getting this effect requires that the cells be as narrow as they can be, or the left edge of the longest content will wander off and leave the header looking strange.

A fix (and the only one I can think of) is to make the sparkline cells as narrow as possible. Unfortunately, this means they'll always wrap, since a wrapped cell is narrower than an unwrapped cell. We could change this to prevent the wrap, but this will (in the absence of media queries) prevent wrapping in all cases, which doesn't get us anywhere.

Is there an example of media-querying, or responding in layout to size, elsewhere in the code? It seems like the natural thing to do might be to explicitly hide or wrap the charts when the viewport is narrower than some threshold, rather than trying to get the natural text wrapping to work. Alternately, we could just left-justify the content, or compromise on the mock, if that seems appropriate. Do you guys have any thoughts about this?

Comments from the review on Reviewable.io

floreks · 2016-02-22T11:06:28Z

Review status: 4 of 9 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 158 [r2] (raw file):
Display of CPU usage units should be changed soon to more user friendly format (2.5 CPU). This way there won't be so big numbers used and we can style it better. For memory I think we can also round it to 1 decimal place.

Still for display we can use 1 table column but wrap it in 2 divs or something and align text to the left. I'd love to help with styling that but for now I've problem with heapster on my local cluster and can't make him produce any metrics.

Comments from the review on Reviewable.io

bryk · 2016-02-22T14:47:48Z

Reviewed 4 of 5 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 30 [r2] (raw file):
Nice, thanks!

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 158 [r2] (raw file):
We have media queries examples in the code, e.g., ./app/frontend/chrome/chrome.scss:89:@media only screen and (max-width: $layout-breakpoint-xs) {. There's even media queries in JS code using $mdMedia service. See _variables.scss file for breakpoints or take a breakpoint from https://github.com/angular/material/blob/HEAD/src/core/style/variables.scss.

I say: go for media queries. Display underneath and next to depending on the size. Please verify that this works from 1920px width to phone size :)

Comments from the review on Reviewable.io

bryk · 2016-02-22T14:51:54Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

a discussion (no related file):
Btw, this PR needs rebase.

Comments from the review on Reviewable.io

joeatwork · 2016-02-24T23:38:52Z

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.scss

+.kd-sparkline {
+  background-color: $body;
+  height: 2.5 * $baseline-grid;
+  vertical-align: -$baseline-grid / 2; // 20% below baseline, to flow with text


@bryk - I've added the vertical-align back in. Without it, the graph visually looks too high, like it's a step up from the label text. If I change the enclosing line-height to zero, it moves the graph but also the label text, so the graph still looks higher than the mock. (I might not understand your line-height suggestion) - please let me know if this is ok.

joeatwork · 2016-02-24T23:39:12Z

I've changed the look of empty fields (That is, when you are expecting metrics from the server but get an empty list of metrics) to match the other fields, displaying a left-aligned dash in that case:

Review status: 5 of 9 files reviewed at latest revision, 7 unresolved discussions.

src/app/frontend/common/components/sparkline/sparkline_controller.js, line 23 [r2] (raw file):
Done.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 29 [r2] (raw file):
Done.

src/app/frontend/common/components/sparkline/sparkline_directive.js, line 30 [r2] (raw file):
Done.

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 158 [r2] (raw file):
I've added media queries - I've also moved the breaking point that hides the sidebar from gt-sm to gt-md, since the new columns expand the width of the table. I've made some choices about the padding, spacing of the sparkline that are probably worth taking a look at- here are some screenshots of the different sized layouts after the proposed patch:

"Natural" XL width:

Middle width - no sidebar, wrapping sparkline. I've padded the top and bottom of the cell in this case by $baseline-grid

Standard "phone" width - there is enough room for the sparkline to display next to the label for the most part

Most narrow - the sparkline wraps naturally (the sparkline will also wrap if the label is very long, which seems reasonable)

Comments from the review on Reviewable.io

joeatwork · 2016-02-24T23:49:49Z

@floreks, @bryk I think I've addressed your concerns with wrapping and single-table-cell layouts in the recent commits - I haven't changed the formats or metrics for the CPU or Memory, but those can be changed globally whenever you'd like since I'm using the same filters to format the values that are used elsewhere. Please take another look and let me know if these changes work for you! The biggest things to consider (and maybe ask me to change) are

I've changed the breakpoint that makes the left column disappear from sm to md, since the new columns were breaking out of the viewport much earlier
I've added a baseline adjustment back to the sparkline (this time in terms of $baseline-grid) because otherwise the baseline of the text and the bottom of the chart want to line up in a funny looking way
I've added some padding and spacing at various viewport widths that isn't in any mock, which means I may be doing goofy un-designerly things. You can see the results in the screenshots attached to the PRQ.

Thanks again for all of your help with this!

codecov-io · 2016-02-25T00:27:38Z

Current coverage is `80.28%`

Merging #387 into master will decrease coverage by -1.24% as of 4071cc6

@@            master    #387   diff @@
======================================
  Files           76      78     +2
  Stmts          617     629    +12
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            503     505     +2
  Partial          0       0       
- Missed         114     124    +10

Review entire Coverage Diff as of 4071cc6

Powered by Codecov. Updated on successful CI builds.

bryk · 2016-02-25T09:53:14Z

Nice! My last comments about style and some tests and let's merge. I cant wait to have this on HEAD

Reviewed 2 of 4 files at r5.
Review status: 7 of 9 files reviewed at latest revision, 5 unresolved discussions.

src/app/frontend/common/components/sparkline/sparkline_controller.js, line 18 [r5] (raw file):
Btw, classes start should with Capital Letter.

src/app/frontend/common/components/sparkline/sparkline_controller.js, line 23 [r5] (raw file):
Can you test this class?

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 156 [r5] (raw file):
Can you move this and the other {{(ctrl.mdMedia('gt-xs') || undefined) && 1}} to a controller's method? We don't want to be putting logic into templates, just method calls and field reads. In controller, we'll be able also to test this.

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.scss, line 129 [r5] (raw file):
Sounds good.

Comments from the review on Reviewable.io

joeatwork · 2016-02-25T23:51:31Z

Review status: 5 of 10 files reviewed at latest revision, 6 unresolved discussions.

a discussion (no related file):
Rebased against current master. Thanks!

src/app/frontend/common/components/sparkline/sparkline_controller.js, line 18 [r5] (raw file):
Fixed in 7ed2a98

src/app/frontend/common/components/sparkline/sparkline_controller.js, line 23 [r5] (raw file):
Added tests in 1427bef

src/app/frontend/replicationcontrollerdetail/replicationcontrollerdetail.html, line 156 [r5] (raw file):
Fixed in 9d99690

Comments from the review on Reviewable.io

joeatwork · 2016-02-25T23:52:32Z

Thanks for your continuing help with this! I think your last comments have been addressed.

cheld · 2016-02-29T10:15:01Z

@joeatwork

The code seems to have some formatting issues. You must run gulp format-javascript before committing.
Normally we squash our commits

@bryk I also see only three data points when using local cluster

bryk · 2016-02-29T12:44:55Z

@cheld Update your heapster. 0.18.x version does not quite work.

@joeatwork squash and gulp format-javascript and we're ready to merge :)

Reviewed 1 of 4 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io

joeatwork · 2016-02-29T16:56:08Z

Linted and squashed!

bryk · 2016-02-29T17:10:00Z

Awesome! Merging. If you need any help from us, just ping.

Reviewed 2 of 2 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

Cpu and memory sparklines

joeatwork · 2016-02-29T23:44:47Z

Thanks, folks!

cheld · 2016-03-01T14:48:16Z

I played around with heapster 0.19.0 and the parameters -stats_resolution -model_resolution -model_frequency.

I experimented a lot, but I was not able to create pretty charts. Can you suggest proper settings?

bryk · 2016-03-01T14:58:21Z

@joeatwork What is your heapster setup?

cheld · 2016-03-01T15:30:04Z

ok, I tried 2.0-alpha. This seems to work much better. Unfortunately, it crashes if one changes the default flags. However, even with the default values, this version produce something that is acceptable.

bryk · 2016-03-01T15:33:00Z

ok, I tried [heapster] 2.0-alpha. This seems to work much better. Unfortunately, it crashes if one changes the default flags. However, even with the default values, this version produce something that is acceptable.

CC @mwielgus

cheld · 2016-03-01T18:09:43Z

@mwielgus

Unfortunately, the crashes are not only related to flags. The version also crashes randomly. I created an issue.

joeatwork · 2016-03-01T18:16:51Z

I've been running with gcr.io/google_containers/heapster:v0.20.0-alpha5 - here's my heapster replication controller in detail:

apiVersion: v1
kind: ReplicationController
metadata: 
  labels: 
    k8s-app: heapster
    kubernetes.io/cluster-service: "true"
    version: v10
  name: heapster-v10
  namespace: kube-system
spec: 
  replicas: 1
  selector: 
    k8s-app: heapster
    version: v10
  template: 
    metadata: 
      creationTimestamp: null
      labels: 
        k8s-app: heapster
        kubernetes.io/cluster-service: "true"
        version: v10
    spec: 
      containers: 
        - command: 
            - /heapster
            - "--source=kubernetes:https://kubernetes.default"
            - "--sink=influxdb:http://monitoring-influxdb:8086"
          image: gcr.io/google_containers/heapster:v0.20.0-alpha5
          imagePullPolicy: IfNotPresent
          name: heapster
          resources: 
            limits: 
              cpu: 100m
              memory: 224Mi
            requests: 
              cpu: 100m
              memory: 224Mi
          terminationMessagePath: /dev/termination-log
      dnsPolicy: ClusterFirst
      restartPolicy: Always
      terminationGracePeriodSeconds: 30

Bumps [tree-kill](https://github.com/pkrumins/node-tree-kill) from 1.2.0 to 1.2.2. - [Release notes](https://github.com/pkrumins/node-tree-kill/releases) - [Commits](pkrumins/node-tree-kill@v1.2.0...v1.2.2) Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Anvith KS <[email protected]>

googlebot added the cla: yes label Feb 17, 2016

bryk reviewed Feb 18, 2016
View reviewed changes

joeatwork force-pushed the cpu-and-memory-sparklines branch from 585c090 to d94f58c Compare February 18, 2016 18:29

floreks reviewed Feb 19, 2016
View reviewed changes

joeatwork force-pushed the cpu-and-memory-sparklines branch from 0ae5a0a to 205a215 Compare February 19, 2016 18:10

joeatwork force-pushed the cpu-and-memory-sparklines branch from 205a215 to 4844eca Compare February 24, 2016 22:16

joeatwork reviewed Feb 24, 2016
View reviewed changes

show sparkline-like charts next to resource metrics

7ef80f3

joeatwork force-pushed the cpu-and-memory-sparklines branch from 9d99690 to 7ef80f3 Compare February 29, 2016 16:55

bryk added a commit that referenced this pull request Feb 29, 2016

Merge pull request #387 from joeatwork/cpu-and-memory-sparklines

13e88ce

Cpu and memory sparklines

bryk merged commit 13e88ce into kubernetes:master Feb 29, 2016

joeatwork deleted the cpu-and-memory-sparklines branch February 29, 2016 23:44

Cpu and memory sparklines #387

Cpu and memory sparklines #387

Conversation

joeatwork commented Feb 17, 2016

joeatwork commented Feb 17, 2016

bryk commented Feb 17, 2016

bryk commented Feb 17, 2016

bryk commented Feb 17, 2016

joeatwork commented Feb 17, 2016

bryk commented Feb 18, 2016

bryk Feb 18, 2016

Choose a reason for hiding this comment

joeatwork Feb 18, 2016

Choose a reason for hiding this comment

bryk commented Feb 18, 2016

joeatwork commented Feb 18, 2016

floreks commented Feb 19, 2016

floreks Feb 19, 2016

Choose a reason for hiding this comment

joeatwork Feb 24, 2016

Choose a reason for hiding this comment

bryk commented Feb 19, 2016

bryk commented Feb 19, 2016

floreks commented Feb 19, 2016

bryk commented Feb 19, 2016

bryk commented Feb 19, 2016

joeatwork commented Feb 19, 2016

floreks commented Feb 22, 2016

bryk commented Feb 22, 2016

bryk commented Feb 22, 2016

joeatwork Feb 24, 2016

Choose a reason for hiding this comment

joeatwork commented Feb 24, 2016

joeatwork commented Feb 24, 2016

codecov-io commented Feb 25, 2016

Current coverage is 80.28%

bryk commented Feb 25, 2016

joeatwork commented Feb 25, 2016

joeatwork commented Feb 25, 2016

cheld commented Feb 29, 2016

bryk commented Feb 29, 2016

joeatwork commented Feb 29, 2016

bryk commented Feb 29, 2016

joeatwork commented Feb 29, 2016

cheld commented Mar 1, 2016

bryk commented Mar 1, 2016

cheld commented Mar 1, 2016

bryk commented Mar 1, 2016

cheld commented Mar 1, 2016

joeatwork commented Mar 1, 2016

Current coverage is `80.28%`