Add monitoring dashboard graph #3923

vikram-raj · 2020-01-11T04:57:33Z

story - https://issues.redhat.com/browse/ODC-2627

This PR adds graphs on dev-monitoring dashboard page.

Adding WIP as pre-can queries are not finalized yet.

vikram-raj · 2020-01-11T04:58:15Z

/kind feature

vikram-raj · 2020-01-11T04:59:45Z

cc: @openshift/team-devconsole-ux

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardGraph.scss

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardGraph.tsx

debsmita1 · 2020-01-13T20:21:14Z

No tests for MonitoringDashboardGraph component?

abhi-kn · 2020-01-14T09:02:24Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboard.tsx

Better to define a new chart type for this & move it under MonitoringDashboardGraph

abhi-kn · 2020-01-14T09:06:58Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardGraph.tsx

humanize & DataType shall vary & depend on query so it's better to drive both through props.

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardGraph.tsx

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboard.tsx

abhi-kn · 2020-01-14T17:31:00Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardGraph.tsx

change the type to GraphTypes

abhi-kn · 2020-01-14T17:31:40Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardGraph.tsx

better to use uppercase for enums

abhi-kn · 2020-01-14T17:36:57Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardQueries.ts

use humanizeCpuCores

abhi-kn · 2020-01-14T17:37:12Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardQueries.ts

use humanizeDecimalBytesPerSec

abhi-kn · 2020-01-14T17:45:45Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardQueries.ts

better to use enum GraphTypes

invincibleJai · 2020-01-16T16:35:58Z

...d/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardCountBlock.tsx

nit: we have type React.FC and do implicit return

FWIW @invincibleJai this is more than just a nit, it's a requirement. We don't want to create anything without types if we can avoid it.

invincibleJai · 2020-01-16T16:36:33Z

...d/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardCountBlock.tsx

nit: can we have class name as odc-filename-*

I am reusing this class from _graph.scss.

invincibleJai · 2020-01-17T12:02:44Z

/lgtm

serenamarie125

@vikram-raj i'm not sure if this story covers this. But is it currently possible to click on the graph, and have it bring you to the Metrics tab with "Custom Query" selected and the PromQL field populated with the associated query & the graph shown?

If this is not part of this story, lmk and i'll enter a bug/enhancement.

Note that this is the behavior of the Project Dashboard graphs, but I'm not sure if we explicitly called this out in the Acceptance Criteria.

invincibleJai · 2020-01-20T03:26:07Z

@serenamarie125 this has been handled as part of #3954, it's in WIP as it depends on work for #3891 cc @vikram-raj

christianvogt · 2020-01-21T01:32:08Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboard.tsx

Nit: MonitoringDashboardQueries is not a component file and therefore the file name shouldn't begin in uppercase.

andrewballantyne

Not sure what the prerequisites are to run this, but I got console & UI errors when I tried to test it...

andrewballantyne · 2020-01-21T11:02:55Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboard.tsx

We don't export types anymore. You can infer the type of a component through React.ComponentProps<typeof MyComponent>

andrewballantyne · 2020-01-21T11:04:53Z

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboard.tsx

Suggested change

<GridItem span={4} key={i}>

<GridItem span={4} key={title}>

Could we not use title for this? Two same-named items wouldn't be helpful, no?

Maybe ${chartType}-${title} if we'd want the same stat in two different chart formats?

andrewballantyne · 2020-01-21T11:09:57Z

.../dev-console/src/components/monitoring/dashboard/__tests__/MonitoringDashboardGraph.spec.tsx

Perhaps I am missing the obvious here, but how does this render a line graph? QueryBrowser is a line graph?

Yes, QueryBrowser component render line graph.

~~Not sure the test needs to know that. If the underlying QueryBrowser changes the way it works, our test wouldn't make sense anymore.~~ nvm... it appears you explicitly check for line before rendering the QueryBrowser lol

andrewballantyne · 2020-01-21T11:10:48Z

frontend/packages/dev-console/src/components/monitoring/dashboard/monitoringDashboardQueries.ts

Perhaps give this a type so if any other items are added to the array they conform to the same shape.

andrewballantyne · 2020-01-21T11:49:55Z

Okay, got the graphs to render... was my local env that had an issue (needed to rebuild the server).

UX Concern (@openshift/team-devconsole-ux) - hovering over a chart allows me to "zoom in"... but I have no way to zoom back out. This feels like a pretty big concern to the sanity of using this feature. To get monitoring in is probably our first priority, but I think we need a bug to track this for fix after feature freeze (or if there is time before feature freeze).

andrewballantyne

LGTM, but I think we need to address the UX issue with the line charts.

@vikram-raj please log a bug around #3923 (comment)

vikram-raj · 2020-01-21T12:09:02Z

Okay, got the graphs to render... was my local env that had an issue (needed to rebuild the server).

UX Concern (@openshift/team-devconsole-ux) - hovering over a chart allows me to "zoom in"... but I have no way to zoom back out. This feels like a pretty big concern to the sanity of using this feature. To get monitoring in is probably our first priority, but I think we need a bug to track this for fix after feature freeze (or if there is time before feature freeze).

Yes, we can take it as bug because in QueryBrowser component has the prop to hideControls and it hides the zoom out button for the graph but didn't disable the zoom-in feature.

vikram-raj · 2020-01-21T12:14:59Z

LGTM, but I think we need to address the UX issue with the line charts.

@vikram-raj please log a bug around #3923 (comment)

@andrewballantyne Here is the ticket to track this issue https://issues.redhat.com/browse/ODC-2779

invincibleJai · 2020-01-21T12:21:55Z

/lgtm

openshift-ci-robot · 2020-01-21T12:22:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, invincibleJai, vikram-raj

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/packages/dev-console/OWNERS~~ [andrewballantyne]

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

openshift-bot · 2020-01-21T12:58:44Z

/retest

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

openshift-bot · 2020-01-21T13:25:04Z

/retest

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

openshift-bot · 2020-01-21T13:37:51Z

/retest

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

kyoto · 2020-01-22T03:27:15Z

FYI @openshift/openshift-team-monitoring

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console labels Jan 11, 2020

openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 11, 2020

openshift-ci-robot requested review from abhinandan13jan and nemesis09 January 11, 2020 04:58

vikram-raj force-pushed the odc-2627 branch from 2b72c96 to d98b7c4 Compare January 13, 2020 08:49

debsmita1 reviewed Jan 13, 2020

View reviewed changes

abhi-kn suggested changes Jan 14, 2020

View reviewed changes

invincibleJai reviewed Jan 14, 2020

View reviewed changes

frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboard.tsx Outdated Show resolved Hide resolved

vikram-raj force-pushed the odc-2627 branch from d98b7c4 to 4be9559 Compare January 14, 2020 12:35

vikram-raj requested review from abhi-kn, debsmita1 and invincibleJai January 14, 2020 12:36

vikram-raj mentioned this pull request Jan 14, 2020

Add clickable action to monitoring dashboard graph #3954

Merged

vikram-raj force-pushed the odc-2627 branch from 4be9559 to 4d7e2b4 Compare January 14, 2020 13:46

abhi-kn reviewed Jan 14, 2020

View reviewed changes

abhi-kn mentioned this pull request Jan 14, 2020

added monitoring tab with metrics section #3961

Merged

4 tasks

vikram-raj force-pushed the odc-2627 branch from 4d7e2b4 to c54c998 Compare January 16, 2020 06:27

invincibleJai reviewed Jan 16, 2020

View reviewed changes

vikram-raj force-pushed the odc-2627 branch from c54c998 to 8d0bd61 Compare January 16, 2020 18:45

vikram-raj requested a review from invincibleJai January 16, 2020 18:46

vikram-raj changed the title ~~[WIP]Add monitoring dashboard graph~~ Add monitoring dashboard graph Jan 17, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2020

openshift-ci-robot assigned invincibleJai Jan 17, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2020

serenamarie125 reviewed Jan 19, 2020

View reviewed changes

christianvogt reviewed Jan 21, 2020

View reviewed changes

vikram-raj force-pushed the odc-2627 branch from 8d0bd61 to a8d2331 Compare January 21, 2020 08:32

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

vikram-raj requested a review from christianvogt January 21, 2020 08:33

andrewballantyne suggested changes Jan 21, 2020

View reviewed changes

Add monitoring dashboard graph

1c6f99d

vikram-raj force-pushed the odc-2627 branch from a8d2331 to 1c6f99d Compare January 21, 2020 12:04

andrewballantyne approved these changes Jan 21, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

openshift-merge-robot merged commit 789499a into openshift:master Jan 21, 2020

spadgett added this to the v4.4 milestone Jan 27, 2020

Add monitoring dashboard graph #3923

Add monitoring dashboard graph #3923

Uh oh!

Conversation

vikram-raj commented Jan 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vikram-raj commented Jan 11, 2020

Uh oh!

vikram-raj commented Jan 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

debsmita1 commented Jan 13, 2020

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

invincibleJai commented Jan 17, 2020

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

invincibleJai commented Jan 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewballantyne left a comment

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!

andrewballantyne Jan 21, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

andrewballantyne commented Jan 21, 2020

Uh oh!

vikram-raj commented Jan 11, 2020 •

edited

Loading

andrewballantyne Jan 21, 2020 •

edited

Loading