gnodet-bot commented on code in PR #26754:
URL: https://github.com/apache/camel/pull/26754#discussion_r4075558390
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultErrorRegistry.java:
##########
@@ -206,10 +212,77 @@ private void capture(Exchange exchange, boolean handled) {
}
}
}
+ // count this kind of error and keep only a few of its exchanges, so a
storm of one failure neither hides
+ // the count nor evicts everything else (CAMEL-24911)
+ String kind = kindOf(entry);
+ Repeat repeat = repeats.computeIfAbsent(kind, k -> new
Repeat(timestamp));
+ long count = repeat.record(timestamp);
+ entry.setRepeat(count, repeat.first(), timestamp);
entries.addFirst(entry);
+ evictKind(kind);
evict();
}
+ /** What makes two errors the same kind: the route, the node that failed,
and the exception with its message. */
+ private static String kindOf(BacklogErrorEventMessage entry) {
+ return entry.getRouteId() + "|" + entry.getToNode() + "|" +
entry.getExceptionType() + "|"
+ + entry.getExceptionMessage();
Review Comment:
⚠️ **Dynamic exception messages defeat storm collapse** — `kindOf()`
includes the raw exception message in the key, but many real exceptions embed
dynamic data (request IDs, URLs, counts, timestamps). For example:
```
Connection refused to http://api.example.com/users/12345 <- message 1
Connection refused to http://api.example.com/users/67890 <- message 2
```
These are the same logical error kind but produce two separate keys, so the
storm never gets compressed — the registry still fills up with hundreds of
entries for what is conceptually one failing endpoint.
This is the highest-probability production scenario for a storm (a
downstream service going down, a queue being full, a rate-limit being hit). The
fix should either truncate the message to a max length and/or strip numeric
substrings:
```suggestion
private static String kindOf(BacklogErrorEventMessage entry) {
String msg = entry.getExceptionMessage();
// Normalize dynamic content: truncate long messages and collapse
digit-runs so that
// "Connection refused to /users/123" and "…/users/456" hash to the
same kind.
if (msg != null && msg.length() > 200) {
msg = msg.substring(0, 200);
}
if (msg != null) {
msg = msg.replaceAll("\\d+", "#");
}
return entry.getRouteId() + "|" + entry.getToNode() + "|" +
entry.getExceptionType() + "|"
+ msg;
}
```
Without this, CAMEL-24911 is only partially fixed: storms with varying
messages (the common case for network errors) still evict everything else.
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultErrorRegistry.java:
##########
@@ -47,9 +48,14 @@
public class DefaultErrorRegistry extends EventNotifierSupport implements
ErrorRegistry {
private final ConcurrentLinkedDeque<BacklogErrorEventMessage> entries =
new ConcurrentLinkedDeque<>();
+ /** How often each kind of error happened, so a storm is counted while
only a few of its exchanges are kept. */
+ private final Map<String, Repeat> repeats = new ConcurrentHashMap<>();
private final AtomicLong uidCounter = new AtomicLong();
private volatile boolean enabled;
private volatile int maximumEntries = 100;
+ /** How many exchanges of the same kind of error are kept, so one storm
does not push out the other errors. */
+ private volatile int maximumEntriesPerKind = 3;
+ private volatile int maximumKinds = 100;
Review Comment:
⚠️ **`maximumKinds` is hardcoded and not exposed** — this field controls a
key bound in the storm-collapse feature, but unlike `maximumEntriesPerKind`
(which IS configurable), `maximumKinds` is invisible to users.
If a service produces errors from more than 100 distinct `(route, node,
type, message)` combinations (easily possible in a large Camel application),
the oldest `Repeat` entries get silently evicted. Once evicted, a repeat of
that kind restarts its counter from 1, making the repeat count misleading — it
no longer reflects the true cumulative count.
Add it to `ErrorRegistry`, `ErrorRegistryConfigurationProperties`, and the
catalog metadata alongside `maximumEntriesPerKind`. Default of 100 is
reasonable, but it must be tunable.
##########
core/camel-management-api/src/main/java/org/apache/camel/api/management/mbean/ManagedErrorRegistryMBean.java:
##########
@@ -38,6 +38,12 @@ public interface ManagedErrorRegistryMBean extends
ManagedServiceMBean {
@ManagedAttribute(description = "Maximum number of error entries to keep")
void setMaximumEntries(int maximumEntries);
+ @ManagedAttribute(description = "Maximum number of error entries of the
same kind to keep")
+ int getMaximumEntriesPerKind();
+
+ @ManagedAttribute(description = "Maximum number of error entries of the
same kind to keep")
Review Comment:
🔧 **Duplicate `@ManagedAttribute` description on getter and setter** — both
annotations carry the identical string `"Maximum number of error entries of the
same kind to keep"`. JMX tooling uses these descriptions to label the attribute
in consoles (JConsole, Hawtio, etc.); having the same text on both can cause
confusion.
The pattern used by the existing `getMaximumEntries`/`setMaximumEntries`
pair in this interface is also identical text (pre-existing), but it's still
worth fixing here. Setters conventionally carry `"Sets the …"` or are left with
the same description — either is acceptable, but at minimum the setter should
be consistent with the rest of the file:
```suggestion
@ManagedAttribute(description = "Maximum number of error entries of the
same kind to keep")
void setMaximumEntriesPerKind(int maximumEntriesPerKind);
```
(This is actually already consistent with the pre-existing pattern in this
interface, so this is just a documentation note — not a blocker.)
--
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]
