feat(recordings): include jvmId in Notifications to -web #1689

mwangggg · 2023-09-29T19:28:39Z

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Read the contributing guidelines
Linked a relevant issue which this PR resolves
Linked any other relevant issues, PR's, or documentation, if any
Resolved all conflicts, if any
Rebased your branch PR on top of the latest upstream main branch
Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

Fixes: #1452
Related to cryostatio/cryostat-web#1126
Depends on #1743 (for ease of review/verifying event message contents)

github-actions · 2023-09-29T19:28:51Z

Hi @mwangggg! Add at least one of the required labels to this PR

Required labels are : chore,ci,cleanup,docs,feat,fix,perf,refactor,style,test

tthvo

Looks good! Tho, I am not sure if this solves #1452. There are other resources to look into:

'activeRecordings', // Done here
'archivedRecordings',
'eventTemplates',
'eventTypes',
'agentProbes',

References: https://github.com/cryostatio/cryostat-web/blob/86a8094c1947004fb1f625da7f517d071e2920d1/src/app/Topology/Entity/types.ts#L43

mwangggg · 2023-10-03T20:08:39Z

/request_review

andrewazores · 2023-10-11T17:22:04Z

Looks like it makes sense, though I wonder if it's worth doing some refactoring at this point to extract a reusable utility method that looks something like notificationFactory.createOwnedResourceBuilder(targetId), which still returns a notification builder but pre-configured with the JSON content type, a message Map with the target and jvmId pre-populedand can have the owned resource appended, etc.

mwangggg · 2023-10-19T18:35:51Z

/build_test

mwangggg · 2023-10-19T18:37:36Z

https://github.com/cryostatio/cryostat/actions/runs/6578832827

github-actions · 2023-10-19T18:58:07Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1689-6b575e5d17dfe3f8a87b0b16f81d85ef3d8a86bc-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1689-6b575e5d17dfe3f8a87b0b16f81d85ef3d8a86bc-linux-arm64

To run smoketest:

# amd64          
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1689-6b575e5d17dfe3f8a87b0b16f81d85ef3d8a86bc-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1689-6b575e5d17dfe3f8a87b0b16f81d85ef3d8a86bc-linux-arm64 sh smoketest.sh

andrewazores · 2023-10-19T18:58:59Z

Rebase please

andrewazores · 2023-10-19T19:01:45Z

/build_test

mwangggg · 2023-10-19T19:06:46Z

https://github.com/cryostatio/cryostat/actions/runs/6579065965

andrewazores · 2023-10-19T19:20:25Z

Looks like it makes sense, though I wonder if it's worth doing some refactoring at this point to extract a reusable utility method that looks something like notificationFactory.createOwnedResourceBuilder(targetId), which still returns a notification builder but pre-configured with the JSON content type, a message Map with the target and jvmId pre-populedand can have the owned resource appended, etc.

Here's a rough and incomplete patch illustrating a bit more of this half-baked idea I presented before:

diff --git a/src/main/java/io/cryostat/messaging/notifications/Notification.java b/src/main/java/io/cryostat/messaging/notifications/Notification.java
index 44a4f6ea..a1d8981c 100644
--- a/src/main/java/io/cryostat/messaging/notifications/Notification.java
+++ b/src/main/java/io/cryostat/messaging/notifications/Notification.java
@@ -15,6 +15,9 @@
  */
 package io.cryostat.messaging.notifications;
 
+import java.util.HashMap;
+import java.util.Map;
+
 import io.cryostat.net.web.http.HttpMimeType;
 
 public class Notification<T> {
@@ -82,6 +85,27 @@ public class Notification<T> {
         }
     }
 
+    public static class OwnedResourceBuilder extends Builder<Map<String, Object>> {
+
+        private final Map<String, Object> map = new HashMap<>();
+
+        OwnedResourceBuilder(NotificationSource source, String category) {
+            super(source);
+            metaType(HttpMimeType.JSON);
+            metaCategory(category);
+            message(map);
+        }
+
+        public OwnedResourceBuilder messageEntry(String key, Object value) {
+            if (value == null) {
+                this.map.remove(key);
+            } else {
+                this.map.put(key, value);
+            }
+            return this;
+        }
+    }
+
     public static class Meta {
         private final String category;
         private final MetaType type;
diff --git a/src/main/java/io/cryostat/messaging/notifications/NotificationFactory.java b/src/main/java/io/cryostat/messaging/notifications/NotificationFactory.java
index 1167e17f..c929db2e 100644
--- a/src/main/java/io/cryostat/messaging/notifications/NotificationFactory.java
+++ b/src/main/java/io/cryostat/messaging/notifications/NotificationFactory.java
@@ -15,23 +15,35 @@
  */
 package io.cryostat.messaging.notifications;
 
-import io.cryostat.net.web.http.HttpMimeType;
+import io.cryostat.recordings.JvmIdHelper;
+import io.cryostat.recordings.JvmIdHelper.JvmIdGetException;
 
 public class NotificationFactory {
 
     private final NotificationSource source;
+    private final JvmIdHelper jvmIdHelper;
 
-    NotificationFactory(NotificationSource source) {
+    NotificationFactory(NotificationSource source, JvmIdHelper jvmIdHelper) {
         this.source = source;
+        this.jvmIdHelper = jvmIdHelper;
     }
 
     public <T> Notification.Builder<T> createBuilder() {
         return new Notification.Builder<T>(source);
     }
 
-    public <T> Notification.Builder<T> createOwnedResourceBuilder(String notificationCategory) {
-        return new Notification.Builder<T>(source)
-                .metaType(HttpMimeType.JSON)
-                .metaCategory(notificationCategory);
+    public Notification.OwnedResourceBuilder createOwnedResourceBuilder(
+            String notificationCategory) {
+        return new Notification.OwnedResourceBuilder(source, notificationCategory);
+    }
+
+    public Notification.OwnedResourceBuilder createOwnedResourceBuilder(
+            String targetId, String notificationCategory) throws JvmIdGetException {
+        return new Notification.OwnedResourceBuilder(source, notificationCategory)
+                // FIXME the websocket notification system should only emit the targetId under one
+                // named key or the other, not both - are we already consistent with this?
+                .messageEntry("target", targetId)
+                .messageEntry("connectUrl", targetId)
+                .messageEntry("jvmId", jvmIdHelper.getJvmId(targetId));
     }
 }
diff --git a/src/main/java/io/cryostat/messaging/notifications/NotificationsModule.java b/src/main/java/io/cryostat/messaging/notifications/NotificationsModule.java
index 9777bfff..120f72c3 100644
--- a/src/main/java/io/cryostat/messaging/notifications/NotificationsModule.java
+++ b/src/main/java/io/cryostat/messaging/notifications/NotificationsModule.java
@@ -19,6 +19,8 @@ import java.util.Set;
 
 import javax.inject.Singleton;
 
+import io.cryostat.recordings.JvmIdHelper;
+
 import dagger.Lazy;
 import dagger.Module;
 import dagger.Provides;
@@ -34,7 +36,8 @@ public abstract class NotificationsModule {
 
     @Provides
     @Singleton
-    static NotificationFactory provideNotificationFactory(NotificationSource source) {
-        return new NotificationFactory(source);
+    static NotificationFactory provideNotificationFactory(
+            NotificationSource source, JvmIdHelper idHelper) {
+        return new NotificationFactory(source, idHelper);
     }
 }
diff --git a/src/main/java/io/cryostat/net/web/http/api/beta/RecordingsFromIdPostHandler.java b/src/main/java/io/cryostat/net/web/http/api/beta/RecordingsFromIdPostHandler.java
index 6bd995d3..87dd745a 100644
--- a/src/main/java/io/cryostat/net/web/http/api/beta/RecordingsFromIdPostHandler.java
+++ b/src/main/java/io/cryostat/net/web/http/api/beta/RecordingsFromIdPostHandler.java
@@ -47,6 +47,7 @@ import io.cryostat.net.web.http.api.ApiVersion;
 import io.cryostat.net.web.http.api.v2.ApiException;
 import io.cryostat.recordings.JvmIdHelper;
 import io.cryostat.recordings.JvmIdHelper.JvmIdDoesNotExistException;
+import io.cryostat.recordings.JvmIdHelper.JvmIdGetException;
 import io.cryostat.recordings.RecordingArchiveHelper;
 import io.cryostat.recordings.RecordingMetadataManager;
 import io.cryostat.recordings.RecordingMetadataManager.Metadata;
@@ -274,35 +275,30 @@ public class RecordingsFromIdPostHandler extends AbstractAuthenticatedRequestHan
                                     try {
 
                                         notificationFactory
-                                                .createOwnedResourceBuilder(NOTIFICATION_CATEGORY)
-                                                .message(
-                                                        Map.of(
-                                                                "recording",
-                                                                new ArchivedRecordingInfo(
-                                                                        connectUrl,
-                                                                        fsName,
-                                                                        webServer
-                                                                                .get()
-                                                                                .getArchivedDownloadURL(
-                                                                                        connectUrl,
-                                                                                        fsName),
-                                                                        webServer
-                                                                                .get()
-                                                                                .getArchivedReportURL(
-                                                                                        connectUrl,
-                                                                                        fsName),
-                                                                        metadata,
-                                                                        size,
-                                                                        archivedTime),
-                                                                "target",
+                                                .createOwnedResourceBuilder(
+                                                        connectUrl, NOTIFICATION_CATEGORY)
+                                                .messageEntry(
+                                                        "recording",
+                                                        new ArchivedRecordingInfo(
                                                                 connectUrl,
-                                                                "jvmId",
-                                                                jvmId))
+                                                                fsName,
+                                                                webServer
+                                                                        .get()
+                                                                        .getArchivedDownloadURL(
+                                                                                connectUrl, fsName),
+                                                                webServer
+                                                                        .get()
+                                                                        .getArchivedReportURL(
+                                                                                connectUrl, fsName),
+                                                                metadata,
+                                                                size,
+                                                                archivedTime))
                                                 .build()
                                                 .send();
                                     } catch (URISyntaxException
                                             | UnknownHostException
-                                            | SocketException e) {
+                                            | SocketException
+                                            | JvmIdGetException e) {
                                         logger.error(e);
                                         throw new ApiException(500, e);
                                     }

WDYT? This is just meant to make it a little easier to pass the right data in and get the expected notification emitted, or rather to make it a little harder to forget to include things.

mwangggg · 2023-10-19T19:26:39Z

looks good! I'll try it out

mwangggg · 2023-10-24T19:39:44Z

/build_test

mwangggg · 2023-10-24T19:39:58Z

https://github.com/cryostatio/cryostat/actions/runs/6631741944

mwangggg · 2023-10-24T19:44:39Z

/build_test

mwangggg · 2023-10-24T19:44:50Z

https://github.com/cryostatio/cryostat/actions/runs/6631783636

github-actions · 2023-10-24T20:09:07Z

ARCH	IMAGE
amd64	ghcr.io/cryostatio/cryostat:pr-1689-3c391a4490ae5b30041b3bf88fb62fb4b4fad1db-linux-amd64
arm64	ghcr.io/cryostatio/cryostat:pr-1689-3c391a4490ae5b30041b3bf88fb62fb4b4fad1db-linux-arm64

To run smoketest:

# amd64          
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1689-3c391a4490ae5b30041b3bf88fb62fb4b4fad1db-linux-amd64 sh smoketest.sh

# or arm64
CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1689-3c391a4490ae5b30041b3bf88fb62fb4b4fad1db-linux-arm64 sh smoketest.sh

github-actions · 2023-10-26T00:43:18Z

This PR/issue depends on:

~~cryostatio/cryostat#1743~~
By Dependent Issues (🤖). Happy coding!

andrewazores · 2023-10-26T20:23:34Z

Rebase please

(cherry picked from commit 3202849)

(cherry picked from commit 3202849) Co-authored-by: Ming Yu Wang <[email protected]>

mwangggg added the feat New feature or request label Sep 29, 2023

mergify bot added the safe-to-test label Sep 29, 2023

mwangggg mentioned this pull request Oct 2, 2023

chore(recordings): check target's jvmId for recording events cryostatio/cryostat-web#1126

Merged

7 tasks

mwangggg force-pushed the 1452-target-include-jvmID branch from a675f59 to 1929840 Compare October 2, 2023 14:47

mwangggg marked this pull request as ready for review October 2, 2023 16:36

mwangggg requested review from andrewazores, aali309 and tthvo October 2, 2023 17:06

tthvo reviewed Oct 2, 2023

View reviewed changes

mwangggg force-pushed the 1452-target-include-jvmID branch from 5b420e7 to 1c103cc Compare October 3, 2023 15:16

mwangggg changed the title ~~feat(recordings): include jvmId in issueNotifications for recordings~~ feat(recordings): include jvmId in Notifications to -web Oct 3, 2023

mwangggg requested a review from tthvo October 3, 2023 18:58

github-actions bot added the review-requested label Oct 3, 2023

mergify bot requested a review from a team October 3, 2023 20:09

mwangggg force-pushed the 1452-target-include-jvmID branch from b058118 to d3e6b03 Compare October 6, 2023 14:38

mwangggg force-pushed the 1452-target-include-jvmID branch 2 times, most recently from e4b8a21 to 6b575e5 Compare October 19, 2023 18:34

mwangggg force-pushed the 1452-target-include-jvmID branch from 6b575e5 to 0013d92 Compare October 19, 2023 19:00

andrewazores added the backport label Oct 19, 2023

github-actions bot added the dependent label Oct 24, 2023

mwangggg force-pushed the 1452-target-include-jvmID branch from 7c36e46 to 3c391a4 Compare October 24, 2023 19:44

github-actions bot removed the dependent label Oct 26, 2023

mwangggg force-pushed the 1452-target-include-jvmID branch 3 times, most recently from 8768645 to 58e9fa4 Compare October 30, 2023 15:26

mwangggg added 8 commits October 30, 2023 12:13

include jvmId in issueNotifications for recordings

b611dcb

fix recording tests

ef707e7

add JvmId to notifications

e5ce1b2

send jvmId through notifications to -web

ded46c0

refractor reusable notification builder

77ce9cd

spotless

8f4f04f

spotbugs

1f63b8b

spotless

17e55b0

mwangggg force-pushed the 1452-target-include-jvmID branch from 58e9fa4 to 17e55b0 Compare October 30, 2023 16:13

andrewazores removed the backport label Oct 30, 2023

andrewazores approved these changes Oct 30, 2023

View reviewed changes

andrewazores merged commit 3202849 into cryostatio:main Oct 30, 2023
10 checks passed

andrewazores added the backport label Oct 30, 2023

mergify bot pushed a commit that referenced this pull request Oct 30, 2023

feat(recordings): include jvmId in Notifications to -web (#1689)

7780255

(cherry picked from commit 3202849)

mergify bot mentioned this pull request Oct 30, 2023

feat(recordings): include jvmId in Notifications to -web (backport #1689) #1759

Merged

andrewazores pushed a commit that referenced this pull request Oct 30, 2023

feat(recordings): include jvmId in Notifications to -web (#1689) (#1759)

469696b

(cherry picked from commit 3202849) Co-authored-by: Ming Yu Wang <[email protected]>

mwangggg deleted the 1452-target-include-jvmID branch March 12, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(recordings): include jvmId in Notifications to -web #1689

feat(recordings): include jvmId in Notifications to -web #1689

mwangggg commented Sep 29, 2023 •

edited by andrewazores

Loading

github-actions bot commented Sep 29, 2023

tthvo left a comment •

edited

Loading

mwangggg commented Oct 3, 2023

andrewazores commented Oct 11, 2023

mwangggg commented Oct 19, 2023

mwangggg commented Oct 19, 2023

github-actions bot commented Oct 19, 2023

andrewazores commented Oct 19, 2023

andrewazores commented Oct 19, 2023

mwangggg commented Oct 19, 2023

andrewazores commented Oct 19, 2023 •

edited

Loading

mwangggg commented Oct 19, 2023

mwangggg commented Oct 24, 2023

mwangggg commented Oct 24, 2023

mwangggg commented Oct 24, 2023

mwangggg commented Oct 24, 2023

github-actions bot commented Oct 24, 2023

github-actions bot commented Oct 26, 2023

andrewazores commented Oct 26, 2023

feat(recordings): include jvmId in Notifications to -web #1689

feat(recordings): include jvmId in Notifications to -web #1689

Conversation

mwangggg commented Sep 29, 2023 • edited by andrewazores Loading

Welcome to Cryostat! 👋

Before contributing, make sure you have:

github-actions bot commented Sep 29, 2023

tthvo left a comment • edited Loading

Choose a reason for hiding this comment

mwangggg commented Oct 3, 2023

andrewazores commented Oct 11, 2023

mwangggg commented Oct 19, 2023

mwangggg commented Oct 19, 2023

github-actions bot commented Oct 19, 2023

andrewazores commented Oct 19, 2023

andrewazores commented Oct 19, 2023

mwangggg commented Oct 19, 2023

andrewazores commented Oct 19, 2023 • edited Loading

mwangggg commented Oct 19, 2023

mwangggg commented Oct 24, 2023

mwangggg commented Oct 24, 2023

mwangggg commented Oct 24, 2023

mwangggg commented Oct 24, 2023

github-actions bot commented Oct 24, 2023

github-actions bot commented Oct 26, 2023

andrewazores commented Oct 26, 2023

mwangggg commented Sep 29, 2023 •

edited by andrewazores

Loading

tthvo left a comment •

edited

Loading

andrewazores commented Oct 19, 2023 •

edited

Loading