davsclaus commented on code in PR #26754:
URL: https://github.com/apache/camel/pull/26754#discussion_r4075610763
##########
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:
Good catch, and it matters for the case this came from: the storm I traced
carried the failing sku in its message, so with the message in the key every
exchange would have been its own kind and the registry would have flooded
exactly as before.
Fixed by making the kind coarser: **route | node | exception type**, with
the message left out. The messages are still there to read on the entries that
are kept, and a new test (`testAStormWhoseMessagesDifferIsStillOneKind`) sends
20 failures whose messages all differ and asserts 3 entries, a count of 20, and
that an unrelated error survives.
I chose this over masking dynamic parts of the message (digits, ids, urls),
because that is guesswork that would merge failures that are genuinely
different - a 404 and a 500 from the same endpoint, say.
##########
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:
Fixed, by removing the constant: the counters are now bounded by
`maximumEntries` instead of a hardcoded 100, so raising the registry size
raises the number of counters with it. With the coarser kind (route, node,
exception type) the map is naturally small anyway - it is bounded by the nodes
that can fail rather than by the payloads that made them fail.
##########
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:
Checked the neighbours before changing this, and the convention here is the
opposite of what the comment describes:
`getMaximumEntries`/`setMaximumEntries`,
`getTimeToLiveSeconds`/`setTimeToLiveSeconds` and `isEnabled`/`setEnabled` in
this same mbean all repeat the identical description on getter and setter. JMX
shows one attribute with one description regardless, so I am keeping the pair
consistent with the file rather than introducing a different style for one
attribute.
--
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]
