Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview #4091

vikram-raj · 2020-01-28T15:24:56Z

Fixes:

https://issues.redhat.com/browse/ODC-2836

Analysis / Root cause:

For the metrics section in the overview monitoring, we show only 3 metrics for the workload. and there was no way to see all the metrics for the workload.

Solution Description:

Added View monitoring dashboard link which will redirect the user to monitoring dashboard and show all the metrics on the dashboard.

Screen shots / Gifs for design review:

Browser conformance:

Chrome
Firefox
Safari
Edge

vikram-raj · 2020-01-28T15:27:20Z

cc: @serenamarie125

serenamarie125 · 2020-01-28T15:46:04Z

Can you add a bit more spacing above/below the link?
pf-global-spacer--sm (8 px)

here's a quick mock

invincibleJai · 2020-01-28T16:25:17Z

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

+              query={
+                workloadName && workloadType
+                  ? q.query({ ns: namespace, workloadName, workloadType })
+                  : q.query({ namespace })
+              }


can we have specs for this

@invincibleJai I will add a test case for this.

added tests for this. PTAL.

vikram-raj · 2020-01-28T17:01:02Z

Can you add a bit more spacing above/below the link?
pf-global-spacer--sm (8 px)

here's a quick mock

Added the more space above and below the link.

serenamarie125

Great thanks! Looks good to me

vikram-raj · 2020-01-29T06:24:03Z

/kind bug

sahil143 · 2020-01-29T14:33:13Z

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

+  let queries: MonitoringQuery[];
+
+  if (workloadName && workloadType) {
+    queries = workloadMetricQueries;
+  } else {
+    queries = monitoringDashboardQueries;
+  }


this can written as const queries: MonitoringQuery[] = workloadName && workloadtype ? workloadMetricQueries : monitoringDashboardQueries

vikram-raj · 2020-01-30T13:37:52Z

/retitle Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview

openshift-ci-robot · 2020-01-30T13:37:59Z

@vikram-raj: This pull request references Bugzilla bug 1796463, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview

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.

christianvogt · 2020-01-30T17:42:30Z

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

+                workloadName && workloadType
+                  ? q.query({ ns: namespace, workloadName, workloadType })
+                  : q.query({ namespace })


Change ns to namespace and update the templates for consistency. Not sure why it matters if workloadName or workloadType are undefined. There shouldn't be anything wrong with providing extra information that a template may not use.
query={q.query({ namespace, workloadName, workloadType })}

Changed ns to namespace. I was not aware of this that we can pass extra information to a template that may not use.

christianvogt · 2020-01-30T17:56:23Z

frontend/packages/dev-console/src/components/monitoring/overview/MonitoringMetrics.tsx

 import MonitoringDashboardGraph from '../dashboard/MonitoringDashboardGraph';

 const WorkloadGraphs = requirePrometheus(({ resource }) => {
  const ns = resource?.metadata?.namespace;
  const workloadName = resource?.metadata?.name;
  const workloadType = resource?.kind?.toLowerCase();
+  const queries = _.slice(workloadMetricQueries, 0, 3);


What is the voodoo #3 :)
I'm a bit concerned that if the order of workloadMetricQueries changes that it will negatively impact this view. Perhaps create another exported value from the module that provides the set of queries to use here.

created an another module for overview metrics.

christianvogt · 2020-01-30T18:01:38Z

frontend/packages/dev-console/src/components/monitoring/overview/MonitoringOverview.tsx

@@ -78,6 +79,15 @@ const MonitoringOverview: React.FC<MonitoringOverviewProps> = ({ resource, event
            Metrics
          </AccordionToggle>
          <AccordionContent id="metrics-content" isHidden={!expanded.includes('metrics')}>
+            <h5 className="odc-monitoring-overview__view-monitoring-dashboard text-right">


Why an h5 ?
Should use pf4 utilities like pf-u-text-align-right if available instead of bootstrap.

removed bootstrap text-right class and defined it in odc-monitoring-overview__view-monitoring-dashboard this class. Didn't find any pf4 utility for this.

hmm must be because we aren't importing those utilities. oh well

…g overview

christianvogt · 2020-02-03T21:40:43Z

/lgtm
/approve

openshift-ci-robot · 2020-02-03T21:41:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, serenamarie125, vikram-raj

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [christianvogt]

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-02-03T23:29:10Z

/retest

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

openshift-ci-robot · 2020-02-04T03:38:55Z

@vikram-raj: All pull requests linked via external trackers have merged. Bugzilla bug 1796463 has been moved to the MODIFIED state.

In response to this:

Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview

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.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 28, 2020

openshift-ci-robot requested review from debsmita1 and nemesis09 January 28, 2020 15:25

openshift-ci-robot added the component/dev-console Related to dev-console label Jan 28, 2020

invincibleJai reviewed Jan 28, 2020

View reviewed changes

vikram-raj force-pushed the ODC-2836 branch from 2e6b023 to dfba0d8 Compare January 28, 2020 16:59

serenamarie125 approved these changes Jan 28, 2020

View reviewed changes

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

vikram-raj force-pushed the ODC-2836 branch from dfba0d8 to f8ed57d Compare January 29, 2020 07:30

sahil143 suggested changes Jan 29, 2020

View reviewed changes

vikram-raj force-pushed the ODC-2836 branch from f8ed57d to 12adbb1 Compare January 29, 2020 15:19

vikram-raj requested review from sahil143 and invincibleJai January 29, 2020 16:49

openshift-ci-robot changed the title ~~add view monitoring dashboard option for metrics section in monitoring overview~~ Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview Jan 30, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 30, 2020

christianvogt reviewed Jan 30, 2020

View reviewed changes

vikram-raj force-pushed the ODC-2836 branch from 12adbb1 to c9f3e22 Compare January 31, 2020 06:50

add view monitoring dashboard option for metrics section in monitorin…

febe4eb

…g overview

vikram-raj force-pushed the ODC-2836 branch from c9f3e22 to febe4eb Compare January 31, 2020 06:55

vikram-raj requested a review from christianvogt January 31, 2020 07:02

openshift-ci-robot assigned christianvogt Feb 3, 2020

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

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

openshift-merge-robot merged commit ce739fe into openshift:master Feb 4, 2020

vikram-raj deleted the ODC-2836 branch February 4, 2020 05:37

spadgett added this to the v4.4 milestone Feb 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview #4091

Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview #4091

vikram-raj commented Jan 28, 2020 •

edited

vikram-raj commented Jan 28, 2020

serenamarie125 commented Jan 28, 2020

invincibleJai Jan 28, 2020

vikram-raj Jan 28, 2020

vikram-raj Jan 29, 2020

vikram-raj commented Jan 28, 2020

serenamarie125 left a comment

vikram-raj commented Jan 29, 2020

sahil143 Jan 29, 2020

vikram-raj commented Jan 30, 2020

openshift-ci-robot commented Jan 30, 2020

christianvogt Jan 30, 2020

vikram-raj Jan 31, 2020

christianvogt Jan 30, 2020

vikram-raj Jan 31, 2020

christianvogt Jan 30, 2020

vikram-raj Jan 31, 2020

christianvogt Feb 3, 2020

christianvogt commented Feb 3, 2020

openshift-ci-robot commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-ci-robot commented Feb 4, 2020

Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview #4091

Bug 1796463: add view monitoring dashboard option for metrics section in monitoring overview #4091

Conversation

vikram-raj commented Jan 28, 2020 • edited

vikram-raj commented Jan 28, 2020

serenamarie125 commented Jan 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikram-raj commented Jan 28, 2020

serenamarie125 left a comment

Choose a reason for hiding this comment

vikram-raj commented Jan 29, 2020

Choose a reason for hiding this comment

vikram-raj commented Jan 30, 2020

openshift-ci-robot commented Jan 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Feb 3, 2020

openshift-ci-robot commented Feb 3, 2020

openshift-bot commented Feb 3, 2020

openshift-ci-robot commented Feb 4, 2020

vikram-raj commented Jan 28, 2020 •

edited