feat(prometheus): Add native /metrics endpoint - #2402
Conversation
|
Maybe an idea for improvement for broker with many queues an topics. Collect metrics periodically in the background and answer http request with the last collected result. This avoids a lot of preasure on the broker and keep getting results back to prometheus "quickly" available. In my experience is better to not not the most actual data but have at most actual data by providing broker stability. |
|
I like the idea @graben. I know for RabbitMQ, there's an internal metrics store that both their console and their prometheus plugin scrape from, so the plugin still has 0 load on idle, does that pattern fix what you're getting at? A couple of other parallel additions I might also just add in this vein:
|
|
Looking into the source a little more, it looks like everything is already just describing MBeans for metrics anyways. The reason RabbitMQ has that pattern is that those metrics need to be actually aggregated, ActiveMQ MBeans are already ready to use. So I'm not sure I see the savings that pre-caching metrics has over lazily updating a cache in response to a caller. I'll still add the streaming response and the lazy TTL cache, as those are definitely worthwhile (imo). |
|
|
||
| @Override | ||
| protected void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||
| boolean perObject = request != null && "true".equalsIgnoreCase(request.getParameter("per_object")); |
There was a problem hiding this comment.
I prefer to use final as much as possible
There was a problem hiding this comment.
Updated a bunch of stuff to use final, lmk if you see anything that could be but isn't.
| ObjectName pattern = new ObjectName("org.apache.activemq:type=Broker,brokerName=*"); | ||
| Set<ObjectName> brokers = mBeanServer.queryNames(pattern, null); | ||
|
|
||
| for (MetricDefinition metric : BROKER_METRICS) { |
There was a problem hiding this comment.
Just as a curiosity, is there any practical difference between iterating the brokers inside the metrics loop or doing the other way around ?
for (ObjectName broker : brokers) {
for (MetricDefinition metric : BROKER_METRICS) {
...
}
}
Do we need to have the metrics grouped together ?
There was a problem hiding this comment.
To be honest, not really. Though if we're implementing streaming responses then this makes it so metrics are grouped per object, which seems intuitive to me.
There was a problem hiding this comment.
LICENSE file should be added to the archive from the central file, not stored redundantly
|
Excited to get a modern metrics endpoint into the broker. I think there are a couple architecture points that need to be sorted out. Use of hand-crafted JSON output-- to avoid a security attack vector his needs serious character escape handling, or (my preference) leverage Jackson JSON processing library. |
@mattrpav I'm not sure I understand the security risk JSON output has or the kind of attack you're foreseeing here. Could you tell me more? Maybe link some previous CVEs on this? |
|
@graben on the lazy cache I did some reading and some tests. Prometheus recommends:
The logic being that it's whoever's scraping Prometheus's responsibility to manage time-stamps. I also did some perf testing to see what affect this would actually have on a real broker using worst-practices. I spun up an m5.xlarge broker and put some queues on it (1_000, 5_000, and 25_0000). Callers would scrape Prometheus every 15s. I tried with 1, 5, and 20 callers. I also ran this under no-load and a load of ~2k msg/s. The best practice is to only touch this endpoint from one caller once every 30-60s and only touch the per-objects when you really need them. Here are the results: 
The CPU load is noticeable, but even with 20 concurrent scrapers on 25k queues with load, scrape times were well under 10s. Importantly, the CPU spikes on 20 concurrent scrapers weren't much worse than 1 or 5 scrapers. There are also a lot of edge cases to handle with a lazy cache (deleted queues, race conditions with concurrent scrapes, etc) and it would not provide much benefit against worst practices. And a scheduled cache also puts this non-zero load on the broker, even if it's not being used. |
The write take data from the broker unsanitized. So things like brokerName, queue/topic names, and clientIds are all user-defined strings that create attack surface for malformed json. |
Jerald
left a comment
Jerald
left a comment
There was a problem hiding this comment.
Long-time ActiveMQ lurker here. I have some interest in Prometheus support myself, very excited to see it happening!
I have a few opportunities for improvement I noticed, not sure what others think, but none of them seem to impact correctness, just future improvements.
| // Metrics can be easily extended by adding them here | ||
| private static final MetricDefinition[] BROKER_METRICS = { |
There was a problem hiding this comment.
Is there an opportunity for this metric list to be automatically generated from the registered MBeans on the broker? I can see how a system could deterministically turn the PascalCase name into a snake_case name for Prometheus (although the names would not be the same as right now).
There was a problem hiding this comment.
In my mind one of the great things about Prometheus is the ability for people to standardize on dashboards. I'm hesitant to do this because it might create churn for dashboards if MBean names ever change.
Though maybe I'm not understanding something. What's the use case you envision for this?
There was a problem hiding this comment.
My mental model (but contributors please chime in) is that the MBean names (and their attributes) are part of the public API, in the sense that ActiveMQ wouldn't break them without documenting it and a Semver change.
Which is to say, MBeans changing is already a breaking change for anyone using them for monitoring, there's no difference in the impact here for Prometheus.
The use-case I see is that this prevents the MBean metrics and Prometheus metrics from diverging over time. Right now, you took some editorial liberties with translating the MBean attribute names to Prometheus metric names. While I don't have an issue with this per se, it does mean the two ways of monitoring an ActiveMQ broker will necessarily diverge a bit. (This also reduces the effort to expose new metrics to Prometheus, because there simply would be no extra effort.)
| final Set<ObjectName> queues; | ||
| final Set<ObjectName> topics; |
There was a problem hiding this comment.
What's the reason to split queue and topic information like this? Could this be a combined "destinations" set without much impact?
There was a problem hiding this comment.
Yes, they must be split. Queues and Topics are separate namespace regions, so it is legit (and spec compliant) to have a queue and topic with the same name.
There was a problem hiding this comment.
Ah! That's a good point, the destination name is not globally unique. I believe it would be the tuple of (destination_type, destination_name) that's unique, does that sound right? (Effectively encoding the same info as the destination URI.)
| queues = mBeanServer.queryNames(new ObjectName( | ||
| "org.apache.activemq:type=Broker,brokerName=*,destinationType=Queue,destinationName=*"), null); | ||
| topics = mBeanServer.queryNames(new ObjectName( | ||
| "org.apache.activemq:type=Broker,brokerName=*,destinationType=Topic,destinationName=*"), null); |
There was a problem hiding this comment.
Are there any other destinationType values used in ActiveMQ? (Notably I'm unsure what composite/virtual destinations look like)
There was a problem hiding this comment.
I'm still relatively new to ActiveMQ so I'm not totally sure. Maybe this @sergio-d-lemos could answer this one?
There was a problem hiding this comment.
I just did a little digging around, but struggled to find the exact spot defining this destinationType field. I think this might be the transitive source though, showing that the temp destination types are also valid: https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/command/ActiveMQDestination.java#L382-L395
Either way, this leads me to believe that you should assume temp destinations have a different type unless you can prove that to not be true. Missing them here would lead to incomplete metric scraping, possibly being very confusing to some people.
| } | ||
|
|
||
| for (final MetricDefinition metric : BROKER_METRICS) { | ||
| final String metricName = "activemq_broker_" + metric.name; |
There was a problem hiding this comment.
Is prefixes like this the conventional pattern for "namespacing" in the prometheus metrics world?
There was a problem hiding this comment.
Yes the standard is namespace_subsystem_name. So you have activemq_broker_connections and activemq_queue_messages.
| private String resolveStringAttribute(final MBeanServer mBeanServer, final ObjectName name, final String attribute) { | ||
| // Partial results are better than no results if something goes wrong | ||
| try { | ||
| final Object value = mBeanServer.getAttribute(name, attribute); | ||
| if (value instanceof String) { | ||
| return (String) value; | ||
| } | ||
| } catch (final Exception exception) { | ||
| LOG.debug("Skipping object {}: identity attribute {} unavailable", name, attribute, exception); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Is there an opportunity to downcast the MBeans to their concrete class as a way to remove some of these string attribute checks?
I'm usually weary of those being brittle in the face of attribute changes, though it's very unlikely to be a concern here.
|
@hai-ben would you have time/interest in running another load test in a different scenario? I know that in some CPU constrained environments with lots of thread churn, ActiveMQ can quickly buckle due to the impact of starving threads of CPU cycles. Given that Prometheus scrapes look to create a relatively-large single-threaded workload, I'm very interested to understand how (if at all) Prometheus scrapes impact ActiveMQ when everything is competing for CPU cycles. As I saw you're using EC2 instances for testing (m5.xlarge), a comparable instance type with CPU constraints would be something like t3.micro. Running a load test using short-lived clients that open/close dedicated connections should easily induce the sort of issues I'm talking about. |
| final String metricName = "activemq_" + typeLower + "_" + metric.name; | ||
| writeMetadata(writer, metricName, metric.withFormattedHelp(typeLower)); | ||
| for (final ObjectName destination : destinations) { | ||
| final String brokerName = sanitizeLabel(destination.getKeyProperty("brokerName")); | ||
| final String destinationName = sanitizeLabel(destination.getKeyProperty("destinationName")); | ||
| final String labels = String.format("broker=\"%s\",destination=\"%s\"", brokerName, destinationName); | ||
| writeSample(writer, metricName, labels, getNumber(mBeanServer, destination, metric.attribute)); | ||
| } |
There was a problem hiding this comment.
I'm realizing there's an alternate way to encode these metrics that could be used. Right now, the destination type (queue/topic) is encoded in the metric name itself, despite all destinations sharing a set of emitted metrics (though queues and topics do have a few metrics unique to themselves). But because the underlying metrics are mostly overlapping, the destination type can also be disambiguated by a label instead of a unique metric name.
For example, the current format for a destination metric name is something like this: activemq_$destType_$metricName (activemq_queue_connections). And it would have labels like this: broker=$brokerName, destination=$destName (broker="foo",destination="FOO.BAR").
But it should be near equivalent to use a destination name like activemq_destination_$metricName and then have labels broker=$brokerName, destinationType=$destType, and destination=$destName.
Both these approaches are valid (as far as I'm aware), so it's mostly a matter of taste. One upside that I see is having destination type as a label more closely aligns with how the MBeans look. If someone were to convert a tool from using MBeans for metrics and move to Prometheus, I'd expect this approach would be an easier transition (but whether or not that matters is a separate question).
| return "unknown"; | ||
| } | ||
| // See: https://prometheus.io/docs/instrumenting/exposition_formats/ | ||
| return value.replace("\\", "\\\\").replace("\"", "\\\"").replace("\n", "\\n").replace("\r", "\\r"); |
There was a problem hiding this comment.
I was just reviewing the prometheus docs on this format, but I realized it doesn't state anything about the CR (\r) needing escaping, only the LF (\n).
Do you know if this is an undocumented assumption in Prometheus? (I've not run into it before myself.) Or should this escaping be removed?
Either way, it would be great (but I totally understand if impractical) to have a real Prometheus parser used as a control in some sort of test here. That will help validate that ActiveMQ is not just emitting the metrics that are expected, but also emitting them in a format that can be consumed by Prometheus correctly. An adversarial test with \r would be a perfect starting point if there's a test suite like this.
There was a problem hiding this comment.
No clue. I think we want to shift this to a library anyways which will handle all of this.
@Jerald I suggest you run your scenarios against ActiveMQ v6.3.1+ with Virtual Threads enabled. In addition to Virtual Threads, a number of hotspots have been removed. I'd be interested if you find hot spots and I'll remove them. |
What I was missing was that users that create destinations on the broker would be different than users consuming metrics. And a bad sanitizer could let the former break the latter. Prometheus output isn't JSON, but I'll swap to the official Prometheus parser/output library to address the concern. |
Sure @Jerald. I'll try those tests with @mattrpav's suggestion of using virtual threads. |
Oh that's great to hear! I'm still stuck using older versions of ActiveMQ in production, with virtual threads likely a ways out, so that's where my thoughts are coming from. I'm happy for this to be a worry of the past! Is there an opportunity for this (EDIT: Prometheus) to be backported to the 5.19 branch? Or would this only be expected to be available in 6.x? (Feel free to leave these questions until later if they detract from the review here.) |
Keep in mind-- the broker and clients are wire-compatible across versions. You can run 6.3.x broker and 5.19.x clients no problem.
Unfortunately, no. Virtual Threads need JDK 25 support and that is only technically possible in v6.3.x |
5 participants
New activemq-prometheus module that adds a
/metricsendpoint to the broker that vends metrics in the Prometheus format without requiring any sort of open JMX port. The plugin relies on the existing Jetty auth systems and only scrapes MBean during a request, so it doesn't put any additional load on the broker unless someone is actually using it and users define their own Prometheus scrapers and monitoring setup.It depends only on the jakarta stuff jetty already does.
I've included an example grafana dashboard in this PR just as a way to get up an running quickly (./activemq-prometheus/src/main/resources/example-grafana-dashboard.json). This wouldn't be in the final PR, instead something like it would be uploaded to grafana.com/dashboards.
See Discussion: #2226
Questions for the community: