gnodet-bot commented on code in PR #29398:
URL: https://github.com/apache/camel/pull/29398#discussion_r4505473341
##########
components/camel-chaos/src/main/java/org/apache/camel/component/chaos/ChaosAdvisor.java:
##########
@@ -85,6 +86,101 @@ enum Kind {
record Suggestion(String route, Kind kind, String recipe, String title,
String rule, String why, int rank) {
}
+ /**
+ * A kind of fault tried in a route (CAMEL-26728): by which rules, how
often they fired there, how the exchanges
+ * they faulted came out (counted per rule, so a rule that fired in more
routes counts in each), and the outcomes of
+ * their expectations.
+ */
+ record KindTried(Kind kind, List<String> rules, long fired, long
recovered, long handled, long failed,
+ List<String> expectations) {
+ }
+
+ /** What has been tried in a route, and the kinds that the suggestions
offer for it and no rule tried yet. */
+ record RouteTried(String route, List<KindTried> tried, List<Kind>
notTried) {
+ }
+
+ /** The kind of fault of a rule. */
+ static Kind kindOf(ChaosRule rule) {
+ if (rule.lifecycle != null || rule.fault == ChaosRule.Fault.LIFECYCLE)
{
+ return Kind.LIFECYCLE;
+ }
+ if (rule.hooks.size() == 1 && rule.hooks.contains(ChaosRule.Hook.DLQ))
{
+ return Kind.DEAD_LETTER;
+ }
+ boolean consumer = rule.hooks.stream().allMatch(h -> h ==
ChaosRule.Hook.POLL || h == ChaosRule.Hook.ENTRY);
+ return switch (rule.fault) {
+ case DELAY -> Kind.SLOW;
+ case DROP -> consumer ? Kind.CONSUMER : Kind.LOST;
+ case CORRUPT -> Kind.CORRUPT;
+ case DUPLICATE -> Kind.DUPLICATE;
+ default -> consumer ? Kind.CONSUMER : Kind.ERRORS;
+ };
+ }
+
+ /**
+ * What has been tried per route: the kinds of fault of the rules (current
and removed) that fired in it, and the
+ * kinds the suggestions offer for it that none tried. The routes in the
order of the suggestions, then the others.
+ */
+ static List<RouteTried> tried(List<ChaosRule> rules, Map<String,
Set<Kind>> suggested, long now) {
+ // route -> kind -> the rules of that kind that fired in the route
+ Map<String, Map<Kind, List<ChaosRule>>> byRoute = new
LinkedHashMap<>();
+ suggested.keySet().forEach(route -> byRoute.put(route, new
LinkedHashMap<>()));
+ for (ChaosRule rule : rules) {
+ for (Map.Entry<String, LongAdder> e :
rule.firedInRoute.entrySet()) {
+ if (e.getValue().sum() > 0) {
+ byRoute.computeIfAbsent(e.getKey(), k -> new
LinkedHashMap<>())
+ .computeIfAbsent(kindOf(rule), k -> new
ArrayList<>()).add(rule);
+ }
+ }
+ }
+ List<RouteTried> answer = new ArrayList<>();
+ for (Map.Entry<String, Map<Kind, List<ChaosRule>>> entry :
byRoute.entrySet()) {
+ String route = entry.getKey();
+ List<KindTried> tried = new ArrayList<>();
+ for (Map.Entry<Kind, List<ChaosRule>> k :
entry.getValue().entrySet()) {
+ List<String> ids = new ArrayList<>();
+ List<String> expectations = new ArrayList<>();
+ long fired = 0;
+ long recovered = 0;
+ long handled = 0;
+ long failed = 0;
+ for (ChaosRule rule : k.getValue()) {
+ String id = rule.version > 1 ? rule.id + " v" +
rule.version : rule.id;
+ if (!ids.contains(id)) {
+ ids.add(id);
+ }
+ fired += rule.firedInRoute.get(route).sum();
+ recovered +=
rule.effect.count(ChaosRule.Effect.Outcome.RECOVERED);
Review Comment:
⚠️ **[high] Global counts in a per-route loop** — `recovered`, `handled`,
and `failed` are drawn from `rule.effect.count(...)`, which is the rule's
**global** `Effect` object, not a per-route counter. Only `fired` correctly
uses `rule.firedInRoute.get(route).sum()`. When a rule fires across multiple
routes, each route's `KindTried` entry will show totals accumulated from all
routes, not just this one. The user-facing report becomes misleading for
multi-route scenarios.
```suggestion
recovered +=
rule.effect.countInRoute(ChaosRule.Effect.Outcome.RECOVERED, route);
```
(Requires adding a `countInRoute(Outcome, String route)` method to `Effect`
that queries the per-route adder, mirroring how `firedInRoute` works.)
##########
components/camel-chaos/src/main/java/org/apache/camel/component/chaos/ChaosManager.java:
##########
@@ -574,9 +588,32 @@ public ChaosReport report() {
lostAtDlq = evictedLostAtDlq;
}
Map<String, RouteCounts> counts = new TreeMap<>(routes);
+ long now = TimeSource.currentTimeMillis();
+ List<ChaosRule> all = new ArrayList<>(current);
+ all.addAll(removed);
+ List<ChaosAdvisor.RouteTried> tried = ChaosAdvisor.tried(all,
suggestedKinds(now), now);
return new ChaosReport(
- seed, seedFrom, TimeSource.currentTimeMillis(), current,
removed, counts, unmet, lost, pending,
- lostAtDlq, recentFaults.latestFirst(),
ChaosTargets.reach(camelContext));
+ seed, seedFrom, now, current, removed, counts, unmet, lost,
pending,
+ lostAtDlq, recentFaults.latestFirst(),
ChaosTargets.reach(camelContext), tried);
+ }
+
+ // the kinds of fault the suggestions offer per route, read again at most
every 10 seconds (the report is read
+ // every second for the status of the application), or when the routes
change
+ private static final long SUGGESTED_FOR_MILLIS = 10_000;
+ private volatile Map<String, Set<ChaosAdvisor.Kind>> suggested;
+ private volatile long suggestedAt;
+ private volatile int suggestedRoutes;
+
+ private Map<String, Set<ChaosAdvisor.Kind>> suggestedKinds(long now) {
+ Map<String, Set<ChaosAdvisor.Kind>> answer = suggested;
+ int routeCount = camelContext.getRoutes().size();
+ if (answer == null || now - suggestedAt > SUGGESTED_FOR_MILLIS ||
routeCount != suggestedRoutes) {
+ answer =
ChaosAdvisor.suggestedKinds(ChaosAdvisor.suggest(camelContext));
+ suggested = answer;
+ suggestedAt = now;
+ suggestedRoutes = routeCount;
Review Comment:
⚠️ **[medium] Non-atomic triple volatile write** — `suggested`,
`suggestedAt`, and `suggestedRoutes` are three independent `volatile` fields
updated in sequence. A concurrent reader entering `suggestedKinds()` between
any two of these writes sees an inconsistent snapshot: e.g. a fresh `suggested`
map but a stale `suggestedAt`, causing the stale check `now - suggestedAt >
SUGGESTED_FOR_MILLIS` to pass when it shouldn't (re-computing unnecessarily),
or to fail when it should (using a stale map).
Collapse to a single volatile reference:
```suggestion
suggested = new SuggestedCache(answer, now, routeCount);
```
where `SuggestedCache` is an immutable record `record
SuggestedCache(Map<String,Set<Kind>> kinds, long at, int routes) {}`. A single
volatile write is atomic — no reader can observe a partial update.
##########
dsl/camel-jbang/camel-jbang-plugin-tui/src/main/java/org/apache/camel/dsl/jbang/core/commands/tui/ChaosInfo.java:
##########
@@ -251,6 +253,10 @@ record RouteCounts(String route, long completed, long
failed, long failedKept, l
}
/** A recent fault of a rule. */
+ /** The kinds of fault tried in a route, and those the suggestions offer
that no rule tried yet. */
+ record TriedRoute(String route, List<String> tried, List<String> notTried)
{
Review Comment:
🐛 **[low] Displaced Javadoc leaves `RecentFault` undocumented** — The new
`TriedRoute` block is inserted between the pre-existing `/** A recent fault of
a rule. */` comment and `record RecentFault`. Java attaches each Javadoc
comment to the immediately following declaration; with two Javadoc comments in
a row, only the last one takes effect. The result: `/** A recent fault of a
rule. */` is orphaned (documents nothing), and `record RecentFault` loses its
Javadoc entirely.
Move the `TriedRoute` block **after** `RecentFault`, or repeat the Javadoc
on `RecentFault` after the new block:
```suggestion
/** The kinds of fault tried in a route, and those the suggestions offer
that no rule tried yet. */
record TriedRoute(String route, List<String> tried, List<String>
notTried) {
}
/** A recent fault of a rule. */
```
##########
components/camel-chaos/src/test/java/org/apache/camel/component/chaos/ChaosTriedTest.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * 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.component.chaos;
+
+import org.apache.camel.BindToRegistry;
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.util.json.JsonArray;
+import org.apache.camel.util.json.JsonObject;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * CAMEL-26728: what has been tried per route, how it went, and what the
suggestions offer that no rule tried yet.
+ */
+class ChaosTriedTest extends ChaosTestSupport {
+
+ @BindToRegistry("http")
+ private final TestHttpComponent http = new TestHttpComponent();
+
+ @Override
+ protected RouteBuilder createRouteBuilder() {
+ return new RouteBuilder() {
+ @Override
+ public void configure() {
+ from("direct:orders").routeId("orders")
+
.errorHandler(defaultErrorHandler().maximumRedeliveries(2).redeliveryDelay(0))
+ .to("http://stock/check";)
+ .to("seda:shipping");
+ from("seda:shipping?pollTimeout=10").routeId("shipping")
+ .errorHandler(deadLetterChannel("mock:dlq"))
+ .to("mock:ship");
+ }
+ };
+ }
+
+ private JsonObject route(String routeId) {
+ for (Object o : (JsonArray) chaos().report().toJson().get("tried")) {
+ JsonObject jo = (JsonObject) o;
+ if (routeId.equals(jo.getString("route"))) {
+ return jo;
+ }
+ }
+ throw new AssertionError("No route " + routeId + " in what was tried");
+ }
+
+ @Test
+ void whatWasTriedPerRouteAndWhatIsLeft() {
+ // nothing tried yet: the suggestions say what is left
+ assertThat((JsonArray) route("orders").get("kinds")).isEmpty();
+ assertThat((JsonArray)
route("orders").get("notTried")).contains("errors", "slow", "lost");
+ assertThat((JsonArray)
route("shipping").get("notTried")).contains("dead letter channel");
+
+ String id = chaos().addRule("endpoint=http://stock/check error
status=503 rate=every:2 until=10m");
+ for (int i = 0; i < 2; i++) {
+ template.sendBody("direct:orders", "order " + i);
+ }
+
+ JsonObject orders = route("orders");
+ JsonObject errors = (JsonObject) ((JsonArray)
orders.get("kinds")).get(0);
+ assertThat(errors.getString("kind")).isEqualTo("errors");
+ assertThat((JsonArray) errors.get("rules")).containsExactly(id);
+ assertThat(errors.getLong("fired")).isPositive();
+ // the redelivery recovered them
+ assertThat(errors.getLong("recovered")).isPositive();
+ assertThat(errors.getLong("failed")).isZero();
Review Comment:
⚠️ **[low] Test gap — single-route scenario masks global-vs-per-route bug**
— The rule fires only in the `orders` route, so `rule.effect.count(RECOVERED)`
and `rule.firedInRoute.get("orders").sum()` are identical. The assertions
`assertThat(errors.getLong("recovered")).isPositive()` and
`assertThat(errors.getLong("failed")).isZero()` pass regardless of whether the
values are per-route or global.
Add a second route to the test with the same rule active, send messages
through both routes, and assert that each route's reported `recovered` and
`failed` reflect only *that* route's executions — not the combined total.
##########
components/camel-chaos/src/main/java/org/apache/camel/component/chaos/ChaosRule.java:
##########
@@ -394,16 +397,24 @@ void fired(Hook hook, Exchange exchange, String routeId,
String where) {
if (exchange != null) {
ChaosExchangeState.faulted(this, exchange);
}
+ String route = routeId != null || exchange == null ? routeId :
ChaosExchangeState.routeId(exchange);
+ countInRoute(route);
ChaosRecentFaults recent = recentFaults;
if (recent != null) {
- recent.add(now, this, hook, routeId != null || exchange == null ?
routeId : ChaosExchangeState.routeId(exchange),
- where, exchange != null ? exchange.getExchangeId() : null);
+ recent.add(now, this, hook, route, where, exchange != null ?
exchange.getExchangeId() : null);
+ }
+ }
+
+ private void countInRoute(String route) {
+ if (route != null && (firedInRoute.size() < MAX_ROUTES ||
firedInRoute.containsKey(route))) {
+ firedInRoute.computeIfAbsent(route, k -> new
LongAdder()).increment();
Review Comment:
⚠️ **[low] TOCTOU race on `MAX_ROUTES` guard** — `firedInRoute.size() <
MAX_ROUTES` and `computeIfAbsent` on the `ConcurrentHashMap` are two separate
operations. Two threads with distinct new route IDs can both pass the size
check before either inserts, pushing the map to `MAX_ROUTES + 1` (or more under
contention). Use an atomic guarded insert:
```suggestion
if (route != null) {
firedInRoute.compute(route, (k, v) -> {
if (v != null) { v.increment(); return v; }
if (firedInRoute.size() < MAX_ROUTES) { LongAdder a = new
LongAdder(); a.increment(); return a; }
return null; // route cap exceeded, drop silently
});
}
```
--
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]
