davsclaus commented on code in PR #27576: URL: https://github.com/apache/camel/pull/27576#discussion_r4219505014
########## core/camel-management/src/test/java/org/apache/camel/management/ManagedRouteGroupExchangeCountTest.java: ########## @@ -0,0 +1,186 @@ +/* + * 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 java.util.concurrent.TimeUnit; + +import org.apache.camel.api.management.ManagedCamelContext; +import org.apache.camel.api.management.mbean.ManagedRouteGroupMBean; +import org.apache.camel.builder.RouteBuilder; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; + +import static org.awaitility.Awaitility.await; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * Tests that a route group counts each exchange once per group, even when the exchange is routed through multiple + * member routes (CAMEL-24887). + */ +@DisabledOnOs(OS.AIX) +public class ManagedRouteGroupExchangeCountTest extends ManagementTestSupport { + + @Test + public void testGroupCountsExchangeOnce() throws Exception { + ManagedCamelContext mcc = context.getCamelContextExtension().getContextPlugin(ManagedCamelContext.class); + + // send 1 message through x1 -> x2 (both in group "g") + template.sendBody("direct:x1", "Hello"); + + ManagedRouteGroupMBean group = mcc.getManagedRouteGroup("g"); + assertNotNull(group); + assertEquals(1, group.getExchangesCompleted()); + assertEquals(1, group.getExchangesTotal()); + assertEquals(0, group.getExchangesInflight()); + + // send 2 more messages + template.sendBody("direct:x1", "World"); + template.sendBody("direct:x1", "!"); + + assertEquals(3, group.getExchangesCompleted()); + assertEquals(3, group.getExchangesTotal()); + } + + @Test + public void testGroupCountsOnceWhenRoutingThroughNonGroupRoute() throws Exception { + ManagedCamelContext mcc = context.getCamelContextExtension().getContextPlugin(ManagedCamelContext.class); + + // send 1 message through x1 (group "g") -> y (no group) -> x2 (group "g") + template.sendBody("direct:x1-via-y", "Hello"); + + ManagedRouteGroupMBean group = mcc.getManagedRouteGroup("g"); + assertNotNull(group); + assertEquals(1, group.getExchangesCompleted()); + assertEquals(1, group.getExchangesTotal()); + } + + @Test + public void testGroupCountsWhenEntryIsOutsideGroup() throws Exception { + ManagedCamelContext mcc = context.getCamelContextExtension().getContextPlugin(ManagedCamelContext.class); + + // send 1 message to a route outside the group that calls x2 (in group "g") directly + template.sendBody("direct:outside", "Hello"); + + ManagedRouteGroupMBean group = mcc.getManagedRouteGroup("g"); + assertNotNull(group); + assertEquals(1, group.getExchangesCompleted()); + } + + @Test + public void testGroupCountsFailure() throws Exception { + ManagedCamelContext mcc = context.getCamelContextExtension().getContextPlugin(ManagedCamelContext.class); + + // send 1 message that will fail in x2-fail (group "g") + try { + template.sendBody("direct:x1-fail", "Hello"); + } catch (Exception e) { + // expected + } + + ManagedRouteGroupMBean group = mcc.getManagedRouteGroup("g"); + assertNotNull(group); + assertEquals(1, group.getExchangesFailed()); + assertEquals(0, group.getExchangesCompleted()); + assertEquals(0, group.getExchangesInflight()); + } + + @Test + public void testTwoGroupsOnOnePath() throws Exception { + ManagedCamelContext mcc = context.getCamelContextExtension().getContextPlugin(ManagedCamelContext.class); + + // send 1 message through g1-route (group "g1") -> g2-route (group "g2") + template.sendBody("direct:g1-route", "Hello"); + + ManagedRouteGroupMBean group1 = mcc.getManagedRouteGroup("g1"); + ManagedRouteGroupMBean group2 = mcc.getManagedRouteGroup("g2"); + assertNotNull(group1); + assertNotNull(group2); + assertEquals(1, group1.getExchangesCompleted()); + assertEquals(1, group2.getExchangesCompleted()); + } + + @Test + public void testWireTapGroupCountsOnce() throws Exception { + ManagedCamelContext mcc = context.getCamelContextExtension().getContextPlugin(ManagedCamelContext.class); + + getMockEndpoint("mock:wt-done").expectedMessageCount(1); + + template.sendBody("direct:wt", "Hello"); + + assertMockEndpointsSatisfied(); + + // wait for the wireTapped exchange to complete in route "wt-target" + await().atMost(5, TimeUnit.SECONDS) + .untilAsserted(() -> assertEquals(1L, mcc.getManagedRoute("wt-target").getExchangesCompleted())); + + // each route counts independently at the route level + assertEquals(1, mcc.getManagedRoute("wt").getExchangesCompleted()); + assertEquals(1, mcc.getManagedRoute("wt-target").getExchangesCompleted()); + + // group dedup is consistent with CamelContext dedup: the wireTapped copy is part + // of the same exchange flow, so both count it as 1 + ManagedRouteGroupMBean group = mcc.getManagedRouteGroup("g"); + assertNotNull(group); + assertEquals(1, group.getExchangesCompleted()); + assertEquals(1, group.getExchangesTotal()); Review Comment: I think this assertion is racy and expects the wrong value: - `WireTapProcessor.configureCopyExchange` uses `createCorrelatedCopy(exchange, false)`, which clears the copy's UnitOfWork (`ExchangeHelper.createCorrelatedCopy`: "do not share the unit of work"). - When the copy reaches `wt-target`, `UnitOfWorkProcessorAdvice` creates a new UnitOfWork, so the group route stack level there is 1 and the group counts it. The CamelContext MBean counts it too. - The `await` above waits on the **route** counter. `CompositePerformanceCounter` is built as `(routeMBean, camelContextMBean, mrg)`, so the route counter is updated before the group counter. This assertion can therefore read the group before the tapped exchange's increment lands. Could you wait on the group counter itself and expect 2 (the tapped copy is a separate exchange)? For example: ```java await().atMost(5, TimeUnit.SECONDS) .untilAsserted(() -> assertEquals(2, group.getExchangesCompleted())); ``` The comment about CamelContext dedup should change accordingly. ########## docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_22.adoc: ########## @@ -2365,3 +2365,10 @@ overwritten. Applications that aggregate classes outside the default whitelist through the replicated repository without supplying their own `hazelcastInstance` must now provide a `Config` with a `JavaSerializationFilterConfig` covering their class names. + +=== camel-management - Route group exchange counters count once per exchange again Review Comment: This is appended at the end of the file, which is inside `== Upgrading Camel 4.21 to 4.22`. Since it is a 4.22.1 → 4.22.2 change, could you move it under `== Upgrading from 4.22.1 to 4.22.2` (near the top of the file)? ########## core/camel-api/src/main/java/org/apache/camel/spi/UnitOfWork.java: ########## @@ -247,6 +247,20 @@ public interface UnitOfWork { */ int routeStackLevel(boolean includeRouteTemplate, boolean includeKamelet); + /** + * Gets the number of routes on the route stack that this {@link UnitOfWork} currently is being routed through, + * which match the given filter. + * <p/> + * For example to know whether the current route is the outermost route of a route group on the stack. + * + * @param filter the filter + * @return the number of matching routes on the route stack + * @since 4.23 + */ + default int routeStackLevel(Predicate<Route> filter) { Review Comment: Minor: with a default of `0`, a custom `UnitOfWork` that doesn't override this makes `ManagedRouteGroup` count at every member route again, which is the behaviour this PR fixes. A real value can't be computed from the existing API, so `0` is probably the pragmatic default. A Javadoc note that implementations should override it would make this explicit. ########## docs/user-manual/modules/ROOT/pages/route-group.adoc: ########## @@ -177,6 +177,10 @@ Camel has optional support for xref:jmx.adoc[JMX management]. This includes mana The following code will get the JMX MBean `ManagedRouteGroupMBean` for the route group with name `order`. Using this MBean you can get metrics, and also operations to start and stop all routes in this group. +Each exchange is counted once per group, even when it is routed through several member routes of the group, Review Comment: This is true only for *nested* calls. If a route outside the group calls two members one after the other, each member is the outermost group route on the stack, so the exchange is counted twice. `ManagedRouteGroupTest` asserts exactly that for group `second` (`assertEquals(2, val)`). Could the wording say something like "counted once per entry into the group; calls nested inside another route of the same group are not counted again"? The same applies to the two upgrade-guide entries. -- 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]
