gnodet-bot commented on code in PR #26754:
URL: https://github.com/apache/camel/pull/26754#discussion_r4075649150
##########
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:
⚠️ **[Re-raise] Dynamic exception messages defeat storm collapse** —
`kindOf()` includes the raw exception message, 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/99999 ← message 2
```
These are the same failure (same route, same node, same exception type) but
the current key treats them as two separate kinds. The storm collapse will not
trigger, both kinds will keep accumulating entries, and the registry will fill
with near-identical errors — exactly the problem this PR is trying to solve.
Consider truncating or normalizing the message before using it as a key:
```suggestion
+ truncateMessage(entry.getExceptionMessage());
```
…and add a `private static String truncateMessage(String msg)` that limits
to, say, 200 characters and strips trailing digits/UUIDs. Or exclude the
message entirely and key only on `(routeId, toNode, exceptionType)` — this is
coarser but immune to dynamic content.
##########
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:
⚠️ **[Re-raise] `maximumKinds` is hardcoded and not exposed** — this bounds
how many distinct error kinds the `repeats` map will track. Unlike
`maximumEntriesPerKind` (which is configurable via
`camel.errorRegistry.maximumEntriesPerKind`), `maximumKinds` is invisible to
users.
If a service produces errors from more than 100 distinct `(route, node,
type, message)` tuples — possible in a large integration with many routes — the
least-recently-seen kind is silently evicted from `repeats`. The next error of
that evicted kind will reset its counter to 1, losing the accumulated count.
Operators have no way to raise this limit.
Either expose it as `camel.errorRegistry.maximumErrorKinds` with the same
plumbing as `maximumEntriesPerKind`, or at minimum document the bound and the
eviction behavior in the Javadoc on `setMaximumEntriesPerKind`.
##########
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:
🔧 **[Re-raise] 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 (JConsole, Hawtio)
displays the description to label the attribute; having the same text on both
signals that the setter's description was copy-pasted rather than written for
its role.
The setter description should describe what setting the value does:
```suggestion
@ManagedAttribute(description = "Sets the maximum number of error
entries of the same kind to keep")
void setMaximumEntriesPerKind(int maximumEntriesPerKind);
```
--
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]
