gnodet commented on code in PR #24356: URL: https://github.com/apache/camel/pull/24356#discussion_r3504896303
########## core/camel-management/src/test/java/org/apache/camel/management/LoadThroughputTest.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.management; + +import org.apache.camel.management.mbean.LoadThroughput; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@DisabledOnOs(OS.AIX) +public class LoadThroughputTest { + + @Test + public void testInitialValueIsZero() { + LoadThroughput t = new LoadThroughput(); + assertEquals(0.0, t.getThroughput()); + } + + @Test + public void testConvergesToSteadyRate() throws Exception { + LoadThroughput t = new LoadThroughput(); + + // simulate 1 exchange per second for 120 seconds (well past 1-minute EWMA window) + long total = 0; + t.update(total); + Thread.sleep(10); + for (int i = 0; i < 120; i++) { + total++; Review Comment: The test introduces multiple `Thread.sleep()` calls. Per project conventions: > *"New test code MUST NOT introduce `Thread.sleep()` calls. Use the Awaitility library instead."* Since `LoadThroughput` relies on `StopWatch` internally, it requires real elapsed time. A better long-term approach would be an injectable time source for fully deterministic tests. At minimum, the sleep-based timing should be replaced with Awaitility polling assertions. ########## core/camel-management/src/main/java/org/apache/camel/management/mbean/LoadThroughput.java: ########## @@ -40,14 +47,10 @@ public void update(long currentReading) { long time = watch.takenAndRestart(); if (time > 0) { long delta = currentReading - last; - if (delta > 0) { - // need to calculate with fractions - thp = (1000d / time) * delta; - } else { - thp = 0; - } - } else { - thp = 0; + // instantaneous rate in exchanges/second for this interval + double instantRate = (1000d / time) * delta; Review Comment: The old code guarded `if (delta > 0)` and set `thp = 0` for non-positive deltas. With EWMA, a negative `instantRate` (from `currentReading < last`, e.g. after a route restart that resets the exchange counter) bleeds into `thp` and takes ~60 seconds to decay back toward zero. Consider clamping: ```suggestion double instantRate = Math.max(0, (1000d / time) * delta); ``` ########## dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/OverviewTab.java: ########## @@ -637,6 +644,34 @@ private void renderInfoPanel(Frame frame, Rect area) { frame.renderWidget(Paragraph.builder().text(Text.from(lines)).build(), inner); } + /** + * Snap to a nice Y-axis ceiling using a 1-2-5 sequence (scaled by THROUGHPUT_SCALE). For example with scale=100: + * 0.20 → 1, 1.5 → 2, 3 → 5, 7 → 10, 15 → 20, 35 → 50, 80 → 100, etc. + */ + private static long niceMax(long rawMax) { + long s = MetricsCollector.THROUGHPUT_SCALE; Review Comment: `niceMax` and `formatThroughput` are duplicated between `OverviewTab` and `EndpointsTab` with **different** implementations: - **Here** (`OverviewTab`): hardcoded array of 10 steps up to `1000 * THROUGHPUT_SCALE`; falls back to `rawMax` (no rounding) beyond that. - **`EndpointsTab`**: uses a `while(true)` loop with 1-2-5 step sequence that scales indefinitely. The same data could get different Y-axis ceilings depending on which chart displays it. Consider extracting both methods to a shared utility (e.g., on `MetricsCollector` where `THROUGHPUT_SCALE` already lives) with a single correct implementation. ########## core/camel-management/src/test/java/org/apache/camel/management/LoadThroughputTest.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.management; + +import org.apache.camel.management.mbean.LoadThroughput; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@DisabledOnOs(OS.AIX) +public class LoadThroughputTest { Review Comment: This test is pure Java math + `Thread.sleep` — no JMX interaction. The `@DisabledOnOs(OS.AIX)` annotation may have been cargo-culted from other management tests that interact with platform MBeans. Is there a specific AIX reason to skip this test? If not, consider removing it. ########## core/camel-management/src/test/java/org/apache/camel/management/LoadThroughputTest.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.management; + +import org.apache.camel.management.mbean.LoadThroughput; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@DisabledOnOs(OS.AIX) +public class LoadThroughputTest { + + @Test + public void testInitialValueIsZero() { + LoadThroughput t = new LoadThroughput(); + assertEquals(0.0, t.getThroughput()); + } + + @Test + public void testConvergesToSteadyRate() throws Exception { + LoadThroughput t = new LoadThroughput(); + + // simulate 1 exchange per second for 120 seconds (well past 1-minute EWMA window) + long total = 0; + t.update(total); + Thread.sleep(10); + for (int i = 0; i < 120; i++) { + total++; + Thread.sleep(10); + t.update(total); + } + + // should converge close to the steady rate + assertTrue(t.getThroughput() > 0, "Throughput should be positive for steady input"); + } + + @Test + public void testSmoothing() throws Exception { + LoadThroughput t = new LoadThroughput(); + Review Comment: This assertion (`> 0`) would also pass with the old raw throughput calculation. Consider tightening it to verify that EWMA smoothing is actually converging toward the expected steady-state rate (e.g., within a reasonable tolerance of the true rate). -- 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]
