feat(jmc-agent): add JMC Agent probes UI

[Josh-Matsuoka]

This PR implements the frontend of the Agent Plugin, adding a page with a table displaying currently available probe templates and an option to upload more and insert them into the target, as well as a page displaying any currently active probes with options to deactivate them.

Currently resolving a problem with the tests, likely an issue with how I've set up the mocks since the mocked probe templates don't seem to be making it to the page (i.e. it's getting stuck at the loading screen in tests), will keep this in draft for the moment until that's resolved.

Depends on https://github.com/cryostatio/cryostat/pull/1126

[andrewazores]

@maxcao13 or @tthvo if you have time, could you take a look at this and help Josh figure out the tests?

[maxcao13]

if you have time, could you take a look at this and help Josh figure out the tests?

Sure, can take a look at them. It also seems like yarn was updated in this pr as well, not sure if that's intended as the lockfile should be updated.

[andrewazores]

I think the yarn update should be backed out from this patch set.

[maxcao13]

I think the problem is that in handleTemplates on line 94 in the AgentProbeTemplates, JSON.parse is called but the subscription value that gets passed to the function is type of array (i.e. context.api.getProbeTemplates returns Observable<ProbeTemplate>[]) , and for some reason it doesn't log an error. I checked this by putting consolelog statements before and after the parse calling. And then the error propagates and setIsLoading is never called to set the state to false ever again when a notification comes.

JSON.parse should be able to parse arrays, but for some reason nothing happens after that call which is weird.

EDIT: Actually I realize, there is no need to call JSON.parse since we are just passing the templates directly. Plus JSON.parse needs an argument of string. Should work after just removing that call.

[tthvo]

Right, just also checked to confirm Max's solution should solve the loading error. Other tests failures:

• Tests involving template deletion: this checks if warning modal is open. However, deletion warning modal is not set up for this agent view, so we need to set up before checks.
• Tests involving notifcations:
  • There should be only 2 mock returns for each tests and order matters.
  • For create notification, there is an extra third mock call.
  • For delete notifcation, the order is inverted (i.e. delete must come second).
  • Currently, jest.spyOn(defaultServices.api, 'getProbeTemplates') only mock returns one template. This should adjust based on tests (i.e. create notifcation test should mock this method return 2 templates) since notification update call refreshTemplate. A better approach is to just patch the component state with new template instead of full query.
• Others: The search texts should be corrected to someProbeTemplate/anotherProbeTemplate as in mocks.

Hope it solved the issue :D

[Josh-Matsuoka]

Managed to get the tests fixed, thanks for the help :) This is ready for review now

[andrewazores]

Following the setup steps and example probe template from here: https://github.com/cryostatio/cryostat/pull/1126#issuecomment-1285681149

The workflow seems to work as anticipated, but the notification that appears when inserting probes has undefined in it:

And also, the deletion prompt for a probe template is titled as "Event Template":

I think just these few strings need to be updated and then this looks OK. I would also like to rename the main menu item from "Agent" to something like "JMC Agent", or perhaps even move this view content under the "Event" main menu item rather than having it as a top-level view. I can imagine that a main menu item named "Agent" would be useful for when we have a Cryostat Agent as well.

[andrewazores]

Here's my concept for just moving the new JMC agent-related views under the existing Events nav item:

I just removed the top-level nav item and moved the Card component inside Events, wrapping the now two cards together in Stack and StackItem components.

[Josh-Matsuoka]

I've pushed some changes to relabel the menu item to JMC Agent, if you'd prefer to have it nested under events I can look into doing that as well. I've also fixed the notifications and the deletion warning

[andrewazores]

diff --git a/src/app/Agent/Agent.tsx b/src/app/Agent/Agent.tsx
index 0cb423e..94f3e93 100644
--- a/src/app/Agent/Agent.tsx
+++ b/src/app/Agent/Agent.tsx
@@ -51,18 +51,6 @@ export const Agent = () => {
   return (
     <>
       <TargetView pageTitle="JMC Agent">
-        <Card>
-          <CardBody>
-            <Tabs activeKey={activeTab} onSelect={handleTabSelect}>
-              <Tab eventKey={0} title="Probe Templates">
-                <AgentProbeTemplates />
-              </Tab>
-              <Tab eventKey={1} title="Live Configuration">
-                <AgentLiveProbes />
-              </Tab>
-            </Tabs>
-          </CardBody>
-        </Card>
       </TargetView>
     </>
   );
diff --git a/src/app/Events/Events.tsx b/src/app/Events/Events.tsx
index 07146ba..7041050 100644
--- a/src/app/Events/Events.tsx
+++ b/src/app/Events/Events.tsx
@@ -37,9 +37,11 @@
  */
 import * as React from 'react';
 import { TargetView } from '@app/TargetView/TargetView';
-import { Card, CardBody, Tab, Tabs } from '@patternfly/react-core';
+import { Card, CardBody, Stack, StackItem, Tab, Tabs } from '@patternfly/react-core';
 import { EventTemplates } from './EventTemplates';
 import { EventTypes } from './EventTypes';
+import { AgentProbeTemplates } from '@app/Agent/AgentProbeTemplates';
+import { AgentLiveProbes } from '@app/Agent/AgentLiveProbes';
 
 export const Events = () => {
   const [activeTab, setActiveTab] = React.useState(0);
@@ -51,18 +53,36 @@ export const Events = () => {
   return (
     <>
       <TargetView pageTitle="Events">
-        <Card>
-          <CardBody>
-            <Tabs activeKey={activeTab} onSelect={handleTabSelect}>
-              <Tab eventKey={0} title="Event Templates">
-                <EventTemplates />
-              </Tab>
-              <Tab eventKey={1} title="Event Types">
-                <EventTypes />
-              </Tab>
-            </Tabs>
-          </CardBody>
-        </Card>
+        <Stack hasGutter>
+          <StackItem>
+            <Card>
+              <CardBody>
+                <Tabs activeKey={activeTab} onSelect={handleTabSelect}>
+                  <Tab eventKey={0} title="Event Templates">
+                    <EventTemplates />
+                  </Tab>
+                  <Tab eventKey={1} title="Event Types">
+                    <EventTypes />
+                  </Tab>
+                </Tabs>
+              </CardBody>
+            </Card>
+          </StackItem>
+          <StackItem>
+            <Card>
+              <CardBody>
+                <Tabs activeKey={activeTab} onSelect={handleTabSelect}>
+                  <Tab eventKey={0} title="Probe Templates">
+                    <AgentProbeTemplates />
+                  </Tab>
+                  <Tab eventKey={1} title="Live Configuration">
+                    <AgentLiveProbes />
+                  </Tab>
+                </Tabs>
+              </CardBody>
+            </Card>
+          </StackItem>
+        </Stack>
       </TargetView>
     </>
   );
diff --git a/src/app/routes.tsx b/src/app/routes.tsx
index acbef47..4ca2ac6 100644
--- a/src/app/routes.tsx
+++ b/src/app/routes.tsx
@@ -39,7 +39,6 @@ import * as React from 'react';
 import { CreateRecording } from '@app/CreateRecording/CreateRecording';
 import { Dashboard } from '@app/Dashboard/Dashboard';
 import { Events } from '@app/Events/Events';
-import { Agent } from '@app/Agent/Agent';
 import { Login } from '@app/Login/Login';
 import { NotFound } from '@app/NotFound/NotFound';
 import { Recordings } from '@app/Recordings/Recordings';
@@ -147,14 +146,6 @@ const routes: IAppRoute[] = [
     description: 'View available JFR event templates and types for target JVMs, as well as upload custom templates.',
     navGroup: CONSOLE,
   },
-  {
-    component: Agent,
-    exact: true,
-    label: 'Agent',
-    path: '/agent',
-    title: 'Agent',
-    navGroup: CONSOLE,
-  },
   {
     component: SecurityPanel,
     exact: true,

This is the patch of changes I made to nest the new card under the Events view. I think I'd prefer to go that route, at least for now. If there is more JMC Agent specific content in the future then we can create the JMC Agent menu item and move all the related content under there if it makes more sense.

I think it would also be good to add some link or some descriptive text somewhere here that also helps indicate to the user what exactly these new views allow them to do. Having it under a JMC Agent menu perhaps gives the user at least a clue of what to search for, or maybe they're even already familiar with it, but for novice users I don't know that it does much good. Putting it under Events gives some contextual clue about the intent and capability of the view so it fits into the Cryostat story better but now there's no indication at all for the user what exactly this view does, what the prerequisites are, what the expected format of the XML template files is (the upload prompt gives some idea by referencing JMC at least), etc.

[andrewazores]

Finally, this is what it looks like when you connect to a target that doesn't have the JMC Agent loaded and visit these new views (whether they're under Events or JMC Agent menu items):

The server response code should probably not be 500 here but rather one of 501, 502, or 503 - each seems justifiable in different ways. When the frontend encounters a 501/502/503 on this request it should not display the error notification, just use it to display the error placeholder view. That placeholder should probably have some more details about the cause of the failure too, or likely causes, like the JMC Agent not being installed or running within the selected target application.

[andrewazores]

Ah perfect, I was just writing a comment to ask about that work when that commit got pushed.

[tthvo]

Should also add another test for ProbeRemoves notification?

[Josh-Matsuoka]

ah I think I missed that in my commit, give me a sec

[tthvo]

We might need to disable the remove button if there are no probes. Right now, users can click and make requests successfully.

[andrewazores]

I made one more tiny enhancement in my last commit. Is everyone happy with this patchset?

[Josh-Matsuoka]

Looks good. Thanks for the help with the cleanup and implementing changes from the review.

[tthvo]

Looks good to me! Just missing some tests for inserting probes button. I will add it.