ARTEMIS-2308 Support exporting metrics #2681

jbertram · 2019-05-26T15:47:35Z

Based on the conversation from #2626 I decided to pursue Micrometer integration which required a completely different approach than the one I took before. There is now no direct integration with Prometheus. The broker uses Micrometer APIs and a plugin structure was added to support specific integrations. A Prometheus plugin is available at https://github.com/jbertram/artemis-prometheus-metrics-plugin.

franz1981

The approach is clean and won't couple the broker with Prometheus API and deps so is +100 for me.
After addressed thread-safeness issues, refacoring of some String constants and maybe considered the MetricsManager API proposal (that's up to u, given that's just personal taste!) LGTM

franz1981 · 2019-05-27T13:29:25Z

...ore-client/src/main/java/org/apache/activemq/artemis/api/core/management/AddressControl.java

@@ -101,19 +103,19 @@
   @Attribute(desc = "names of all bindings (both queues and diverts) bound to this address")
   String[] getBindingNames() throws Exception;

-   @Attribute(desc = "number of messages added to all the queues for this address")
+   @Attribute(desc = "number of messages currently in all queues bound to this address (includes scheduled, paged, and in-delivery messages)")


why no refactor this desc as you've done for the others? eg ROUTED_MESSAGE_COUNT_DESCRIPTION

I didn't refactor the description here because I'm not actually exported this metric since it will be easy to aggregate all the message counts of all the queues on the address in whatever system the metrics are exported to.

franz1981 · 2019-05-27T13:44:41Z

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java

+
+      if (server != null && server.getMetricsManager() != null) {
+         MetricsManager metricsManager = server.getMetricsManager();
+         metricsManager.registerQueueGauge("message.count", addressName, queueName, pendingMetrics, metrics -> Double.valueOf(pendingMetrics.getMessageCount()), QueueControl.MESSAGE_COUNT_DESCRIPTION);


Would be nice to collect in some static final fields all the metrics IDs eg message.count
to avoid mistakes while registering them

I'm not entirely sure what you mean. Do you mean declaring something like:

static final String MESSAGE_COUNT = "message.count";

And then using MESSAGE_COUNT in the method invocation?

franz1981 · 2019-05-27T14:29:39Z

...mis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/MetricsManager.java

+
+   private MeterRegistry meterRegistry;
+
+   private Map<String, List<Meter>> meters = new HashMap<>();


meters::remove could be called by different threads eg QueueImpl::unregisterMeters and
same for meters::add: probably is better to use a thread-safe map here ie ConcurrentHashMap.
List<Meter> instead could be left an ArrayList, because I see that there isn't a strong concurrency requirements.

Agreed. I'll change that.

franz1981 · 2019-05-27T14:34:00Z

...mis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/MetricsManager.java

+      return meterRegistry;
+   }
+
+   public void registerQueueGauge(String metricName, String address, String queue, Object object, ToDoubleFunction f, String description) {


I would use QueuePendingMessageMetrics queueState instead of Object object to help us to remember what to use there/why

This isn't possible because a QueuePendingMessageMetrics isn't always passed into the method (e.g. for all the queue metrics that aren't tracked by the QueuePendingMessageMetrics object).

franz1981 · 2019-05-27T15:11:12Z

...mis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/MetricsManager.java

+import org.apache.activemq.artemis.api.core.management.ResourceNames;
+import org.apache.activemq.artemis.core.server.ActiveMQServerLogger;
+
+public class MetricsManager {


That's a huge comment; API change proposal...

private final String brokerName; private final MeterRegistry meterRegistry; private final Map<String, List<Meter>> meters = new ConcurrentHashMap<>(); public MetricsManager(String brokerName, ActiveMQMetricsPlugin metricsPlugin) { this.brokerName = brokerName; meterRegistry = metricsPlugin.getRegistry(); Metrics.globalRegistry.add(meterRegistry); new JvmMemoryMetrics().bindTo(meterRegistry); } public MeterRegistry getMeterRegistry() { return meterRegistry; } @FunctionalInterface public interface MetricGaugeBuilder { void register(String metricName, Object state, ToDoubleFunction f, String description); } public void registerQueueGauge(String address, String queue, Consumer<MetricGaugeBuilder> builder) { final MeterRegistry meterRegistry = this.meterRegistry; if (meterRegistry == null) { return; } final List<Gauge.Builder> newMeters = new ArrayList<>(); builder.accept((metricName, state, f, description) -> { Gauge.Builder meter = Gauge.builder("artemis." + metricName, state, f).tag("broker", brokerName).tag("address", address).tag("queue", queue).description(description); newMeters.add(meter); }); final String resource = ResourceNames.QUEUE + queue; this.meters.compute(resource, (s, meters) -> { //the old meters are ignored on purpose meters = new ArrayList<>(newMeters.size()); for (Gauge.Builder gauge : newMeters) { meters.add(gauge.register(meterRegistry)); } return meters; }); } public void registerAddressGauge(String address, Consumer<MetricGaugeBuilder> builder) { final MeterRegistry meterRegistry = this.meterRegistry; if (meterRegistry == null) { return; } final List<Gauge.Builder> newMeters = new ArrayList<>(); builder.accept((metricName, state, f, description) -> { Gauge.Builder meter = Gauge.builder("artemis." + metricName, state, f).tag("broker", brokerName).tag("address", address).description(description); newMeters.add(meter); }); final String resource = ResourceNames.ADDRESS + address; this.meters.compute(resource, (s, meters) -> { //the old meters are ignored on purpose meters = new ArrayList<>(newMeters.size()); for (Gauge.Builder gauge : newMeters) { meters.add(gauge.register(meterRegistry)); } return meters; }); } public void registerBrokerGauge(Consumer<MetricGaugeBuilder> builder) { final MeterRegistry meterRegistry = this.meterRegistry; if (meterRegistry == null) { return; } final List<Gauge.Builder> newMeters = new ArrayList<>(); builder.accept((metricName, state, f, description) -> { Gauge.Builder meter = Gauge.builder("artemis." + metricName, state, f).tag("broker", brokerName).description(description); newMeters.add(meter); }); final String resource = ResourceNames.BROKER + "." + brokerName; this.meters.compute(resource, (s, meters) -> { //the old meters are ignored on purpose meters = new ArrayList<>(newMeters.size()); for (Gauge.Builder gauge : newMeters) { meters.add(gauge.register(meterRegistry)); } return meters; }); } public void remove(String component) { meters.computeIfPresent(component, (s, meters) -> { if (meters == null) { return null; } for (Meter meter : meters) { Meter removed = meterRegistry.remove(meter); if (ActiveMQServerLogger.LOGGER.isDebugEnabled()) { ActiveMQServerLogger.LOGGER.debug("Removed meter: " + removed.getId()); } } return null; }); }

That could be used with:

if (server != null && server.getMetricsManager() != null) { server.getMetricsManager().registerQueueGauge(queueName, addressName, builder -> { builder.register("message.count", pendingMetrics, metrics -> Double.valueOf(pendingMetrics.getMessageCount()), QueueControl.MESSAGE_COUNT_DESCRIPTION); builder.register(//...other useful metrics

Maybe it's just me, but I think the existing implementation is easier to read. :)

Is there a particular benefit to your implementation? I'm assuming it has better performance for some esoteric reason. :)

Maybe :P It has a better behaviour under load, because it won't call many times put on the concurrent hash map, just once...

michaelandrepearce · 2019-05-28T08:50:18Z

docs/user-manual/en/metrics.md

+Important runtime metrics have been instrumented via the Micrometer API, and
+all a user needs to do is implement `org.apache.activemq.artemis.core.server.metrics.ActiveMQMetricsPlugin`
+in order to instantiate and configure a `io.micrometer.core.instrument.MeterRegistry`
+implementation. Many implementations of `MeterRegistr` are available from


sentance seems to end abruptly i assume you meant to put a link to the micrometer web page with a list of these? also looks to be a small typo in MeterRegistr, assume it should be MeterRegister

michaelandrepearce · 2019-05-28T08:51:38Z

Great stuff. +1 on this from me. Nice abstraction from implementations. Added one comment on docs where it seems something ends mid-sentance.

michaelandrepearce · 2019-05-28T08:56:40Z

artemis-server/src/main/resources/schema/artemis-configuration.xsd

@@ -1012,6 +1012,33 @@
            </xsd:complexType>
         </xsd:element>

+         <xsd:element name="metrics-plugin" maxOccurs="1" minOccurs="0">


Is the test version of artemis-configuration.xsd updated also, to keep them in sync.

clebertsuconic · 2019-05-28T12:42:03Z

It would be nice now to have the components repo, and an option to include it with the installation.
Great stuff...

is this ready to merge?

franz1981 · 2019-05-28T12:53:20Z

artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/AddressInfo.java

+   }
+
+   public void unregisterMeters(MetricsManager metricsManager) {
+      if (metricsManager != null) {


Given that metricsManager is volatile, it should be called in a local variable and if not null, used to call its method

Ups sorry, reading it from the web I haven't noticed from where metricsManager was coming

franz1981 · 2019-05-28T12:54:39Z

...mis-server/src/main/java/org/apache/activemq/artemis/core/server/metrics/MetricsManager.java

+         .description(description)
+         .register(meterRegistry);
+
+      meters.add(meter);


This risk to be called by different threads: take a look to the implementation I've posted above (that is using the compute method, that is thread safe for a specific key).

jbertram · 2019-05-28T16:28:54Z

I just pushed the latest changes:

Fixed documentation (Mike)
Fixed XSD (Mike)
Addressed thread-safety issues (Franz)
Added static finals for metric names (Franz)

clebertsuconic · 2019-05-29T19:07:30Z

@jbertram there still work to be improved, mainly when we get the repository, but this is good as is for now. We can send more commits later.

michaelandrepearce · 2019-05-30T04:38:59Z

docs/user-manual/en/metrics.md

+**Address**
+
+- routed.message.count
+- unrouted.message.count


@jbertram what about paging size? This is quite important thing to watch out for.

franz1981 reviewed May 27, 2019

View reviewed changes

jbertram force-pushed the ARTEMIS-2308-3 branch from 11ff0bd to bee50bc Compare May 27, 2019 18:24

michaelandrepearce reviewed May 28, 2019

View reviewed changes

franz1981 reviewed May 28, 2019

View reviewed changes

jbertram force-pushed the ARTEMIS-2308-3 branch from bee50bc to 1e73f51 Compare May 28, 2019 16:26

ARTEMIS-2308 Support exporting metrics

9be3452

jbertram force-pushed the ARTEMIS-2308-3 branch from 1e73f51 to 9be3452 Compare May 28, 2019 16:41

franz1981 approved these changes May 29, 2019

View reviewed changes

asfgit closed this in a7641e6 May 29, 2019

michaelandrepearce reviewed May 30, 2019

View reviewed changes

jbertram deleted the ARTEMIS-2308-3 branch June 10, 2021 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARTEMIS-2308 Support exporting metrics #2681

ARTEMIS-2308 Support exporting metrics #2681

jbertram commented May 26, 2019

franz1981 left a comment

franz1981 May 27, 2019

jbertram May 27, 2019

franz1981 May 27, 2019

jbertram May 27, 2019

franz1981 May 27, 2019

jbertram May 27, 2019

franz1981 May 27, 2019

jbertram May 27, 2019

franz1981 May 27, 2019 •

edited

jbertram May 27, 2019

franz1981 May 27, 2019

michaelandrepearce May 28, 2019 •

edited

michaelandrepearce commented May 28, 2019 •

edited

michaelandrepearce May 28, 2019

clebertsuconic commented May 28, 2019

franz1981 May 28, 2019

franz1981 May 28, 2019

franz1981 May 28, 2019

jbertram commented May 28, 2019

clebertsuconic commented May 29, 2019

michaelandrepearce May 30, 2019 •

edited


		private MeterRegistry meterRegistry;

		private Map<String, List<Meter>> meters = new HashMap<>();

ARTEMIS-2308 Support exporting metrics #2681

ARTEMIS-2308 Support exporting metrics #2681

Conversation

jbertram commented May 26, 2019

franz1981 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franz1981 May 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelandrepearce May 28, 2019 • edited

Choose a reason for hiding this comment

michaelandrepearce commented May 28, 2019 • edited

Choose a reason for hiding this comment

clebertsuconic commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbertram commented May 28, 2019

clebertsuconic commented May 29, 2019

michaelandrepearce May 30, 2019 • edited

Choose a reason for hiding this comment

franz1981 May 27, 2019 •

edited

michaelandrepearce May 28, 2019 •

edited

michaelandrepearce commented May 28, 2019 •

edited

michaelandrepearce May 30, 2019 •

edited