Checks results coming from Wanda #987

dottorblaster · 2022-11-15T17:26:29Z

As above, this PR introduces checks results coming from Wanda.

How was this tested?

Storybook, unit tests

dottorblaster · 2022-11-16T16:13:19Z

+555 ATM, sorry for this folks 🥲

arbulu89

Hey @dottorblaster
Long review hehe

Besides that, I find the next things missing. They can be worked in other PRs, but we should track them:

Update cluster details page checks result summary box
Improve the running view
Error handling

arbulu89 · 2022-11-17T07:39:26Z

assets/js/components/ChecksResults/CheckResult.jsx

      <ExecutionIcon executionState={executionState} health={health} />
+      {label}


As this is used in the old runner code as well, let's be sure that it doesn't impact the output

arbulu89 · 2022-11-17T08:10:34Z

assets/js/components/ExecutionResults/ExecutionResultsPage.jsx

+      executionID={executionID}
+      hostnames={hostnames}
+      clusterName={cluster?.name}
+      onCatalogRefresh={() => {


I don't think we need this anymore.
The new catalog doesn't have any difference based on provider.
In any case, in one of the draft PRs, the catalog can be updated using other UPDATE_CATALOG_NEW type.
PR

arbulu89 · 2022-11-17T08:13:29Z

assets/js/components/ExecutionResults/ExecutionResultsPage.jsx

+  return (
+    <ExecutionResults
+      clusterID={clusterID}
+      executionID={executionID}


Ok, I like the options of showing specific execution ids, but most probably, by default, we won't have the latest execution ids, so we would need to have some way of getting the latest execution for this cluster using the /api/checks/executions?group_id=#{clusterID} and fetching the latest one

/api/checks/executions?group_id=#{clusterID}&items_per_page=1 should work if they are ordered as expected (desc by date)

I checked Wanda code and it looks like they're not ordered https://github.com/trento-project/wanda/blob/main/lib/wanda/executions.ex#L47

Opening a PR for that if it's ok.

There you go trento-project/wanda#93

we won't have the latest execution ids

Absolutely agreed @arbulu89, I'll track the leftover as a subtask in the context of the same user story. We miss the piece where the dashboard actually asks Wanda about the executions.

arbulu89 · 2022-11-17T08:14:30Z

assets/js/trento.jsx

@@ -60,6 +61,10 @@ const App = () => {
                path="clusters/:clusterID/checks/results"
                element={<ChecksResults />}
              />
+              <Route
+                path="clusters/:clusterID/execution/:executionID"


Maybe better clusters/:clusterID/executions/:executionID?

We should as well be able to fetch the latest execution. Maybe we could have a "clusters/:clusterID/execution route for that

As above, we miss that part for now. Changing the route anyway 👍

arbulu89 · 2022-11-17T08:15:46Z

assets/js/components/ExecutionResults/ExecutionResultsPage.jsx

+  const hostnames = useSelector((state) =>
+    state.hostsList.hosts.map(({ id, hostname }) => ({ id, hostname }))
+  );
+  const cluster = useSelector((state) =>


We already have the getCluster selector exactly for this

assets/js/components/ExecutionResults/ExecutionResults.jsx

arbulu89 · 2022-11-17T08:50:16Z

assets/js/lib/test-utils/factories.js

+    group_id: groupID,
+    result: resultEnum(),
+    started_at: '2022-11-09T15:11:31.436586Z',
+    status: faker.helpers.arrayElement(['running', 'completed']),


statusEnum maybe?

arbulu89 · 2022-11-17T08:51:21Z

assets/js/lib/test-utils/factories.js

+    started_at: '2022-11-09T15:11:31.436586Z',
+    status: faker.helpers.arrayElement(['running', 'completed']),
+    timeout: [],
+    check_results: [


I see a lot of hardcoded values here. Couldn't we use faker to populate them?
Besides that, it would even make sense to have smaller factory functions to build the whole result, as it would give more fleixibility

assets/js/components/ChecksResults/checksUtils.js

arbulu89 · 2022-11-17T09:00:00Z

assets/js/components/ExecutionResults/ExecutionResults.stories.jsx

@@ -0,0 +1,154 @@
+import React from 'react';


You need to explain us how this works hehe
It looks super powerful

I will! I promise 😅

CDimonaco · 2022-11-17T11:00:49Z

assets/js/components/ExecutionResults/ExecutionResults.jsx

+              reachable
+              unreachableMessage=""
+            >
+              {checks.map((checkID) => {


Maybe we can extract that logic, inside the arrow function, inside another component? Seems a little bit convoluted to follow

nelsonkopliku

Tricky one 😄

WANDA_URL=http://localhost:4001 iex -S mix phx.server helped making it run, plus adjusting wanda to make them click.

Then playing a bit with the API I got this result.

With the right set of data I could get a clue of what the output was.

We'll need to iterate on, as usual.

assets/js/components/ExecutionResults/ExecutionResults.jsx

nelsonkopliku · 2022-11-17T17:20:18Z

assets/js/components/ChecksResults/checksUtils.js

+
+export const getHealth = (checkResults, checkID, agentID) => {
+  const checkResult = checkResults.find(({ check_id }) => check_id === checkID);
+  if (checkResult) {


What about an early return here?

1 for using a guard here if possible

nelsonkopliku · 2022-11-17T17:31:53Z

assets/js/components/ChecksResults/checksUtils.js

+
+    const failedExpectationEvaluations =
+      agentCheckResult?.expectation_evaluations.filter(
+        (expectationEvaluation) => 'message' in expectationEvaluation


I am wondering whether we can look for the type property.

And I am now realizing our API doc needs improvement in documenting the error scenarios. I'll track it as debt and tackle asap.

I think that we need to add a type field to the schema that could be error or result so we can do this type matching here. Then rename the existing type fields in error_type and expectation_type. This could be done when mapping in the phoenix views, but we need a way to differentiate the oneof basically.

Mapping on the shape looks prone to error in the long run.

P.S. This is good for the first iteration, but please let's keep this on the radar.

Also, you need to filter out the expect_sameexpectations till we don't have the new UI or they will show as passing always, and I'd rather to remove them from the count for the first iteration.

dottorblaster · 2022-11-22T17:19:13Z

@arbulu89 @fabriziosestito @CDimonaco @nelsonkopliku I think I addressed it all, could you see if I missed something?

arbulu89

@dottorblaster So far so good.
I think we can merge and iterate on the evolution!

arbulu89 · 2022-11-23T07:53:00Z

assets/js/components/ChecksResults/checksUtils.js

+  };
+};
+
+export const getCheckDescription = (catalog, checkID) =>


At the end this is "custom" option for findCheck in this file.
We could have simply find.Check(catalog, checkID)?.description I guess.
But well, maybe this is more descritive

No, you're actually right

arbulu89 · 2022-11-23T07:56:57Z

assets/js/components/ChecksResults/checksUtils.js

+  return checkResults.map(({ check_id }) => check_id);
+};
+
+export const getHealth = (checkResults, checkID, agentID) => {


Maybe rename this to getCheckHealthByAgent or something ?
By the way, he I hate that we should the check state by agent...
It breaks all the logic of our execution payload.
Waiting to update this, we wouldn't need this kind of evaluation functions this way...

I hate that too 😄 looking forward to a new UX!

arbulu89 · 2022-11-23T08:15:26Z

assets/js/components/ExecutionResults/ExecutionResults.jsx

+  return (
+    <div>
+      <Modal
+        title={catalog?.find(({ id }) => id === selectedCheck)?.description}


We should use the new getCheckDescription here

arbulu89 · 2022-11-23T08:15:42Z

assets/js/components/ExecutionResults/ExecutionResults.jsx

+        onClose={() => setModalOpen(false)}
+      >
+        <ReactMarkdown className="markdown" remarkPlugins={[remarkGfm]}>
+          {catalog?.find(({ id }) => id === selectedCheck)?.description}


The same, use getCheckDescription here

arbulu89 · 2022-11-23T08:16:45Z

assets/js/components/ExecutionResults/ExecutionResults.jsx

+                  <CheckResult
+                    key={checkID}
+                    checkId={checkID}
+                    description={


getCheckDescription

nelsonkopliku

Looks like a good starting point to iterate on, indeed.

dottorblaster · 2022-11-23T16:08:36Z

At +666 I'd like to merge and call this PR THE PULL REQUEST OF THE BEAST

dottorblaster added the enhancement New feature or request label Nov 15, 2022

dottorblaster marked this pull request as ready for review November 16, 2022 15:56

dottorblaster requested review from arbulu89, CDimonaco, EMaksy, fabriziosestito and jamie-suse November 16, 2022 15:56

dottorblaster force-pushed the execution-page branch 2 times, most recently from b5e60a3 to 3a326ce Compare November 16, 2022 16:05

arbulu89 reviewed Nov 17, 2022

View reviewed changes

CDimonaco reviewed Nov 17, 2022

View reviewed changes

nelsonkopliku reviewed Nov 17, 2022

View reviewed changes

dottorblaster force-pushed the execution-page branch from 2c5a8d7 to d7e6a97 Compare November 22, 2022 17:09

arbulu89 approved these changes Nov 23, 2022

View reviewed changes

nelsonkopliku approved these changes Nov 23, 2022

View reviewed changes

dottorblaster added 13 commits November 23, 2022 16:41

Add a new page for the new checks results visualization

4516129

Add new aliases to Storybook config

f62a20a

Add ExecutionResults component

faf3bb2

Export some legacy components to be reused in ExecutionResults

4615e22

Wire up exection results page

9cf713c

Add wanda API client

4cf56da

Refactor utility functions inside checks result module

2a9c482

Add labels to checks results

766d4a6

Add tests to ExecutuionResults

e285112

Address the majority of the comments

be24d97

Cover checksUtils with tests

19e8155

Filter away expect_same expectations in the checks count

c3e7f99

Externalize label computing into a function

5c9f8f5

dottorblaster added 2 commits November 23, 2022 16:41

Add more tests, use getCheckDescription

c9a1926

Refactor getCheckDescription to use findCheck

6bda1f8

dottorblaster force-pushed the execution-page branch from 3df8301 to 6bda1f8 Compare November 23, 2022 15:42

dottorblaster merged commit e0aae7b into main Nov 23, 2022

dottorblaster deleted the execution-page branch November 23, 2022 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checks results coming from Wanda #987

Checks results coming from Wanda #987

dottorblaster commented Nov 15, 2022 •

edited

dottorblaster commented Nov 16, 2022

arbulu89 left a comment

arbulu89 Nov 17, 2022

arbulu89 Nov 17, 2022

arbulu89 Nov 17, 2022

fabriziosestito Nov 18, 2022

nelsonkopliku Nov 18, 2022

nelsonkopliku Nov 18, 2022

dottorblaster Nov 18, 2022

arbulu89 Nov 17, 2022

dottorblaster Nov 18, 2022

arbulu89 Nov 17, 2022

arbulu89 Nov 17, 2022

arbulu89 Nov 17, 2022

arbulu89 Nov 17, 2022

dottorblaster Nov 22, 2022

CDimonaco Nov 17, 2022

nelsonkopliku left a comment

nelsonkopliku Nov 17, 2022

fabriziosestito Nov 18, 2022

nelsonkopliku Nov 17, 2022

fabriziosestito Nov 18, 2022 •

edited

fabriziosestito Nov 18, 2022

dottorblaster commented Nov 22, 2022

arbulu89 left a comment

arbulu89 Nov 23, 2022

dottorblaster Nov 23, 2022

arbulu89 Nov 23, 2022

dottorblaster Nov 23, 2022

arbulu89 Nov 23, 2022

arbulu89 Nov 23, 2022

arbulu89 Nov 23, 2022

nelsonkopliku left a comment

dottorblaster commented Nov 23, 2022

		<ExecutionIcon executionState={executionState} health={health} />
		{label}

Checks results coming from Wanda #987

Checks results coming from Wanda #987

Conversation

dottorblaster commented Nov 15, 2022 • edited

How was this tested?

dottorblaster commented Nov 16, 2022

arbulu89 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

nelsonkopliku 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

fabriziosestito Nov 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottorblaster commented Nov 22, 2022

arbulu89 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

nelsonkopliku left a comment

Choose a reason for hiding this comment

dottorblaster commented Nov 23, 2022

dottorblaster commented Nov 15, 2022 •

edited

fabriziosestito Nov 18, 2022 •

edited