gnodet commented on code in PR #24658:
URL: https://github.com/apache/camel/pull/24658#discussion_r3570067642
##########
core/camel-management/src/main/java/org/apache/camel/management/mbean/ManagedPerformanceCounter.java:
##########
@@ -65,6 +65,11 @@ public abstract class ManagedPerformanceCounter extends
ManagedCounter
private long[] percentileWindow;
private int percentileIndex;
private int percentileCount;
+ // rolling 1-minute exchange rate (Extended statistics only)
+ private static final int EXCHANGE_RATE_WINDOW_SECONDS = 60;
+ private final Object exchangeRateLock = new Object();
+ private long[] exchangeRateSeconds;
Review Comment:
The `synchronized` approach here is inconsistent with the lock-free pattern
used by the rest of this class (percentile window, `LoadThroughput`). Since
`updateExchangeRate()` is called from `completedExchange()` and
`failedExchange()` — the hottest paths in the counter — adding contention here
may not be desired.
The existing percentile window uses a simple ring buffer without
synchronization. Consider adopting the same approach, or at minimum documenting
why synchronization is needed here but not for the percentile window.
##########
core/camel-management-api/src/main/java/org/apache/camel/api/management/mbean/ManagedPerformanceCounterMBean.java:
##########
@@ -111,6 +111,12 @@ public interface ManagedPerformanceCounterMBean extends
ManagedCounterMBean {
@ManagedAttribute(description = "99th percentile of recent processing
times [milliseconds]. Requires Extended statistics level, returns -1
otherwise.")
long getProcessingTimeP99();
+ /**
+ * @since 4.22
+ */
+ @ManagedAttribute(description = "Rolling 1-minute exchange rate
[exchanges/minute]. Requires Extended statistics level, returns -1 otherwise.")
Review Comment:
The description says "exchange rate [exchanges/minute]" but the
implementation returns a raw count, not a rate. After PR #24645 merged
`getThroughput()` (EWMA exchanges/second) into this same interface, having both
metrics with similar-sounding names but different semantics could be confusing.
Consider:
- Renaming to `getExchangeCount1m()` to clarify it's a count, not a rate
- Or dividing by 60 in the implementation to return an actual per-second rate
##########
core/camel-management/src/test/java/org/apache/camel/management/ManagedPercentileStatisticsTest.java:
##########
@@ -59,12 +59,14 @@ public void testPercentileStatsOnRoute() throws Exception {
Long p50 = (Long) mbeanServer.getAttribute(on, "ProcessingTimeP50");
Long p95 = (Long) mbeanServer.getAttribute(on, "ProcessingTimeP95");
Long p99 = (Long) mbeanServer.getAttribute(on, "ProcessingTimeP99");
+ Long rate = (Long) mbeanServer.getAttribute(on, "ExchangeRate1m");
assertTrue(p50 >= 0, "p50 should be >= 0, was: " + p50);
assertTrue(p95 >= 0, "p95 should be >= 0, was: " + p95);
assertTrue(p99 >= 0, "p99 should be >= 0, was: " + p99);
assertTrue(p50 <= p95, "p50 should be <= p95, was p50=" + p50 + "
p95=" + p95);
assertTrue(p95 <= p99, "p95 should be <= p99, was p95=" + p95 + "
p99=" + p99);
+ assertEquals(5, rate.longValue());
}
@Test
Review Comment:
This assertion assumes all 5 exchanges complete within the 60-second window,
which is nearly guaranteed. However, the bucketing by `currentSecond % 60`
means there is a theoretical edge case at second boundaries. Consider adding a
brief comment documenting this assumption.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
