HWKMETRICS-83 Create glue code component to integrate with hawkular-bus

[tsegismont]

HWKMETRICS-83 Create glue code component to integrate with hawkular-bus

This pull request is comprised of three commits.

• Implement an InsertedDataPoint event stream

The MetricsService interface now exposes an #insertedDataEvents method. The method returns a hot Observable emitting Metric objects, when their data has been succesfully inserted.

Clients can subscribe to this Observable (preferably on a dedicated scheduler!) and they will get notified.

• Fire @ServiceReady event when MetricsService is initialized

The Metrics JAX- RS module now fires a CDI @ServiceReady event.
This event can be observed by a "super"-module in order to know when the MetricsService instance is initialized.

The event is fired just before the MetricsServiceLifecycle status changes to "STARTED", so that clients can subscribe to the #insertedDataEvents observable before new data points are accepted.

• Introduce Hawkular Metrics Component module

There is a new "hawkular-metrics-component" WAR module. It is meant to replace "hawkular-metrics-api-jaxrs" module in Hawkular deployment.

This new WAR is a "super"-module: it uses Maven WAR plugin overlay mechanism to add extra classes to the standalone metrics web application.

The extra classes allow to subscribe to the #insertedDataEvents observable and publish messages on the bus.

In order to make the migration easy, I have shamelessly copied Alerts' AvailDataMessage and MetricDataMessage classes. Theses messages are posted on to the "HawkularAvailData" and "HawkularMetricData" topics, respectively.

After this PR is merged we only need to change the agent code to remove the "double-push" part. And change Hawkular POM to switch to the new Metrics component, that's it.

You may notice that the integration test class borrows a lot from the RESTTest class in rest-tests module. I think we need to introduce a new 'test-utils' module, in a DRY approach. But I wouldn't want to block this PR and would rather do this as a separate PR.

[stefannegrea]

This PR is postponed until 0.9.0.

[pilhuhn]

@stefannegrea Could that land in master already (it checks pass), so that hawkular can reference it already via src-dep?

[tsegismont]

Rebased on master

[jshaughn]

So the #insertedDataEvents Observable emits every stored datapoint? Is that just for all the various types of data? is it possible to set up Observables for specific metrics? I'm thinking along the lines of just those that may be useful for alerting. Today alerts looks at everything that comes onto the bus and filters out what it is not useful. I'm wondering whether we can push that filtering to the source (as a future optimization perhaps).

[jshaughn]

The @ServiceReady looks interesting. At the moment the hawkular-alerts-metrics war is, I think, the only co-located metrics client. And it jumps through some hoops to establish a metrics service connection. I'm hoping that maybe this eases some of that pain.

[tsegismont]

Le 12/10/2015 15:45, Jay Shaughnessy a écrit :

So the #insertedDataEvents Observable emits every stored datapoint? Is
that just for all the various types of data? is it possible to set up
Observables for specific metrics? I'm thinking along the lines of just
those that may be useful for alerting. Today alerts looks at everything
that comes onto the bus and filters out what it is not useful. I'm
wondering whether we can push that filtering to the source (as a future
optimization perhaps).

Yes, it emits every stored datapoint. Actually, it emits the Metric
instance emitted by the Observable supplied to #addDataPoints.

#insertedDataEvents returns a single Observable instance which emits
Metric instances of all types (GAUGE, COUNTER, AVAILABILITY).

If you have access to the MetricsService instance, it is possible to get
an Observable for specific metrics: call #insertedDataEvents and chain
with Observable#filter.

The bus integration proposed in this PR does not offer the possibility
to select metrics though.
Firstly I'm not sure which topic we'd publish messages to. The goal of
the topic is to let any component pick up events it's interested in.
Secondly, I don't know how we'd keep Alerts and Metrics in sync with
respect to the data Alerts is interested in.

We could bring that up in the next water cooler.

[tsegismont]

Le 12/10/2015 15:47, Jay Shaughnessy a écrit :

The @ServiceReady looks interesting. At the moment the
hawkular-alerts-metrics war is, I think, the only co-located metrics
client. And it jumps through some hoops to establish a metrics service
connection. I'm hoping that maybe this eases some of that pain.

Are you talking about the hawkular-alerter-metrics module? I'm not sure
CDI events can be propagated to other WAR deployments.

[jshaughn]

Are you talking about the hawkular-alerter-metrics module? I'm not sure CDI events can be propagated to other WAR deployments.

Yes, that is what I am referring to. And I think you are right. I do @Inject a metricsService but I had to write a Producer, and that Producer code is a bit tricky/fragile.

[stefannegrea]

What do you think about moving this module under api? Since it is meant for external consumption?

[tsegismont]

Le 13/10/2015 00:03, Stefan Negrea a écrit :

What do you think about moving this module under api? Since it is meant
for external consumption?

The api directory was created because we had two different versions of
the REST API on top of api-common. The new module is not an "API".
Actually I could live with the two REST module flattened in the ROOT dir

In the future - when JAX-RS 1.1 impl goes away, which should happen soon
according to this summer's discussion :=) - I'd like to:

• merge the JAX-RS module with api-common into a single JAR module (the
  "REST" module)
• create a hawkular-metrics-standalone WAR module, with a dependency on
  the REST module
• make hawkular-metrics-standalone a true (not overlay) WAR module, with
  a dependency on the REST module

[jsanda]

The MetricsService interface now exposes an #insertedDataEvents method. The method returns a hot Observable emitting Metric objects, when their data has been succesfully inserted.

Clients can subscribe to this Observable (preferably on a dedicated scheduler!) and they will get notified.

Subscribers will by default execute on, i.e., observer on, the Rx computation scheduler since that is what we use by default to handle notifications from the C* driver when requests complete. We should probably point this out in the Javadocs.

[jsanda]

Do InsertedDataSubscriber, MetricDataPublisher, and AvailDataPublisher need to be EJBs? Can they just be managed / CDI beans?

Interestingly, I tried changing them to be managed beans and InsertedDataITest fails. InsertedDataSubscriber.onMetricsServiceReady never gets called.

[jsanda]

In MetricsServiceITest.shouldReceiveInsertedDataNotifications, can you replace the usage of CountDownLatch and the subscriber with a TestSubscriber? Then we have something like,

TestSubscriber<Metric<?>> testSubscriber = new TestSubscriber<>();
metricsService.insertedDataEvents()
        .filter(metric -> metric.getId().getTenantId().equals(tenantId))
        .subscribe(testSubscriber);

...

// assertTrue(latch.await(1, MINUTES), "Did not receive all notifications");

// assertEquals(actual.size(), expected.size());
// assertTrue(actual.containsAll(expected));

testSubscriber.awaitTerminalEvent(1, MINUTES);
assertEquals(testSubscriber.getOnNextEvents().size(), expected.size());
assertTrue(testSubscriber.getOnNextEvents().containsAll(expected));

And how about breaking up shouldReceiveInsertedDataNotifications separate test methods for each metric type? It makes the tests more self-documenting IMO.

[tsegismont]

Le 12/10/2015 19:24, Jay Shaughnessy a écrit :

I do @Inject https://github.com/Inject a metricsService but I had to
write a Producer, and that Producer code is a bit tricky/fragile.

I'm not sure it's safe BTW. MetricsService is not only a data access
service. It also works with the scheduler to execute background tasks.

[tsegismont]

Le 13/10/2015 00:47, jsanda a écrit :

Subscribers will by default execute on the Rx computation scheduler
since that is what we use by default to handle notifications from the C*
driver when requests complete.

Exactly. But depending on what they do with notifications (blocking
operations?), we may want them to use another scheduler.

[tsegismont]

Do InsertedDataSubscriber, MetricDataPublisher, and AvailDataPublisher need to be EJBs? Can they just be managed / CDI beans?

They could. But I don't see a case for avoiding Java EE constructs in hawkular-metrics-component (as opposed to the standalone app). All other Hawkular components have so much dependencies on Java EE that I doubt the Hawkular server will ever run on something else.

Interestingly, I tried changing them to be managed beans and InsertedDataITest fails. InsertedDataSubscriber.onMetricsServiceReady never gets called.

Maybe you forgot to mark the beans with @Eager?

[tsegismont]

Le 13/10/2015 05:11, jsanda a écrit :

In MetricsServiceITest.shouldReceiveInsertedDataNotifications, can you
replace the usage of CountDownLatch and the subscriber with a
TestSubscriber? Then we have something like,

OK

And how about breaking up |shouldReceiveInsertedDataNotifications|
separate test methods for each metric type? It makes the tests more
self-documenting IMO.

I liked the idea of having metrics of different types emitted by the
Observable in the same test. Which part isn't clear enough?

[tsegismont]

In MetricsServiceITest.shouldReceiveInsertedDataNotifications, can you
replace the usage of CountDownLatch and the subscriber with a
TestSubscriber? Then we have something like,

OK

I have been experimenting with TestSubscriber and I'd rather not use it.

Firstly, it's marked @Experimental, so it could be changed... or simply removed in a future release.
Secondly, #insertDataEvents observable does not emit a terminal event until MetricsService is shutdown. So we couldn't use the method you pointed out.
Eventually, its' not thread safe because it accumulates #onNext events in an raw ArrayList.

[jsanda]

Exactly. But depending on what they do with notifications (blocking
operations?), we may want them to use another scheduler.

I did some investigation, and the version of ActiveMQ we use implements JMS 1.1 not 2.0. It does not look like there is support in JMS 1.1 for async sending. ActiveMQ supports async sending as described here. It looks like that might simply be fire-n-forget which could be sufficient. With all that said, I think using a different scheduler here makes sense since sending is blocking.

I am considering opening a ticket/PR for Hawkular Bus to introduce an Rx wrapper for the MessageProcessor class that would execute on the I/O scheduler by default.

[jshaughn]

I'm not sure it's safe BTW. MetricsService is not only a data access service. It also works with the scheduler to execute background tasks.

I'd be happy if there were some obvious/supported way to establish the co-located service. I didn't see one so I had to take this route. Actually, I had to submit a Metrics PR just to get this approach to work...

[jsanda]

They could. But I don't see a case for avoiding Java EE constructs in hawkular-metrics-component (as opposed to the standalone app). All other Hawkular components have so much dependencies on Java EE that I doubt the Hawkular server will ever run on something else.

When there is a compelling reason to use some feature, library, etc., then I am all for it. I am not in favor of using it simply because we can, whether it is JEE or something else. On the other hand....

Interestingly, I tried changing them to be managed beans and InsertedDataITest fails. InsertedDataSubscriber.onMetricsServiceReady never gets called.

Maybe you forgot to mark the beans with @eager?

I did try marking them with @eager and the tests still fail. I am curious to know why it does not work, but I am not curious enough to spend more time on it at the moment or hold up the PR because of it :)

[tsegismont]

I am considering opening a ticket/PR for Hawkular Bus to introduce an Rx wrapper for the MessageProcessor class that would execute on the I/O scheduler by default.

Why not. In any case, I think it's important to keep the piece of Javadoc indicating that users of the core lib shouldn't invoke blocking calls when subscribing to the inserted events stream on the computation scheduler.

[jsanda]

I have been experimenting with TestSubscriber and I'd rather not use it.

Firstly, it's marked @experimental, so it could be changed... or simply removed in a future release.

If you take a close look, you will see that a lot of things in RxJava are marked @experimental. We may even use some of those APIs. I am less concerned about experimental APIs in test code.

Secondly, #insertDataEvents observable does not emit a terminal event until MetricsService is shutdown. So we couldn't use the method you pointed out.

Good point

Eventually, its' not thread safe because it accumulates #onNext events in an raw ArrayList.

Fair enough. Making TestSubscriber thread safe or at least having the option to do so might be a nice enhancement. I will check and see if there is already a ticket for this.

[jsanda]

I'm not sure it's safe BTW. MetricsService is not only a data access service. It also works with the scheduler to execute background tasks.

I'd be happy if there were some obvious/supported way to establish the co-located service. I didn't see one so I had to take this route. Actually, I had to submit a Metrics PR just to get this approach to work...

@jshaughn I am looking into that this week. I will create a ticket to track that effort/investigation this afternoon.

[tsegismont]

I'd be happy if there were some obvious/supported way to establish the co-located service. I didn't see one so I had to take this route. Actually, I had to submit a Metrics PR just to get this approach to work...

I'm not sure there's a simple, obvious and supported way to do this.

[tsegismont]

If you take a close look, you will see that a lot of things in RxJava are marked @experimental. We may even use some of those APIs. I am less concerned about experimental APIs in test code.

Indeed. Scary :)

[stefannegrea]

@tsegismont, can we move the new module under api folder and rename it accordingly since it is going to be consumed externally?

[tsegismont]

@tsegismont, can we move the new module under api folder and rename it accordingly since it is going to be consumed externally?

See my reply here #393 (comment)

[jsanda]

I just chatted briefly, and we agree that the module location can be discussed and changed (if necessary) after merging. I think that the other questions/concerns have been addressed.