feat(notifications): implement vertx event bus to listen for notifications emitted

[maxcao13]

Fixes #921

So far I've implemented the feature very shallowly, thoughts on what needs to be done on a larger scale? I'm not quite sure what you meant by the NotificationListener thin wrapper feature. Right now, every consumer handlers trigger on any DeleteActiveRecording Notification, and that might not be good so I was thinking maybe passing the RecordingDescriptor key when publishing the notification to the address and then each consumer just if else checking the message? Is that why we need a wrapper around a Notification to better facilitate this?

Also, would you recommend resources for understanding the module, provides, component, injection design patterns of Dagger, Vertx and other inversion of control frameworks? This part of back-end development is definitely something I would like to get more comfortable with along my time on the project.

[andrewazores]

I'm not quite sure what you meant by the NotificationListener thin wrapper feature.

I just meant to define some interface like NotificationListener, so that you can invert control and rather than each class needing to have the Vertx injected and subscribe to the event bus, you could have such classes implement NotificationListener and have a Dagger module @Binds @IntoSet for these listeners (very much like RequestHandlers). This would mostly just be to reduce how much surface area Vertx has in our code base, so that if/when we migrate to another web framework it's already abstracted away and less of a chore to change implementations, and also to simplify unit testing in the meantime. I'll write some snippets of example code illustrating the idea here.

Right now, every consumer handlers trigger on any DeleteActiveRecording Notification, and that might not be good so I was thinking maybe passing the RecordingDescriptor key when publishing the notification to the address and then each consumer just if else checking the message? Is that why we need a wrapper around a Notification to better facilitate this?

It's the nature of a pub/sub system or an event bus to send out messages broadly, and on the other end try to listen only for messages you're likely to care about. In this case the event bus "address", which for us is the notification category, fills that role. Beyond that, the message consumer may need to do some further check to determine if the message is something it does care about, which you've handled here.

Also, would you recommend resources for understanding the module, provides, component, injection design patterns of Dagger, Vertx and other inversion of control frameworks? This part of back-end development is definitely something I would like to get more comfortable with along my time on the project.

Well, Vertx isn't an inversion-of-control framework. Dagger gives us dependency injection, which is generally a kind of inversion of control. For that, Dagger's own docs are pretty good. DI isn't limited to backend development either, in fact Dagger is probably much more commonly used in mobile development than it is in backend development.

[andrewazores]

interface NotificationListener<T> {
    Set<String> categories();
    void onMessage(String category, T message);
}

class NotificationPublisher {
    Vertx vertx;
    Set<NotificationListener> listeners;

    NotificationPublisher() {
        listener.forEach(listener -> {
            listener.categories().forEach(category -> {
                vertx.eventBus().consumer(category).handler(message -> listener.onMessage(category, message));
            });
        });
    }

    <T> void send(String category, T message) {
        vertx.eventBus().publish(category, message);
    }
}

class NotificationModule {
    @Binds @IntoSet abstract NotificationListener bindTestListener(TestListener listener);

    // in Cryostat we already have the NotificationFactory that is something like this
    @Provides static NotificationPublisher provideNotificationPublisher(Vertx vertx, Set<NotificationListener> listeners) {
        return new NotificationPublisher(vertx, listeners);
    }
}

class TestListener implements NotificationListener<Foo> {
    @Override
    public Set<String> categories() {
        return Set.of("cat", "dog");
    }

    @Override
    public void onMessage(String category, Foo message) {
        System.out.println(String.format("[%s]: %s", category, message.asJson()));
    }
}

class TestSender {
    NotificationPublisher publisher;

    void hello() {
        publisher.send("dog", new Foo(1234));
    }
}

That's a little sloppy, but I hope it gets the point across. This way only the publisher class actually knows anything about Vertx, and it uses it in a very isolated fashion. The listeners don't need to add a Vertx dependency injection, and the only notification-related tests needed on the TestListener would be to simply call onMessage and verify that the expected result occurred (ex. Mockito.verify(dep).doSomething()).

[maxcao13]

Not sure how to bind the ActiveRecordingReportCache into the set of listeners in the NotificationPublisher without making it public.

Also may be better ways to parse the Notification message but I don't think using POJOs here is smart nor sending the fields preparsed for generality of the onMessage method.

Ignoring tests, thoughts on the structure?

[andrewazores]

Not sure how to bind the ActiveRecordingReportCache into the set of listeners in the NotificationPublisher without making it public.

Not sure exactly what you mean here.

cryostat/src/main/java/io/cryostat/net/web/http/api/v2/HttpApiV2Module.java

Line 94 in 4c45fbc

abstract RequestHandler bindApiGetHandler(ApiGetHandler handler);

cryostat/src/main/java/io/cryostat/net/web/http/api/v2/ApiGetHandler.java

Line 62 in 4c45fbc

class ApiGetHandler extends AbstractV2RequestHandler<ApiGetHandler.ApiResponse> {

Does this example help answer the question at all?

Also may be better ways to parse the Notification message but I don't think using POJOs here is smart

Why?

[maxcao13]

Not sure how to bind the ActiveRecordingReportCache into the set of listeners in the NotificationPublisher without making it public.

Not sure exactly what you mean here.

cryostat/src/main/java/io/cryostat/net/web/http/api/v2/HttpApiV2Module.java

Line 94 in 4c45fbc

abstract RequestHandler bindApiGetHandler(ApiGetHandler handler);

cryostat/src/main/java/io/cryostat/net/web/http/api/v2/ApiGetHandler.java

Line 62 in 4c45fbc

class ApiGetHandler extends AbstractV2RequestHandler<ApiGetHandler.ApiResponse> {

Does this example help answer the question at all?

Sorry, I meant that the NotificationsModule is in java.io.cryostat.messaging.notifications.NotificationsModule and the report cache is in java..io.cryostat.net.reports.ActiveRecordingReportCache so I think the NotificationsModule can't see the cache as a class since it's in a different package and that's what I meant.

Also may be better ways to parse the Notification message but I don't think using POJOs here is smart

Why?

Uh, I think I was thinking like generally, since Notifications have different structures, it may be difficult to extend the parsing to all kinds, but I realize that Notifications are all generally the same structure of Meta and Message so it probably isn't that hard to parse.

[andrewazores]

Sorry, I meant that the NotificationsModule is in java.io.cryostat.messaging.notifications.NotificationsModule and the report cache is in java..io.cryostat.net.reports.ActiveRecordingReportCache so I think the NotificationsModule can't see the cache as a class since it's in a different package and that's what I meant.

I see. Though, why does the NotificationsModule need visiblity of the ActiveRecordingReportCache? The ActiveRecordingReportCache is provided by the ReportsModule, so the ReportsModule can do the @Binds @IntoSet to bind the cache as a notification listener. This way the NotificationListener interface just needs to be publicly visible so that the ReportsModule and cache class can see it. Dagger will take care of generating the Set<NotificationListener> for injection for whatever callers want it.

Uh, I think I was thinking like generally, since Notifications have different structures, it may be difficult to extend the parsing to all kinds, but I realize that Notifications are all generally the same structure of Meta and Message so it probably isn't that hard to parse.

Makes sense. Yea, I think your insight that our notifications all have a consistent form of Meta and Message, where both of those are also generally just flat map structures, is the key.

The MessagingServer and WsMessage classes predate the current unified Notification system that we have, which is why the server and WsMessage are so open-ended and generic. In practice the only kind of messages we ever send anymore are Notifications that come through the NotificationFactory, so perhaps the WsMessage can be removed and simply replaced with Notification. WsMessage is an empty definition anyway and the only class that extends it is Notification, so this typing simplification should be pretty straightforward to perform, and then provides for better type safety elsewhere since you can start relying on messages to be of type Notification. And Notification<T> already does look a lot like that NotificationListener<T>, onMessage(String category, T message) we talked about earlier.

[maxcao13]

Is this sort of what it should look like? Sorry, I'm sort of having trouble adapting this to the example you linked, mostly because of the Notification<T>.

[andrewazores]

Yea, that looks like it's along the right lines. Forcing the type to be Notification<Map<String, Object>> doesn't seem right, though. I'm not sure if every notification we currently emit has that format, and if it does, do we want to commit to that being the only format moving forward? If so then we may as well remove the <T> off of Notification<T> and just solidify the Map<String, Object> type, rather than leaving it defined as a generic type but in practice only allowing one type.

I think I would rather leave the Notification<T> part generic, and allow the message consumers to determine what to do with it (ie assume what they can cast the message to). That might mean removing <?> and just leaving it as a bare, non-generic/non-parameterized type definition. You'll get compile time warnings about unchecked casts and such, but maybe that's just a necessary evil for now.

[maxcao13]

Before tests and removing prints, thoughts?

[andrewazores]

Looking good, nice and clean now. Just a few more notes.

[andrewazores]

Looks good. I have some further refactoring ideas in mind but I think I'll just play around with that myself after we get this merged, and see what I end up liking.