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

Generate Comparison Charts #3500

Merged
merged 3 commits into from
Jul 26, 2023
Merged

Generate Comparison Charts #3500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9fcbb11
PBENCH-1208
MVarshini Mar 9, 2023
3062e84
remove the commented lines
MVarshini Jul 24, 2023
c46aed1
unused imports
MVarshini Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions dashboard/src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,16 @@ const App = () => {
path={APP_ROUTES.ANALYSIS}
element={<ComingSoonPage />}
/>
<Route
path={APP_ROUTES.VISUALIZATION}
element={<ComparisonComponent />}
/>
</Route>
<Route
path={APP_ROUTES.SEARCH}
element={<ComingSoonPage />}
/>
<Route
path={APP_ROUTES.COMPARISON}
element={<ComparisonComponent />}
/>
</Route>

<Route path="*" element={<NoMatchingPage />} />
</Route>
</Routes>
Expand Down
147 changes: 126 additions & 21 deletions dashboard/src/actions/comparisonActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,28 +37,29 @@ export const getQuisbyData = (dataset) => async (dispatch, getState) => {
dispatch(parseChartData());
}
} catch (error) {
if (error?.response && error.response?.data) {
const errorMsg = error.response.data?.message;
const isUnsupportedType = errorMsg
if (
error?.response?.data &&
error.response.data?.message
?.toLowerCase()
.includes("unsupported benchmark");
if (isUnsupportedType) {
dispatch({
type: TYPES.IS_UNSUPPORTED_TYPE,
payload: errorMsg,
});
}
.includes("unsupported benchmark")
) {
Comment on lines +40 to +45
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is a need to check error?.response?.data before checking error.response.data?.message?.toLowerCase().includes(...) -- this would be a great place to use the ?. operator throughout:

    if (
      error?.response?.data?.message?.toLowerCase().includes("unsupported benchmark")
    ) {

That is, we'll get a false if any of the values are false/undefined/missing, which is what I think you want.

(That said, I agree with Dave's comment below that it would be better if we got the Server's error message instead of a generic, canned one, so we might want to defer or omit the down-casing and the includes(), but I think you can still do it with one test.)

Ditto for the code at line 187.

dispatch({
type: TYPES.IS_UNSUPPORTED_TYPE,
payload: error.response.data.message,
});
Comment on lines +40 to +49
Copy link
Member

Choose a reason for hiding this comment

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

So if we get back a server error that's not "unsupported benchmark", we fall through to the else and get a generic error message string? Wouldn't it be better to show the error if there is one, and save the fallback for a communication error that doesn't yield an error?.response?.data?.message?

} else {
dispatch(showToast(DANGER, ERROR_MSG));
}
dispatch({ type: TYPES.NETWORK_ERROR });
}
dispatch({ type: TYPES.COMPLETED });
};

const COLORS = ["#8BC1F7", "#0066CC", "#519DE9", "#004B95", "#002F5D"];
export const parseChartData = () => (dispatch, getState) => {
const response = getState().comparison.data.data;
const isCompareSwitchChecked = getState().comparison.isCompareSwitchChecked;
const chartData = [];
let i = 0;

for (const run of response) {
const options = {
Expand Down Expand Up @@ -97,26 +98,130 @@ export const parseChartData = () => (dispatch, getState) => {
},
};

const datasets = [
{
label: run.instances[0].dataset_name,
data: run.instances.map((i) => i.time_taken),
backgroundColor: "#8BC1F7",
},
];

const datasets = [];
const data = {
labels: run.instances.map((i) => i.name),
labels: [...new Set(run.instances.map((i) => i.name))],
id: `${run.test_name}_${run.metrics_unit}`,
datasets,
};
const result = run.instances.reduce(function (r, a) {
r[a.dataset_name] = r[a.dataset_name] || [];
r[a.dataset_name].push(a);
return r;
}, Object.create(null));

for (const [key, value] of Object.entries(result)) {
console.log(key);
Copy link
Member

Choose a reason for hiding this comment

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

Is this a debugging statement you want to keep? 😆


const map = {};
for (const element of value) {
map[element.name] = element.time_taken.trim();
}
const mappedData = data.labels.map((label) => {
return map[label];
});
const obj = { label: key, backgroundColor: COLORS[i], data: mappedData };
i++;
Copy link
Member

Choose a reason for hiding this comment

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

You've got 5 COLORS values defined. Is it possible we could have more than 5 datasets selected for comparison? If so, we'll go to COLORS[5] and Javascript won't be happy with you. (If there's other code limiting the selection to 5, that's OK; but at least a comment would be good to clarify that.)

datasets.push(obj);
}

const obj = { options, data };
chartData.push(obj);
i = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Is this "reset" really necessary? Isn't i local to the function?

}
const type = isCompareSwitchChecked
? TYPES.SET_COMPARE_DATA
: TYPES.SET_PARSED_DATA;

dispatch({
type: TYPES.SET_PARSED_DATA,
type,
payload: chartData,
});
};

export const toggleCompareSwitch = () => (dispatch, getState) => {
dispatch({
type: TYPES.TOGGLE_COMPARE_SWITCH,
payload: !getState().comparison.isCompareSwitchChecked,
});
};
Comment on lines +142 to +147
Copy link
Member

Choose a reason for hiding this comment

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

toggleCompareSwitch doesn't need to return a thunk, it could just be the thunk, which would save a function call.

Actually, instead of returning a thunk, toggleCompareSwitch could return just the action, and the action doesn't need to contain a payload field because the reducer already has access to the old isCompareSwitchChecked value and it could calculate the new value from that.


export const setSelectedId = (isChecked, rId) => (dispatch, getState) => {
let selectedIds = [...getState().comparison.selectedResourceIds];
if (isChecked) {
selectedIds = [...selectedIds, rId];
} else {
selectedIds = selectedIds.filter((item) => item !== rId);
}
Comment on lines +150 to +155
Copy link
Member

Choose a reason for hiding this comment

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

Alternately, using just const's and a ternary 😉:

  const prev = getState().comparison.selectedResourceIds;
  const selectedIds = isChecked ? [...prev, rId] : prev.filter((id) => id !== rId);

dispatch({
type: TYPES.SET_SELECTED_RESOURCE_ID,
payload: selectedIds,
});
};
Comment on lines +149 to +160
Copy link
Member

Choose a reason for hiding this comment

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

Why is setSelectedId() a function which returns a thunk? What I think we actually want is a function which returns an action.

I think the reason you're using a thunk here is to get access to getState(). However, I think that's globally available, via useStore().getState(), or you can access the state via useSelector() (or, you can have the caller pass in the state).

So, instead of having the caller dispatch to a thunk which dispatches the action, this function can generate the action and the caller can dispatch it without needing the thunk.

The same comment applies to setChartModalContent().


export const compareMultipleDatasets = () => async (dispatch, getState) => {
Comment on lines +161 to +162
Copy link
Member

Choose a reason for hiding this comment

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

Again, why is this function returning a thunk instead of just being the thunk, which saves a function call.

try {
dispatch({ type: TYPES.LOADING });

const endpoints = getState().apiEndpoint.endpoints;
const selectedIds = [...getState().comparison.selectedResourceIds];
Comment on lines +166 to +167
Copy link
Member

Choose a reason for hiding this comment

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

Why is selectedIds a copy of the Redux store? We're not modifying it (are we?), so we can just refer to it directly and avoid the cost of the copy.


const params = new URLSearchParams();
params.append("datasets", selectedIds.toString());
const response = await API.get(
uriTemplate(endpoints, "datasets_compare", {}),
{ params }
);
if (response.status === 200 && response.data.json_data) {
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, I guess, since we really only expect 200 on success; but I still like the idea of using response.ok in general to test "success" except where we care about "alternate success" values. (And the only place the server does that deliberately is on UPLOAD/RELAY where 201/CREATED is distinct from 200/OK (duplicate).)

dispatch({
type: TYPES.SET_QUISBY_DATA,
payload: response.data.json_data,
});
dispatch({
type: TYPES.UNMATCHED_BENCHMARK_TYPES,
payload: "",
});
dispatch(parseChartData());
}
} catch (error) {
if (
error?.response?.data &&
error.response.data?.message
?.toLowerCase()
.includes("benchmarks must match")
) {
dispatch({
type: TYPES.UNMATCHED_BENCHMARK_TYPES,
payload: error.response.data.message,
});
} else {
dispatch(showToast(DANGER, ERROR_MSG));
}
dispatch({ type: TYPES.NETWORK_ERROR });
}
dispatch({ type: TYPES.COMPLETED });
};

export const setChartModalContent = (chartId) => (dispatch, getState) => {
const isCompareSwitchChecked = getState().comparison.isCompareSwitchChecked;
const data = isCompareSwitchChecked
? getState().comparison.compareChartData
: getState().comparison.chartData;

const activeChart = data.filter((item) => item.data.id === chartId)[0];

dispatch({
type: TYPES.SET_CURRENT_CHARTID,
payload: activeChart,
});
};

export const setChartModal = (isOpen) => ({
type: TYPES.SET_CHART_MODAL,
payload: isOpen,
});

export const setSearchValue = (value) => ({
type: TYPES.SET_SEARCH_VALUE,
payload: value,
});
7 changes: 7 additions & 0 deletions dashboard/src/actions/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,10 @@ export const SET_QUISBY_DATA = "SET_QUISBY_DATA";
export const SET_PARSED_DATA = "SET_PARSED_DATA";
export const SET_ACTIVE_RESOURCEID = "SET_ACTIVE_RESOURCEID";
export const IS_UNSUPPORTED_TYPE = "IS_UNSUPPORTED_TYPE";
export const TOGGLE_COMPARE_SWITCH = "TOGGLE_COMPARE_SWITCH";
export const SET_SELECTED_RESOURCE_ID = "SET_SELECTED_RESOURCE_ID";
export const UNMATCHED_BENCHMARK_TYPES = "UNMATCHED_BENCHMARK_TYPES";
export const SET_CHART_MODAL = "SET_CHART_MODAL";
export const SET_CURRENT_CHARTID = "SET_CURRENT_CHARTID";
export const SET_COMPARE_DATA = "SET_COMPARE_DATA";
export const SET_SEARCH_VALUE = "SET_SEARCH_VALUE";
96 changes: 41 additions & 55 deletions dashboard/src/modules/components/ComparisonComponent/ChartGallery.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,13 @@ import {
Title,
Tooltip,
} from "chart.js";
import {
Card,
EmptyState,
EmptyStateBody,
EmptyStateVariant,
Gallery,
GalleryItem,
} from "@patternfly/react-core";
import { Card, Gallery, GalleryItem } from "@patternfly/react-core";
import { setChartModal, setChartModalContent } from "actions/comparisonActions";

import { Bar } from "react-chartjs-2";
import { ExpandArrowsAltIcon } from "@patternfly/react-icons";
import React from "react";
import { useSelector } from "react-redux";
import { useDispatch } from "react-redux";

ChartJS.register(
BarElement,
Expand All @@ -28,55 +23,46 @@ ChartJS.register(
CategoryScale,
LinearScale
);
const EmptyStateExtraSmall = (props) => (
<EmptyState variant={EmptyStateVariant.xs}>
<div>{props.message}</div>
<EmptyStateBody>Benchmark type is currently unsupported!</EmptyStateBody>
</EmptyState>
);
const ChartGallery = () => {
const { chartData, unsupportedType } = useSelector(
(state) => state.comparison
);

const ChartGallery = (props) => {
const dispatch = useDispatch();

const handleExpandClick = (chartId) => {
dispatch(setChartModal(true));
dispatch(setChartModalContent(chartId));
};
return (
<>
{unsupportedType ? (
<EmptyStateExtraSmall message={unsupportedType} />
) : (
<>
{chartData && chartData.length > 0 && (
<Gallery
className="chart-wrapper"
hasGutter
minWidths={{
default: "100%",
md: "30rem",
xl: "35rem",
}}
maxWidths={{
md: "40rem",
xl: "1fr",
}}
>
{chartData.map((chart) => (
<Card
className="chart-card"
isRounded
isLarge
key={chart.data.id}
{props.dataToPlot && props.dataToPlot.length > 0 && (
<Gallery
className="chart-wrapper"
hasGutter
minWidths={{
default: "100%",
md: "30rem",
xl: "35rem",
}}
maxWidths={{
md: "40rem",
xl: "1fr",
}}
>
{props.dataToPlot.map((chart) => (
<Card className="chart-card" isRounded isLarge key={chart.data.id}>
<div className="expand-icon-container">
<div
className="icon-wrapper"
onClick={() => handleExpandClick(chart.data.id)}
>
<GalleryItem className="galleryItem chart-holder">
<Bar
options={chart.options}
data={chart.data}
width={450}
/>
</GalleryItem>
</Card>
))}
</Gallery>
)}
</>
<ExpandArrowsAltIcon />
</div>
</div>
<GalleryItem className="galleryItem chart-holder">
<Bar options={chart.options} data={chart.data} width={450} />
</GalleryItem>
</Card>
))}
</Gallery>
)}
</>
);
Expand Down
Loading