Generate Comparison Charts #3500

MVarshini · 2023-07-24T06:49:54Z

PBENCH-1208

Generate Comparison Charts

Generate Comparison Charts zoom feature modal data change

dbutenhof

This mostly looks OK, but I think the handling of the search box (e.g., implicitly accepting regex but quietly quoting all the special characters on a compilation failure) needs some clarification. I've got a few other minor comments, too, including a debugging console.log which should probably be removed. 😁

dbutenhof · 2023-07-24T11:33:00Z

dashboard/src/actions/comparisonActions.js

+    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")
+    ) {
+      dispatch({
+        type: TYPES.IS_UNSUPPORTED_TYPE,
+        payload: error.response.data.message,
+      });


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?

dbutenhof · 2023-07-24T11:34:28Z

dashboard/src/actions/comparisonActions.js

+    }, Object.create(null));
+
+    for (const [key, value] of Object.entries(result)) {
+      console.log(key);


Is this a debugging statement you want to keep? 😆

dbutenhof · 2023-07-24T11:36:26Z

dashboard/src/actions/comparisonActions.js


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


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

dbutenhof · 2023-07-24T11:39:27Z

dashboard/src/actions/comparisonActions.js

+        return map[label];
+      });
+      const obj = { label: key, backgroundColor: COLORS[i], data: mappedData };
+      i++;


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.)

dbutenhof · 2023-07-24T11:43:23Z

dashboard/src/actions/comparisonActions.js

+      uriTemplate(endpoints, "datasets_compare", {}),
+      { params }
+    );
+    if (response.status === 200 && response.data.json_data) {


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).)

dbutenhof · 2023-07-24T13:23:43Z

dashboard/src/modules/components/ComparisonComponent/PanelContent.jsx

+        input = new RegExp(searchValue, "i");
+      } catch (err) {
+        input = new RegExp(
+          searchValue.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"),


So you're allowing RegEx characters in the user's filter string, but if it doesn't compile you're quoting all special characters and compiling again? I'm not sure this is what I'd want.

If I use regex characters accidentally, quoting them is good, but probably not as a fallback. That is, if I didn't intentionally construct a regex, then the results of AB*C will surprise me;

If I construct a regex but get it wrong, quoting all special characters will probably still surprise me.

I think that if we're not going to allow regex input, then you should quote (or reject) special characters up front (or simply use a non-regex filter), and if we intentionally allow regex then a compile failure should be an error visible to the user so they can retry.

I also noticed that in the demos you were using this to find the uperf benchmark runs. As mentioned earlier, using the name is unreliable here: it'd be better to search for an exact match in server.benchmark... although I can imagine users finding name search useful as well, I suspect that in general they'll need a lot more.

I don't think we want to get into this for this PR, but something to keep in mind is that I think perhaps in the longer run what we want is a general search component for datasets that you can use on each page that allows filtering by wildcard (regex?) name match or by arbitrary metadata. That is, when I get to the comparison I probably don't want to see all my/public uperf but rather the set of uperf data I generated for a specific purpose, which might be based on my config string or a global.lightspeed.run value or something.

I don't think users (myself included) will expect a search box to automatically accept a regex unless it is labeled as doing so. Moreover, lots of people are unfamiliar with regex's, and they are going to be surprised if their search string looks like myresult_[2023-7-9].txt and it matches "myresult_80txt".

Perhaps you could have a checkbox or switch which enables regex search? With that "off", there will be no surprises, and with it "on" you can report compilation errors when they occur, which will also prevent surprises.

Also, why are you (implicitly) making the regex case-blind? That seems likely to incur additional surprises.

webbnh

I didn't find anything worth blocking the merge for, but I've pointed out a few code quality issues that you might want to consider addressing.

webbnh · 2023-07-25T01:36:18Z

dashboard/src/actions/comparisonActions.js

+    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")
+    ) {


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.

webbnh · 2023-07-25T02:00:15Z

dashboard/src/actions/comparisonActions.js

+export const toggleCompareSwitch = () => (dispatch, getState) => {
+  dispatch({
+    type: TYPES.TOGGLE_COMPARE_SWITCH,
+    payload: !getState().comparison.isCompareSwitchChecked,
+  });
+};


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.

webbnh · 2023-07-25T15:03:47Z

dashboard/src/actions/comparisonActions.js

+  let selectedIds = [...getState().comparison.selectedResourceIds];
+  if (isChecked) {
+    selectedIds = [...selectedIds, rId];
+  } else {
+    selectedIds = selectedIds.filter((item) => item !== rId);
+  }


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

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

webbnh · 2023-07-25T15:25:46Z

dashboard/src/actions/comparisonActions.js

+export const setSelectedId = (isChecked, rId) => (dispatch, getState) => {
+  let selectedIds = [...getState().comparison.selectedResourceIds];
+  if (isChecked) {
+    selectedIds = [...selectedIds, rId];
+  } else {
+    selectedIds = selectedIds.filter((item) => item !== rId);
+  }
+  dispatch({
+    type: TYPES.SET_SELECTED_RESOURCE_ID,
+    payload: selectedIds,
+  });
+};


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().

webbnh · 2023-07-25T15:27:12Z

dashboard/src/actions/comparisonActions.js

+
+export const compareMultipleDatasets = () => async (dispatch, getState) => {


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

webbnh · 2023-07-25T16:32:03Z

dashboard/src/modules/components/ComparisonComponent/ChartModal.jsx

+              width={750}
+              height={450}


Are these absolute (e.g., pixel) values? Don't we want the size relative to the width of the chart/window/screen?

webbnh · 2023-07-25T16:46:47Z

dashboard/src/modules/components/ComparisonComponent/PanelContent.jsx

+        input = new RegExp(searchValue, "i");
+      } catch (err) {
+        input = new RegExp(
+          searchValue.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"),


I don't think users (myself included) will expect a search box to automatically accept a regex unless it is labeled as doing so. Moreover, lots of people are unfamiliar with regex's, and they are going to be surprised if their search string looks like myresult_[2023-7-9].txt and it matches "myresult_80txt".

Perhaps you could have a checkbox or switch which enables regex search? With that "off", there will be no surprises, and with it "on" you can report compilation errors when they occur, which will also prevent surprises.

webbnh · 2023-07-25T16:49:00Z

dashboard/src/modules/components/ComparisonComponent/PanelContent.jsx

+        input = new RegExp(searchValue, "i");
+      } catch (err) {
+        input = new RegExp(
+          searchValue.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"),


Also, why are you (implicitly) making the regex case-blind? That seems likely to incur additional surprises.

webbnh · 2023-07-25T16:56:16Z

dashboard/src/modules/components/ComparisonComponent/PanelContent.jsx

+      }
+      return item.name.search(input) >= 0;


The docs suggest that, since you want a boolean result, you should use RegExp.test() instead of search():

return input.test(item.name)

webbnh · 2023-07-25T16:59:20Z

dashboard/src/modules/components/ComparisonComponent/PanelContent.jsx

+              {filteredDatasets.map((item) => {
+                return (


I don't think you need a code blob here, since all it does is return an expression -- you can just have the arrow function evaluate to the expression (analogous to what you've done at lines 42 and 44).

dbutenhof

I would prefer the compare pane search be cleaned up, but let's move this forward so we can stage it soon.

MVarshini added 2 commits July 24, 2023 12:10

PBENCH-1208

9fcbb11

Generate Comparison Charts zoom feature modal data change

remove the commented lines

3062e84

MVarshini requested review from ndokos, webbnh and dbutenhof July 24, 2023 06:49

unused imports

c46aed1

dbutenhof requested changes Jul 24, 2023

View reviewed changes

webbnh approved these changes Jul 25, 2023

View reviewed changes

dbutenhof approved these changes Jul 26, 2023

View reviewed changes

dbutenhof merged commit e119384 into distributed-system-analysis:main Jul 26, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate Comparison Charts #3500

Generate Comparison Charts #3500

MVarshini commented Jul 24, 2023

dbutenhof left a comment

dbutenhof Jul 24, 2023

dbutenhof Jul 24, 2023

dbutenhof Jul 24, 2023

dbutenhof Jul 24, 2023

dbutenhof Jul 24, 2023

dbutenhof Jul 24, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh left a comment

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

webbnh Jul 25, 2023

dbutenhof left a comment


		export const compareMultipleDatasets = () => async (dispatch, getState) => {

Generate Comparison Charts #3500

Generate Comparison Charts #3500

Conversation

MVarshini commented Jul 24, 2023

dbutenhof 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

webbnh 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

dbutenhof left a comment

Choose a reason for hiding this comment