Add dashboards page with health card #1591

rawagner · 2019-05-21T11:18:47Z

This PR introduces dashboards page as designed in https://docs.google.com/document/d/1q0DIcUKgFhvNbtGt-SMl35Z2jHA_QO2r7aaBuFmwE2U/edit#heading=h.koix4vo7e16v

Dashboards page has its own redux store to be able to share resources (prometheus queries results, various url requests and in the future also k8s resources) between various dashboard cards and also dashboard tabs.

Dashboards page currently contains only one tab (Overview) - plan is to be able to contribute other tabs via static extension mechanism. Overview tab contains only one card - Health which can be extended via static plugin mechanism to show healths for other subsystems (ie KubeVirt or Storage).
Other cards will follow in separate PRs.

Dashboard page is not included in navigation yet, but can be accessed directly via /dashboards route.

Dashboards page - Overview tab

Dashboards page - Overview tab with demo-plugin enabled

@vojtechszocs @cloudbehl

openshift-ci-robot · 2019-05-21T11:19:01Z

Hi @rawagner. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

spadgett · 2019-05-21T12:28:03Z

/assign
/cc @alecmerdler

spadgett · 2019-05-21T12:28:26Z

/ok-to-test

spadgett

We should strive to build our UI by composing simple functional components instead of complicated class-based components.

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

spadgett · 2019-05-21T12:57:11Z

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

Is this an HTTP method like GET, HEAD, etc. or something else? Should have a real type.

changed this a bit and added better types to make it clear. Its not called fetchMethod, but fetch which adds possibility to specify custom function for fetching remote resources.

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

frontend/public/components/dashboards-page/with-dashboard-resources.tsx

frontend/public/reducers/dashboards.ts

rawagner · 2019-05-21T15:20:26Z

@spadgett thank you for the quick review! I will go through the comments and once done I will let you know.

spadgett · 2019-05-21T15:22:11Z

We added this hook for polling prometheus to show area charts, and @TheRealJon is working on generalizing it so it's more generic.

https://github.com/openshift/console/blob/master/frontend/public/components/graphs/use-prometheus-poll.ts

Just wondering if it makes sense to use a similar approach for the dashboards. Then we have a consistent polling mechanism throughout console and avoid duplicate logic.

spadgett · 2019-05-21T17:08:15Z

Dashboards page has its own redux store to be able to share resources (prometheus queries results, various url requests and in the future also k8s resources) between various dashboard cards and also dashboard tabs.

Do we have specific cases where prometheus query results are in fact shared across dashboard cards? I think this is where a lot of the complexity is coming in vs using a simple poll hook and a functional component.

spadgett · 2019-05-21T18:58:59Z

#1596 adds the generic usePoll

rawagner · 2019-05-22T05:34:11Z

Dashboards page has its own redux store to be able to share resources (prometheus queries results, various url requests and in the future also k8s resources) between various dashboard cards and also dashboard tabs.

Do we have specific cases where prometheus query results are in fact shared across dashboard cards? I think this is where a lot of the complexity is coming in vs using a simple poll hook and a functional component.

Yes we do - we use the same queries for Capacity and Utilization card. Once Storage team contributes their Storage dashboard tab they will also want to reuse some storage queries and also Storage health query which will be the same for Health card in Overview and Storage tab. (Storage tab can seen here https://docs.google.com/presentation/d/15nDG3VuQRy6vJbMX4yq6ZvJ5G2Qawaz9glnLT8GEvX4/edit#slide=id.g4d3241551c_0_57 )
We also dont want to refetch all data once the tab changes.

mareklibra · 2019-05-22T07:55:31Z

frontend/public/components/dashboard/dashboard-card/dashboard-card-title-see-all.tsx

@spadgett , isn't there a better place where to store strings all-together?

@spadgett , isn't there a better place where to store strings all-together?

We don't currently externalize any strings, so there's no good place at the moment.

frontend/packages/console-demo-plugin/src/plugin.ts

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

frontend/public/components/dashboards-page/overview-dashboard/health-card.tsx

frontend/public/components/dashboards-page/overview-dashboard/overview-dashboard.tsx

frontend/public/components/dashboards-page/with-dashboard-resources.tsx

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

spadgett · 2019-06-03T13:14:07Z

frontend/public/actions/dashboards.ts

Do we ever intend to have dashboards with pod metrics for normal users? This function doesn't handle using the prometheus tenancy service that lets normal users fetch metrics.

It is only as admin view for now. We dont have any dropdown for choosing other namespace than all for now.

We can punt for now, but I'd assume we'd eventually want to replace the current project dashboard with the same experience reusing these components.

spadgett · 2019-06-03T13:34:28Z

frontend/public/components/dashboard/dashboard-card/card-body.tsx

We've been adding skeleton screens to a lot of UIs. That might be better here instead of a loading indicator. See #1569 and #1608

OK as a follow on.

Nice! UXD started to mockup dashboards with these skeleton screens some time ago and we plan to implement them too.

frontend/public/components/dashboard/health-card/health-card.scss

frontend/public/components/dashboards-page/with-dashboard-resources.tsx

spadgett · 2019-06-03T14:33:21Z

frontend/public/components/dashboards-page/overview-dashboard/health-card.tsx

I think this is OK, but we should be careful that this won't hot loop.

frontend/public/components/dashboard/dashboard-card/card.scss

spadgett · 2019-06-03T14:35:20Z

frontend/public/style.scss

I'm not sure this is right.

cc @rhamilto

Why is this needed? We import color-variables above

color-variables is pf3, sass-utilities/colors is pf4. Variable names are slightly different $color-pf-black-100 (pf3) vs $pf-color-black-100 (pf4)

Variable names are slightly different $color-pf-black-100 (pf3) vs $pf-color-black-100 (pf4)

I'm not sure what to recommend, but that's going to get really confusing :(

I could use the pf3 ones but some variables have different values :/ But if want, i will use it, it shouldnt be a big change and once ready we can switch all css variables to pf4.

No, I would use the PF4 variables here. Just thinking about how to deal with this. Maybe we should create pf3- aliases for legacy colors to avoid confusion, but that's outside the scope of this PR.

Worth a comment in this file explaining, though.

comment added

frontend/public/components/dashboard/dashboard.scss

spadgett · 2019-06-03T14:37:58Z

Thank you for changing these to functional components. This looks a lot cleaner!

spadgett · 2019-06-03T20:28:15Z

frontend/package.json

We should align on how we handle this. Currently console uses this custom hook:

https://github.com/openshift/console/blob/master/frontend/public/components/utils/ref-width-hook.ts

We might look at something based on hooks like:

https://github.com/rehooks/component-size

There are also polyfills for ResizeObserver:

https://caniuse.com/#search=ResizeObserver
https://github.com/que-etc/resize-observer-polyfill

@christianvogt mentioned dev console needs it for topology view as well.

cc @TheRealJon

I tried useRefWidth and it works fine for me. react-measure dep is gone.

spadgett · 2019-06-04T14:04:57Z

Can you check the linter errors?

rawagner · 2019-06-04T14:23:02Z

Can you check the linter errors?

prettier! Thats great.

Linting errors are fixed now.

spadgett · 2019-06-04T16:54:19Z

frontend/public/components/dashboard/dashboard-card/card.scss

If this is only used in one place for the header, can it just be a style on co-dashboard-header__icon instead of having a separate class?

right, merged those two to one class co-dashboard-header__icon

frontend/public/components/dashboard/dashboard-card/card-help.tsx

spadgett · 2019-06-04T17:01:13Z

frontend/public/components/dashboards-page/overview-dashboard/health-card.tsx

Suggested change

stopWatchURL('healtz');

stopWatchURL('healthz');

oops, fixed

spadgett · 2019-06-04T17:02:21Z

frontend/public/components/dashboards-page/overview-dashboard/health-card.tsx

I think it's cleaner to make this an expression directly in the markup from the return value below.

Sounds good to me. Its now part of return statement

spadgett · 2019-06-04T17:03:13Z

frontend/public/components/dashboards-page/overview-dashboard/health-card.tsx

You're not checking if the flag is pending, so it could flash the wrong product name briefly on initial load.

added check for pending flag. While the flag is pending we show loading in Health card body and See All link is not shown at all

spadgett · 2019-06-04T17:06:19Z

frontend/public/style.scss

Why is this needed? We import color-variables above

spadgett · 2019-06-05T14:50:19Z

/approve

Please squash, thanks!

rawagner · 2019-06-05T15:03:25Z

commits squashed

spadgett

/lgtm

spadgett · 2019-06-05T18:38:22Z

/retest

openshift-bot · 2019-06-05T19:02:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-06-05T20:20:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

rawagner · 2019-06-06T16:52:41Z

@spadgett Can you please approve again ? I had to rebase and it removed the label. Thank you!

spadgett · 2019-06-06T17:12:04Z

/lgtm

openshift-ci-robot · 2019-06-06T17:12:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, spadgett, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2019

openshift-ci-robot requested review from jeff-phillips-18 and rhamilto May 21, 2019 11:18

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2019

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 21, 2019

openshift-ci-robot assigned spadgett May 21, 2019

openshift-ci-robot requested a review from alecmerdler May 21, 2019 12:28

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 21, 2019

spadgett requested changes May 21, 2019

View reviewed changes

spadgett added this to the v4.2 milestone May 21, 2019

mareklibra reviewed May 22, 2019

View reviewed changes

rawagner force-pushed the dashboard branch from c1620c7 to 580ecc4 Compare May 22, 2019 08:03

vojtechszocs suggested changes May 22, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2019

rawagner force-pushed the dashboard branch from 580ecc4 to d8141b4 Compare May 29, 2019 08:57

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2019

rawagner force-pushed the dashboard branch 4 times, most recently from a376426 to 5755038 Compare May 29, 2019 15:02

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2019

spadgett reviewed Jun 3, 2019

View reviewed changes

rawagner force-pushed the dashboard branch 2 times, most recently from bf5d826 to 272cb86 Compare June 4, 2019 13:36

rawagner force-pushed the dashboard branch from 272cb86 to 3f8089a Compare June 4, 2019 14:21

spadgett reviewed Jun 4, 2019

View reviewed changes

rawagner force-pushed the dashboard branch from 1b10772 to f593aca Compare June 4, 2019 18:56

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2019

rawagner force-pushed the dashboard branch from f593aca to 565a012 Compare June 5, 2019 14:58

spadgett approved these changes Jun 5, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 5, 2019

Add dashboards page with health card

d276b88

rawagner force-pushed the dashboard branch from 565a012 to d276b88 Compare June 6, 2019 05:18

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 6, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2019

openshift-merge-robot merged commit 32e4fee into openshift:master Jun 6, 2019

pcbailey mentioned this pull request Jun 14, 2019

Add the top consumers card to dashboard #1722

Merged

Add dashboards page with health card #1591

Add dashboards page with health card #1591

Uh oh!

Conversation

rawagner commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented May 21, 2019

Uh oh!

spadgett commented May 21, 2019

Uh oh!

spadgett commented May 21, 2019

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rawagner May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rawagner commented May 21, 2019

Uh oh!

spadgett commented May 21, 2019

Uh oh!

spadgett commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented May 21, 2019

Uh oh!

rawagner commented May 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rawagner commented May 21, 2019 •

edited

Loading

rawagner May 29, 2019 •

edited

Loading

spadgett commented May 21, 2019 •

edited

Loading