Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1804638: add tooltip to devconsole monitoring graph #4363

Merged
merged 1 commit into from Feb 25, 2020
+39 −20
Merged

Bug 1804638: add tooltip to devconsole monitoring graph #4363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions frontend/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboard.tsx
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ import { match as RMatch } from 'react-router-dom';
import { Helmet } from 'react-helmet';
import { Grid, GridItem } from '@patternfly/react-core';
import { getURLSearchParams } from '@console/internal/components/utils';
import MonitoringDashboardGraph from './MonitoringDashboardGraph';
import ConnectedMonitoringDashboardGraph from './MonitoringDashboardGraph';
import {
monitoringDashboardQueries,
workloadMetricsQueries,
Expand Down Expand Up @@ -39,7 +39,7 @@ const MonitoringDashboard: React.FC<MonitoringDashboardProps> = ({ match }) => {
</GridItem>
{_.map(queries, (q, i) => (
<GridItem span={i === 0 ? 5 : 4} key={q.title}>
<MonitoringDashboardGraph
<ConnectedMonitoringDashboardGraph
title={q.title}
namespace={namespace}
graphType={q.chartType}
Expand Down
42 changes: 30 additions & 12 deletions ...end/packages/dev-console/src/components/monitoring/dashboard/MonitoringDashboardGraph.tsx
View file
Open in desktop
@@ -1,46 +1,64 @@
import * as React from 'react';
import { QueryBrowser } from '@console/internal/components/monitoring/query-browser';
import { connect } from 'react-redux';
import { QueryBrowser, QueryObj } from '@console/internal/components/monitoring/query-browser';
import { Humanize } from '@console/internal/components/utils';
import { ByteDataTypes } from '@console/shared/src/graph-helper/data-utils';
import { PrometheusGraphLink } from '@console/internal/components/graphs/prometheus-graph';
import { queryBrowserPatchQuery } from '@console/internal/actions/ui';
import './MonitoringDashboardGraph.scss';

export enum GraphTypes {
area = 'Area',
line = 'Line',
}

interface MonitoringDashboardGraphProps {
type DispatchProps = {
patchQuery: (patch: QueryObj) => void;
};

type OwnProps = {
title: string;
query: string;
namespace: string;
graphType?: GraphTypes;
humanize: Humanize;
byteDataType: ByteDataTypes;
}
};

type MonitoringDashboardGraphProps = OwnProps & DispatchProps;

const defaultTimespan = 30 * 60 * 1000;

const MonitoringDashboardGraph: React.FC<MonitoringDashboardGraphProps> = ({
export const MonitoringDashboardGraph: React.FC<MonitoringDashboardGraphProps> = ({
query,
namespace,
title,
patchQuery,
graphType = GraphTypes.area,
}) => {
return (
<div className="odc-monitoring-dashboard-graph">
<h5>{title}</h5>
<PrometheusGraphLink query={query}>
<QueryBrowser
hideControls
defaultTimespan={defaultTimespan}
namespace={namespace}
queries={[query]}
isStack={graphType === GraphTypes.area}
/>
<div onMouseEnter={() => patchQuery({ query })}>
<QueryBrowser
hideControls
defaultTimespan={defaultTimespan}
namespace={namespace}
queries={[query]}
isStack={graphType === GraphTypes.area}
/>
</div>
</PrometheusGraphLink>
</div>
);
};

export default MonitoringDashboardGraph;
const mapDispatchToProps = (dispatch): DispatchProps => ({
patchQuery: (v: QueryObj) => dispatch(queryBrowserPatchQuery(0, v)),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very odd to me that every graph is using the same index.

@kyoto can you please provide insight into the proper usage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always using the index 0 looks wrong to me, so I'm not sure why this appears to be working.

For the admin perspective, I used onMouseEnter as a workaround for the problem (see #4060).

The root problem is that the tooltips assume the query is in Redux, which is actually only true for the Metrics page. Longer term, we should aim to fix that in the tooltips and remove these workarounds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kyoto
Longer term we will move to share more of the dashboards.

});

export default connect<{}, DispatchProps, OwnProps>(
null,
mapDispatchToProps,
)(MonitoringDashboardGraph);
6 changes: 3 additions & 3 deletions ...es/dev-console/src/components/monitoring/dashboard/__tests__/MonitoringDashboard.spec.tsx
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import { shallow } from 'enzyme';
import * as link from '@console/internal/components/utils';
import MonitoringDashboard from '../MonitoringDashboard';
import MonitoringDashboardGraph from '../MonitoringDashboardGraph';
import ConnectedMonitoringDashboardGraph from '../MonitoringDashboardGraph';
import { monitoringDashboardQueries, topWorkloadMetricsQueries } from '../../queries';

type MonitoringDashboardProps = React.ComponentProps<typeof MonitoringDashboard>;
Expand Down Expand Up @@ -35,7 +35,7 @@ describe('Monitoring Dashboard Tab', () => {
const wrapper = shallow(<MonitoringDashboard {...monitoringDashboardProps} />);
expect(
wrapper
.find(MonitoringDashboardGraph)
.find(ConnectedMonitoringDashboardGraph)
.first()
.props().query,
).toEqual(workloadQuery);
Expand All @@ -50,7 +50,7 @@ describe('Monitoring Dashboard Tab', () => {
const wrapper = shallow(<MonitoringDashboard {...monitoringDashboardProps} />);
expect(
wrapper
.find(MonitoringDashboardGraph)
.find(ConnectedMonitoringDashboardGraph)
.first()
.props().query,
).toEqual(dashboardQuery);
Expand Down
3 changes: 2 additions & 1 deletion ...v-console/src/components/monitoring/dashboard/__tests__/MonitoringDashboardGraph.spec.tsx
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@ import { shallow } from 'enzyme';
import { QueryBrowser } from '@console/internal/components/monitoring/query-browser';
import { PrometheusGraphLink } from '@console/internal/components/graphs/prometheus-graph';
import { monitoringDashboardQueries } from '../../queries';
import MonitoringDashboardGraph, { GraphTypes } from '../MonitoringDashboardGraph';
import { MonitoringDashboardGraph, GraphTypes } from '../MonitoringDashboardGraph';

describe('Monitoring Dashboard graph', () => {
let monitoringDashboardGraphProps: React.ComponentProps<typeof MonitoringDashboardGraph>;
Expand All @@ -17,6 +17,7 @@ describe('Monitoring Dashboard graph', () => {
query: query.query({ namespace: 'test-project' }),
humanize: query.humanize,
byteDataType: query.byteDataType,
patchQuery: jest.fn(),
};
});

Expand Down
4 changes: 2 additions & 2 deletions frontend/packages/dev-console/src/components/monitoring/overview/MonitoringMetrics.tsx
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';
import * as _ from 'lodash';
import { requirePrometheus } from '@console/internal/components/graphs';
import { topWorkloadMetricsQueries } from '../queries';
import MonitoringDashboardGraph from '../dashboard/MonitoringDashboardGraph';
import ConnectedMonitoringDashboardGraph from '../dashboard/MonitoringDashboardGraph';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to change all these from MonitoringDashboardGraph to ConnectedMonitoringDashboardGraph. Infact you shouldn't care. You should use the default import as is.

I suggest changing all these import name changes back to simply MonitoringDashboardGraph

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so you'll need to change the name because of the eslint rule for using the same name as an exported value.


const WorkloadGraphs = requirePrometheus(({ resource }) => {
const namespace = resource?.metadata?.namespace;
Expand All @@ -12,7 +12,7 @@ const WorkloadGraphs = requirePrometheus(({ resource }) => {
return (
<>
{_.map(topWorkloadMetricsQueries, (q) => (
<MonitoringDashboardGraph
<ConnectedMonitoringDashboardGraph
key={q.title}
title={q.title}
namespace={namespace}
Expand Down