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.

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

feat(jmcagent): JMC Agent Feature Parity #375

Merged
merged 20 commits into from
Apr 19, 2024

Conversation

Josh-Matsuoka
Copy link
Contributor

@Josh-Matsuoka Josh-Matsuoka commented Apr 16, 2024

Fixes #14
Depends on cryostatio/cryostat-core#373
Depends on #385

This PR re-implements the JMC Agent functionality for Cryostat 3.0.

I took inspiration from how the event templates system was rewritten for rewriting the probe template service.

Opening as draft for the moment as it doesn't quite run properly yet and I still need to re add the notifications. There will likely need to be some cleanup done in core as well.

@github-actions github-actions bot added the needs-triage Needs thorough attention from code reviewers label Apr 16, 2024
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@andrewazores andrewazores added feat New feature or request safe-to-test and removed needs-triage Needs thorough attention from code reviewers labels Apr 17, 2024
@Josh-Matsuoka
Copy link
Contributor Author

Probe Template handlers work, tested via

./mvnw quarkus:dev
curl -F [email protected] -X POST http://localhost:8181/api/v3/probes/template2
curl http://localhost:8181/api/v3/probes
curl -X DELETE http://localhost:8181/api/v3/probes/template2
curl http://localhost:8181/api/v3/probes

Just need to test with a target with the agent deployed to make sure insertion/removal of probes works as expected. (Testing without an active agent throws the expected error since the AgentController bean isn't registered)

@Josh-Matsuoka
Copy link
Contributor Author

Probe handlers work as expected

curl -F [email protected] -X POST http://localhost:8181/api/v3/probes/template2
curl -X POST http://localhost:8181/api/v3/targets/1/probes/template2
curl http://localhost:8181/api/v3/targets/1/probes
[{"id":"demo.jfr.test1","name":"JFR Hello World Event 1 %TEST_NAME%","clazz":"io.cryostat.jmcagent.S3ProbeTemplateService","description":"Defined in the xml file and added by the agent.","path":"demo/jfrhelloworldevent1","recordStackTrace":true,"useRethrow":false,"methodName":"deleteTemplate","methodDescriptor":"(Ljava/lang/String;)V","location":"WRAP","returnValue":null}]

web UI still isn't quite working, looking at the network requests they're succeeding and returning the expected results, but the card shows an error "e.data is undefined"

@Josh-Matsuoka Josh-Matsuoka marked this pull request as ready for review April 18, 2024 19:11
@Josh-Matsuoka Josh-Matsuoka changed the title feat(jmcagent) JMC Agent Feature Parity feat(jmcagent): JMC Agent Feature Parity Apr 18, 2024
@andrewazores
Copy link
Member

/build_test

Copy link

Workflow started at 4/18/2024, 3:28:30 PM. View Actions Run.

Copy link

OpenAPI schema change detected:

diff --git a/schema/openapi.yaml b/schema/openapi.yaml
index b7db9ef..ae68add 100644
--- a/schema/openapi.yaml
+++ b/schema/openapi.yaml
@@ -1235,20 +1235,60 @@ paths:
             application/json:
               schema:
                 additionalProperties:
                   additionalProperties:
                     type: string
                   type: object
                 type: object
           description: OK
       tags:
         - Discovery
+  /api/v2/probes:
+    get:
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
+  /api/v2/probes/{probeTemplateName}:
+    delete:
+      parameters:
+        - in: path
+          name: probeTemplateName
+          required: true
+          schema:
+            type: string
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
+    post:
+      parameters:
+        - in: path
+          name: probeTemplateName
+          required: true
+          schema:
+            type: string
+      requestBody:
+        content:
+          application/x-www-form-urlencoded:
+            schema:
+              properties:
+                probeTemplate:
+                  $ref: '#/components/schemas/FileUpload'
+              type: object
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
   /api/v2/rules:
     get:
       responses:
         "200":
           content:
             application/json:
               schema:
                 $ref: '#/components/schemas/V2Response'
           description: OK
         "401":
@@ -1512,20 +1552,66 @@ paths:
                 $ref: '#/components/schemas/V2Response'
           description: OK
         "401":
           description: Not Authorized
         "403":
           description: Not Allowed
       security:
         - SecurityScheme: []
       tags:
         - Events
+  /api/v2/targets/{connectUrl}/probes:
+    delete:
+      parameters:
+        - in: path
+          name: connectUrl
+          required: true
+          schema:
+            format: uri
+            type: string
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
+    get:
+      parameters:
+        - in: path
+          name: connectUrl
+          required: true
+          schema:
+            format: uri
+            type: string
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
+  /api/v2/targets/{connectUrl}/probes/{probeTemplateName}:
+    post:
+      parameters:
+        - in: path
+          name: connectUrl
+          required: true
+          schema:
+            format: uri
+            type: string
+        - in: path
+          name: probeTemplateName
+          required: true
+          schema:
+            type: string
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
   /api/v2/targets/{connectUrl}/snapshot:
     post:
       parameters:
         - in: path
           name: connectUrl
           required: true
           schema:
             format: uri
             type: string
       responses:
@@ -1716,20 +1802,64 @@ paths:
                 type: string
           description: OK
         "401":
           description: Not Authorized
         "403":
           description: Not Allowed
       security:
         - SecurityScheme: []
       tags:
         - Recordings
+  /api/v3/probes:
+    get:
+      responses:
+        "200":
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/V2Response'
+          description: OK
+      tags:
+        - JMC Agent
+  /api/v3/probes/{probeTemplateName}:
+    delete:
+      parameters:
+        - in: path
+          name: probeTemplateName
+          required: true
+          schema:
+            type: string
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
+    post:
+      parameters:
+        - in: path
+          name: probeTemplateName
+          required: true
+          schema:
+            type: string
+      requestBody:
+        content:
+          application/x-www-form-urlencoded:
+            schema:
+              properties:
+                probeTemplate:
+                  $ref: '#/components/schemas/FileUpload'
+              type: object
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
   /api/v3/reports/{encodedKey}:
     get:
       parameters:
         - in: path
           name: encodedKey
           required: true
           schema:
             type: string
       responses:
         "200":
@@ -1889,20 +2019,70 @@ paths:
                 type: array
           description: OK
         "401":
           description: Not Authorized
         "403":
           description: Not Allowed
       security:
         - SecurityScheme: []
       tags:
         - Events
+  /api/v3/targets/{id}/probes:
+    delete:
+      parameters:
+        - in: path
+          name: id
+          required: true
+          schema:
+            format: int64
+            type: integer
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
+    get:
+      parameters:
+        - in: path
+          name: id
+          required: true
+          schema:
+            format: int64
+            type: integer
+      responses:
+        "200":
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/V2Response'
+          description: OK
+      tags:
+        - JMC Agent
+  /api/v3/targets/{id}/probes/{probeTemplateName}:
+    post:
+      parameters:
+        - in: path
+          name: id
+          required: true
+          schema:
+            format: int64
+            type: integer
+        - in: path
+          name: probeTemplateName
+          required: true
+          schema:
+            type: string
+      responses:
+        "200":
+          description: OK
+      tags:
+        - JMC Agent
   /api/v3/targets/{id}/recordingOptions:
     get:
       parameters:
         - in: path
           name: id
           required: true
           schema:
             format: int64
             type: integer
       responses:

Copy link

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8743608495

@andrewazores
Copy link
Member

No luck with a newer Agent version (1.0.1-SNAPSHOT just built from master), still getting the class file version 61 error. Any advice?

@Josh-Matsuoka
Copy link
Contributor Author

Josh-Matsuoka commented Apr 19, 2024

No luck with a newer Agent version (1.0.1-SNAPSHOT just built from master), still getting the class file version 61 error. Any advice?

You need to make sure it builds with 17 and bump the asm version up to 9.1, try the following patch for the agent pom.xml:

diff --git a/agent/pom.xml b/agent/pom.xml
index b7cbefe5..2ab94cad 100644
--- a/agent/pom.xml
+++ b/agent/pom.xml
@@ -74,8 +74,8 @@
                <changelist>-SNAPSHOT</changelist>
                <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
                <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
-               <maven.compiler.source>11</maven.compiler.source>
-               <maven.compiler.target>11</maven.compiler.target>
+               <maven.compiler.source>17</maven.compiler.source>
+               <maven.compiler.target>17</maven.compiler.target>
                <jmc.config.path>${project.basedir}/../configuration</jmc.config.path>
                <!-- Plugin Versions -->
                <maven.checkstyle.version>3.3.0</maven.checkstyle.version>
@@ -91,7 +91,7 @@
                <nexus.staging.plugin.version>1.6.13</nexus.staging.plugin.version>
                <maven.gpg.version>3.1.0</maven.gpg.version>
                <!-- Dependency Versions -->
-               <asm.version>8.0.1</asm.version>
+               <asm.version>9.1</asm.version>
                <junit.version>4.13.2</junit.version>
        </properties>
        <scm>
@@ -220,7 +220,7 @@
                                                                <Agent-Class>org.openjdk.jmc.agent.Agent</Agent-Class>
                                                                <Premain-Class>org.openjdk.jmc.agent.Agent</Premain-Class>
                                                                <Can-Retransform-Classes>true</Can-Retransform-Classes>
-                                                               <Bundle-RequiredExecutionEnvironment>JavaSE-11</Bundle-RequiredExecutionEnvironment>
+                                                               <Bundle-RequiredExecutionEnvironment>JavaSE-17</Bundle-RequiredExecutionEnvironment>
                                                                <Built-By></Built-By>
                                                        </manifestEntries>
                                                </transformer>

@andrewazores
Copy link
Member

Nice, that worked! However now after inserting probes, switching to the Live Configuration tab shows an empty state. If I navigate away and back, or refresh the page, then I see the expected content:

image

image

This is probably just a web-client bug - it seems like the Live Configuration tab/table isn't reacting to the notification.

If it is the case that it's on the web-client side then I think this PR is good to go. Could you help check that out, ie. make sure it isn't some other notification that is missing or has a different category than the client expects etc.?

@andrewazores
Copy link
Member

/build_test

Copy link

Workflow started at 4/19/2024, 4:08:11 PM. View Actions Run.

@andrewazores
Copy link
Member

Also, are there any upstream JMC plans to update the Agent to use a newer asm?

Copy link

No OpenAPI schema changes detected.

@andrewazores
Copy link
Member

It looks like the client only expects ProbeTemplateUploaded, ProbeTemplateDeleted, ProbeTemplateApplied, and ProbesRemoved categories:

https://github.com/cryostatio/cryostat-web/blob/89e2786769ee5bfde86ece129f09611c8fb55d28/src/app/Shared/Services/api.types.ts#L569

which matches what you've implemented here. So I think it's probably OK and the client just needs to react to the notifications.

Copy link

CI build and push: At least one test failed ❌ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8759037935

@andrewazores
Copy link
Member

One spotbugs detection:

Error:  Medium: Exception is caught when Exception is not thrown in io.cryostat.jmcagent.JMCAgent.lambda$postProbe$0(String, JFRConnection) [io.cryostat.jmcagent.JMCAgent] At JMCAgent.java:[line 87] REC_CATCH_EXCEPTION

I'm not sure why the GraphQL schema job is failing, but that should be unrelated to this PR so I will ignore it for now.

@Josh-Matsuoka
Copy link
Contributor Author

Also, are there any upstream JMC plans to update the Agent to use a newer asm?

I have a bug open for that as well https://bugs.openjdk.org/browse/JMC-7948 , I'll look into it as well as fixing the other upstream bug.

@Josh-Matsuoka
Copy link
Contributor Author

Josh-Matsuoka commented Apr 19, 2024

cryostatio/cryostat-core#374 goes alongside the last commit to make the ProbeDefinitionException visible so it can be caught explicitly fixing the spotbugs issue

@andrewazores
Copy link
Member

I approved and merged that commit and kicked off a new -core 2.30.3 release build for it. That'll take a little while to become available. Once it is I will trigger Dependabot to upgrade it here, then we can rebase this PR and I'll approve and merge it as well.

@Josh-Matsuoka
Copy link
Contributor Author

sounds good

@andrewazores
Copy link
Member

/build_test

Copy link

Workflow started at 4/19/2024, 5:14:57 PM. View Actions Run.

Copy link

No GraphQL schema changes detected.

Copy link

No OpenAPI schema changes detected.

Copy link

CI build and push: All tests pass ✅ (JDK17)
https://github.com/cryostatio/cryostat3/actions/runs/8759686962

@andrewazores andrewazores merged commit a1e941f into cryostatio:main Apr 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Story] JMC Agent
2 participants