Added monitoring page with tabs as part of ODC-2571 #3837

abhi-kn · 2019-12-27T19:16:24Z

US: https://issues.redhat.com/browse/ODC-2571

Analysis:
Need to add new menu item 'Monitoring' with tabs 'Dashboard' & 'Metrics'.

Solution:

Added new menu item Monitoring along with routes in console/plugin
created Monitoring page with tabs
created new components for Dashboard & Metrics page
added unit tests

Screen shots for design review:
@openshift/team-devconsole-ux

abhi-kn · 2019-12-27T19:17:33Z

/assign @rohitkrai03

invincibleJai · 2019-12-30T05:48:09Z

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

+import * as React from 'react';
+import { Helmet } from 'react-helmet';
+
+const MonitoringDashboard: React.FC<{}> = () => {


nit : can we just have React.FC instead of React.FC<{}>

sahil143 · 2019-12-30T08:54:58Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+  );
+};
+
+export default useActiveNamespace(MonitoringPage);


nit: use in front of a name is usually used for custom hooks. You can probably name it something else like connectActiveNamespace.

Agreed. Renamed as suggested

sahil143 · 2019-12-30T08:58:55Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+                  component: MonitoringMetrics,
+                },
+              ]}
+              className={`co-m-${_.get(ProjectModel.kind, 'kind', ProjectModel.kind)}`}


This _.get method will always give undefined and will fall back to the default parameter every time. Also, no need to use get for models as they have hardcoded values

Suggested change

className={`co-m-${_.get(ProjectModel.kind, 'kind', ProjectModel.kind)}`}

className={`co-m-${ProjectModel.kind}`}

sahil143

Selecting namespace form the project selector doesn't update the URL. Similarly, changing the namespace in the URL doesn't update the selected item in the project selector.

vikram-raj · 2019-12-30T09:54:05Z

frontend/packages/dev-console/src/plugin.tsx

+    properties: {
+      perspective: 'dev',
+      exact: false,
+      path: ['/dev/monitoring/all-namespaces', '/dev/monitoring/ns/:ns'],


Selecting namespace form the project selector doesn't update the URL. Similarly, changing the namespace in the URL doesn't update the selected item in the project selector.

This issue is happening because of dev in the path. Do we need dev in the path?

I think dev has been added in the path because there's already a Monitoring Nav item present in the admin perspective (which has Alerting, Metrics & Dashboards as sub-items).

As @debsmita1 mentioned, I had added dev in path to differentiate Monitoring Nav in Admin & Dev perspective. But having /dev in path doesn't update the URL on namespace change & hence fixed the bug by changing path with dev prefix.

vikram-raj · 2019-12-30T10:55:49Z

frontend/packages/dev-console/src/plugin.tsx

+    type: 'Page/Route',
+    properties: {
+      perspective: 'dev',
+      exact: false,


Exact should be true.

Having exact as true, Tab routes doesn't work & we get 404. Need to check with @christianvogt regarding this.

I had similar issues with Project Details page. Christian and I opted out of using false as it can lead to unintentional matches. Since this is a new route, I am unsure it'll be of a big deal either way.

If you want an example of what I did, see:

{ type: 'Page/Route', properties: { perspective: 'dev', exact: true, path: ['/k8s/cluster/projects'], loader: async () => (await import( './components/projects/details/AllProjectsDetailList' /* webpackChunkName: "all-projects-detail-list" */ )).default, }, }, { type: 'Page/Route', properties: { perspective: 'dev', path: ['/k8s/cluster/projects/:name'], loader: async () => (await import( './components/projects/details/ProjectDetailsPage' /* webpackChunkName: "project-details" */ )).default, }, },

In this file.

@andrewballantyne actually exact defaults to false so now I've split the routes & followed Project Details page approach.

Ah, right you are... it was late when I made that comment :)

sahil143

/lgtm

andrewballantyne · 2020-01-02T20:14:37Z

/kind feature

andrewballantyne

/assign

andrewballantyne · 2020-01-03T04:49:25Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+  );
+};
+
+export default connectActiveNamespace(MonitoringPage);


Root-level pages don't need to connect to the store to get the namespace. Each page, using the type: 'Page/Route' plugin, will be created using React-Router. This injects a match prop. Inside it is the namespace.

props.match.params {ns: "<my-current-namespace>"}

The ns is the param you gave to the route URL in the plugin file '/dev-monitoring/ns/:ns'.

Checking for all-projects becomes a check against the missing param.

@andrewballantyne removed connection to store & now using the match prop to get namespace

andrewballantyne · 2020-01-03T04:51:52Z

frontend/packages/dev-console/src/components/monitoring/utils.ts

+export const connectActiveNamespace = (Component) =>
+  connect((state: RootState) => ({ activeNamespace: getActiveNamespace(state) }))(Component);
+
+export type ConnectActiveNamespaceProps = {
+  activeNamespace: string;
+  match: RMatch<{
+    ns?: string;
+  }>;
+};


Based on my other comment, this isn't needed.

andrewballantyne · 2020-01-03T04:57:19Z

frontend/packages/dev-console/src/plugin.tsx

+    type: 'Page/Route',
+    properties: {
+      perspective: 'dev',
+      exact: false,


I had similar issues with Project Details page. Christian and I opted out of using false as it can lead to unintentional matches. Since this is a new route, I am unsure it'll be of a big deal either way.

If you want an example of what I did, see:

{ type: 'Page/Route', properties: { perspective: 'dev', exact: true, path: ['/k8s/cluster/projects'], loader: async () => (await import( './components/projects/details/AllProjectsDetailList' /* webpackChunkName: "all-projects-detail-list" */ )).default, }, }, { type: 'Page/Route', properties: { perspective: 'dev', path: ['/k8s/cluster/projects/:name'], loader: async () => (await import( './components/projects/details/ProjectDetailsPage' /* webpackChunkName: "project-details" */ )).default, }, },

In this file.

abhi-kn · 2020-01-03T12:20:09Z

/retest

invincibleJai · 2020-01-03T12:17:34Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+  history.push(redirectURI);
+};
+
+export const MonitoringPage: React.FC<MonitoringPageProps> = (props) => {


nit : we can destructure props to { match }

invincibleJai · 2020-01-03T12:18:16Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+};
+
+export const MonitoringPage: React.FC<MonitoringPageProps> = (props) => {
+  const activeNamespace = _.get(props, 'match.params.ns');


and avoid _.get with const activeNamespace = match.params.ns;

match.params will be an empty object when in all namespace so to avoid additional checks I've used _.get

yeah doesn't matter much just nit and as even for _.get we are not passing any default value so will be undefined for all namespace and same will be for match.params.ns as params is {}

We want to avoid using _.get especially when the type itself is defined in a way that disallows intermediate undefined values as is the case here.

Avoided _.get

invincibleJai · 2020-01-03T12:58:52Z

Have verified locally works as expected.

Note: user will see blank pages for dashboard and metrics tab as those are the separate US

abhi-kn · 2020-01-03T14:55:08Z

/retest

invincibleJai · 2020-01-03T15:08:59Z

/lgtm

andrewballantyne

I'm not sure this new plugin config is good. I think you went half-way between what you had and what I previously did for the Project Details page. Recommend you go back to the way you had it or talk to Christian about alternatives tomorrow.

andrewballantyne · 2020-01-05T21:09:05Z

frontend/packages/dev-console/src/plugin.tsx

+  {
+    type: 'Page/Route',
+    properties: {
+      perspective: 'dev',
+      exact: true,
+      path: ['/dev-monitoring/all-namespaces'],
+      loader: async () =>
+        (await import(
+          './components/monitoring/MonitoringPage' /* webpackChunkName: "dev-console-monitoring" */
+        )).default,
+    },
+  },
+  {
+    type: 'Page/Route',
+    properties: {
+      perspective: 'dev',
+      path: ['/dev-monitoring/ns/:ns'],
+      loader: async () =>
+        (await import(
+          './components/monitoring/MonitoringPage' /* webpackChunkName: "dev-console-monitoring" */
+        )).default,
+    },
+  },


I don't think this is appropriate. Two different routes configs to the same component? I think we should either do one for the "all project" list and one for the "Details" or just house them both under the non-exact config you had before. This is not helping much.

Perhaps @christianvogt knows of a better use of the plugin to get around the previous issue you had @abhi-kn - would recommend reaching out to him tomorrow.

Technically this is correct.
But I think you can create a single path as follows with exact: false:

/dev-monitoring(/all-namespaces$|/ns/:ns)

Tried with suggested path & it works partially. Basically :ns doesn't work so I've reverted to single route with multiple paths.

christianvogt · 2020-01-06T15:22:45Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+};
+
+export const MonitoringPage: React.FC<MonitoringPageProps> = (props) => {
+  const activeNamespace = _.get(props, 'match.params.ns');


We want to avoid using _.get especially when the type itself is defined in a way that disallows intermediate undefined values as is the case here.

christianvogt · 2020-01-06T15:27:06Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+      <NamespacedPage
+        hideApplications
+        variant={NamespacedPageVariants.light}
+        onNamespaceChange={handleNamespaceChange}


Why is this needed?
Isn't this performing the standard behavior of switching between namespaces?

Without this URL doesn't get updated on selecting a project from Project List

christianvogt · 2020-01-06T15:34:15Z

frontend/packages/dev-console/src/components/monitoring/MonitoringPage.tsx

+export const MONITORING_NS_PAGE_URI = '/dev-monitoring/ns';
+export const MONITORING_ALL_NS_PAGE_URI = '/dev-monitoring/all-namespaces';
+
+export interface MonitoringPageProps {


We are no longer exporting component props.
Instead, when needed, a consumer can use React.ComponentProps<typeof MyComponent> to get a reference to the prop type.

Agreed. Removed export

christianvogt · 2020-01-06T15:38:46Z

frontend/packages/dev-console/src/components/monitoring/__tests__/MonitoringPage.spec.tsx

+type MonitoringPageProps = React.ComponentProps<typeof MonitoringPage>;
+let monPageProps: MonitoringPageProps;


Not much value had separating the type definition like this.
May just as well do:
let monPageProps: React.ComponentProps<typeof MonitoringPage>

Also, it's more clear to move the declaration into the test itself or at the very least inside the describe block.

Agreed. Modified as suggested.

christianvogt · 2020-01-06T15:39:51Z

frontend/packages/dev-console/src/components/monitoring/__tests__/MonitoringPage.spec.tsx

+    };
+
+    const component = shallow(<MonitoringPage {...monPageProps} />);
+    expect(component.exists()).toBe(true);


You can remove this assertion.
An assertion that the wrapper itself exists is useless because it will never be false.

christianvogt · 2020-01-06T15:47:28Z

frontend/packages/dev-console/src/components/monitoring/__tests__/MonitoringPage.spec.tsx

+    };
+    const component = shallow(<MonitoringPage {...monPageProps} />);
+    expect(component.find(ProjectListPage).exists()).toBe(true);
+    expect(component.find(ProjectListPage).prop('title')).toBe('Monitoring');


This asserts static content.

Agree, remove assertion.

christianvogt · 2020-01-06T15:48:09Z

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

+describe('Monitoring Dashboard Tab', () => {
+  it('should render Monitoring Dashboard tab', () => {
+    const wrapper = shallow(<MonitoringDashboard />);
+    expect(wrapper.exists()).toBe(true);


Wrapper will always exist.

christianvogt · 2020-01-06T15:48:49Z

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

+  it('should render Monitoring Dashboard tab', () => {
+    const wrapper = shallow(<MonitoringDashboard />);
+    expect(wrapper.exists()).toBe(true);
+    expect(wrapper.contains(<title>Dashboard</title>)).toBe(true);


Asserts static content.

Added a dummy test case as the page is empty now.

Not sure this is helpful... test file creation isn't a complicated process. Would recommend deleting this file.

christianvogt · 2020-01-06T15:49:07Z

.../packages/dev-console/src/components/monitoring/metrics/__tests__/MonitoringMetrics.spec.tsx

+    expect(wrapper.exists()).toBe(true);
+    expect(wrapper.contains(<title>Metrics</title>)).toBe(true);


Same comments as before.

Added a dummy test cases as the page is empty now

christianvogt · 2020-01-06T16:02:44Z

frontend/packages/dev-console/src/plugin.tsx

+  {
+    type: 'Page/Route',
+    properties: {
+      perspective: 'dev',
+      exact: true,
+      path: ['/dev-monitoring/all-namespaces'],
+      loader: async () =>
+        (await import(
+          './components/monitoring/MonitoringPage' /* webpackChunkName: "dev-console-monitoring" */
+        )).default,
+    },
+  },
+  {
+    type: 'Page/Route',
+    properties: {
+      perspective: 'dev',
+      path: ['/dev-monitoring/ns/:ns'],
+      loader: async () =>
+        (await import(
+          './components/monitoring/MonitoringPage' /* webpackChunkName: "dev-console-monitoring" */
+        )).default,
+    },
+  },


Technically this is correct.
But I think you can create a single path as follows with exact: false:

/dev-monitoring(/all-namespaces$|/ns/:ns)

andrewballantyne · 2020-01-07T20:22:33Z

frontend/packages/dev-console/src/components/monitoring/__tests__/MonitoringPage.spec.tsx

+    const component = shallow(<MonitoringPage {...monPageProps} />);
+    expect(component.exists()).toBe(true);
+    expect(component.find(PageHeading).exists()).toBe(true);
+    expect(component.find(PageHeading).prop('title')).toBe('Monitoring');


I don't think Unit Tests serve to validate acceptance criteria. They serve to help avoid accidental breaks.

andrewballantyne · 2020-01-07T20:23:42Z

frontend/packages/dev-console/src/components/monitoring/__tests__/MonitoringPage.spec.tsx

+    };
+    const component = shallow(<MonitoringPage {...monPageProps} />);
+    expect(component.find(ProjectListPage).exists()).toBe(true);
+    expect(component.find(ProjectListPage).prop('title')).toBe('Monitoring');


Agree, remove assertion.

andrewballantyne · 2020-01-07T20:25:32Z

frontend/packages/dev-console/src/components/monitoring/__tests__/MonitoringPage.spec.tsx

+    const actualTabs = component
+      .find(HorizontalNav)
+      .prop('pages')
+      .map((page) => page.name);
+    expect(actualTabs).toEqual(expectedTabs);


I don't mind this static test so much as it validates the order, which is another flex point that could be broken by changes.

andrewballantyne · 2020-01-07T20:26:00Z

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

+  it('should render Monitoring Dashboard tab', () => {
+    const wrapper = shallow(<MonitoringDashboard />);
+    expect(wrapper.exists()).toBe(true);
+    expect(wrapper.contains(<title>Dashboard</title>)).toBe(true);


Not sure this is helpful... test file creation isn't a complicated process. Would recommend deleting this file.

andrewballantyne · 2020-01-07T20:33:13Z

.../packages/dev-console/src/components/monitoring/metrics/__tests__/MonitoringMetrics.spec.tsx

+describe('Monitoring Metrics Tab', () => {
+  it('should render Monitoring Metrics tab', () => {
+    const wrapper = shallow(<MonitoringMetrics />);
+    expect(wrapper.contains(<title>Metrics</title>)).toBe(true);


Same here, I'd drop this file too. They can be created later to handle any functionality that is added.

andrewballantyne · 2020-01-08T13:19:58Z

/lgtm
/approve

openshift-ci-robot · 2020-01-08T13:20:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhi-kn, andrewballantyne, invincibleJai, sahil143

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~~ [andrewballantyne]

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

andrewballantyne · 2020-01-08T14:16:50Z

Unblocking the team by getting this in. I don't believe there was any pending issues outside of tests that needs to be addressed.

@abhi-kn I'd like to see these changes done to cleanup the tests, but I think this foundation work should get in first so the team can build upon it.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 27, 2019

openshift-ci-robot requested review from debsmita1 and divyanshiGupta December 27, 2019 19:16

openshift-ci-robot added the component/dev-console Related to dev-console label Dec 27, 2019

openshift-ci-robot assigned rohitkrai03 Dec 27, 2019

invincibleJai reviewed Dec 30, 2019

View reviewed changes

sahil143 suggested changes Dec 30, 2019

View reviewed changes

vikram-raj reviewed Dec 30, 2019

View reviewed changes

sahil143 approved these changes Jan 2, 2020

View reviewed changes

openshift-ci-robot assigned sahil143 Jan 2, 2020

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

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

andrewballantyne suggested changes Jan 3, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 3, 2020

abhi-kn force-pushed the ODC-2571 branch from 715a62f to 1ab2e6c Compare January 3, 2020 11:57

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

invincibleJai reviewed Jan 3, 2020

View reviewed changes

openshift-ci-robot assigned invincibleJai Jan 3, 2020

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

andrewballantyne suggested changes Jan 5, 2020

View reviewed changes

christianvogt reviewed Jan 6, 2020

View reviewed changes

Added monitoring page with tabs as part of ODC-2571

5fbbda2

abhi-kn added 4 commits January 7, 2020 13:46

Fixed namespace change bug & review comments

8d06601

removed connection to store & fixed comments

3240cf7

fixed lint error in spec file

9590a7a

fixed retaining tab route on NS change along with review comments

4e47de3

abhi-kn force-pushed the ODC-2571 branch from e899868 to 4e47de3 Compare January 7, 2020 08:19

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

andrewballantyne suggested changes Jan 7, 2020

View reviewed changes

invincibleJai mentioned this pull request Jan 8, 2020

Dev metrics: visualisation of queries in metrics #3891

Merged

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

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

openshift-merge-robot merged commit 196d87b into openshift:master Jan 8, 2020

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

	className={`co-m-${_.get(ProjectModel.kind, 'kind', ProjectModel.kind)}`}
	className={`co-m-${ProjectModel.kind}`}

		type MonitoringPageProps = React.ComponentProps<typeof MonitoringPage>;
		let monPageProps: MonitoringPageProps;

		expect(wrapper.exists()).toBe(true);
		expect(wrapper.contains(<title>Metrics</title>)).toBe(true);

Added monitoring page with tabs as part of ODC-2571 #3837

Added monitoring page with tabs as part of ODC-2571 #3837

Conversation

abhi-kn commented Dec 27, 2019 • edited

abhi-kn commented Dec 27, 2019

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

sahil143 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahil143 left a comment

Choose a reason for hiding this comment

andrewballantyne commented Jan 2, 2020

andrewballantyne left a 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

abhi-kn commented Jan 3, 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

invincibleJai commented Jan 3, 2020

abhi-kn commented Jan 3, 2020

invincibleJai commented Jan 3, 2020

andrewballantyne left a 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

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

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

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

Choose a reason for hiding this comment

andrewballantyne commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

andrewballantyne commented Jan 8, 2020

abhi-kn commented Dec 27, 2019 •

edited