CAMEL-24003: Add activity queue to BacklogTracer with EventNotifier#24598
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Review: CAMEL-24003 - Add activity queue to BacklogTracer
The PR is well-structured, follows existing patterns, and provides a useful lightweight monitoring capability. I found a few issues worth discussing before merge:
1. [Medium] Text vs JSON asymmetry in ActivityDevConsole
In ActivityDevConsole.java, doCallText() uses tracer.getActivity() (non-destructive — returns unmodifiable view), while doCallJson() uses tracer.dumpActivity() (destructive when removeOnDump=true, which is the default).
This means:
- Calling the text endpoint shows the activity without clearing it
- Calling the JSON endpoint clears the activity queue
Every other dev console in the codebase is symmetric (both text/JSON use the same method). For a monitoring/rolling-window use case (especially with a polling TUI), the destructive JSON behavior seems wrong — each poll would clear the queue. Recommendation: both should use getActivity() (non-destructive), since the activity queue is a rolling window meant for live monitoring.
2. [Medium] JSON field inconsistency between JMX and DevConsole
The two JSON representations of the same data include different fields:
| Field | JMX dumpActivityAsJSon |
DevConsole doCallJson |
|---|---|---|
uid |
Missing | Included |
remoteEndpoint |
Missing | Included |
fromRouteId |
Always included | Conditionally included (if non-null) |
These should be consistent.
3. [Low] Activity capture requires messageHistory in standby mode
The activity capture code in traceEvent() is placed after the early return guard that exits when standby=true, enabled=false, and messageHistory=false (the default). This means the activity queue only captures when:
enabled=true(full tracing on), ORstandby=trueANDmessageHistory=true
If the intent is for the activity queue to work in pure standby mode without requiring message history, the activity capture block should be moved above the early return guard. If this is intentional (e.g., the TUI always enables message history with standby), it should be documented.
4. [Low] Activity queue stores full event objects
The PR description emphasizes "lightweight metadata (no body or headers)." However, the activity queue stores full BacklogTracerEventMessage objects that hold references to the full serialized data (body, headers, properties). While the output methods only extract metadata fields, the underlying objects prevent GC of the full data. For 100 entries this is probably fine, but consider creating a lightweight "activity summary" class if memory efficiency matters.
5. [Low] Missing upgrade guide entry
Per project conventions, new user-visible configuration properties (camel.trace.activitySize) and dev console (activity) should have an entry in docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_22.adoc.
6. [Info] API additions properly have @since 4.22 tags ✅
7. [Info] Concurrency patterns are consistent with existing code ✅
Overall: Well-designed feature. The text/JSON asymmetry (#1) and JSON field inconsistency (#2) seem like likely bugs worth fixing before merge.
|
Claude Code on behalf of @davsclaus Thanks for the thorough review @gnodet! Addressed items 1-3 in b22efd1: 1. Text vs JSON asymmetry — Fixed. 2. JSON field inconsistency — Fixed. 3. Activity capture in standby mode — Fixed. Moved the activity capture block above the early return guard so it only requires 4. Full event objects — Acknowledged. For 100 entries this is fine; a lightweight summary class can be a follow-up if profiling shows it matters. 5. Upgrade guide — Not needed since this is a new feature, not a behavioral change to existing functionality. |
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Thanks for addressing the review feedback! The changes look good:
- ✅ Activity capture moved before the early return guard — now works in standby mode without requiring
messageHistory - ✅ JSON field set aligned between
dumpActivityAsJSon()andActivityDevConsole— both now includeuid, conditionalfromRouteId, andremoteEndpoint - ✅ DevConsole JSON switched from
dumpActivity()togetActivity()— consistent non-destructive reads
LGTM.
|
Actually for 4 we need the full objects in the CLI/TUI and where else - and this is also a developer feature - not enabled in production |
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 553 tested, 28 compile-only — current: 552 all testedMaveniverse Scalpel detected 581 affected modules (current approach: 552).
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
b22efd1 to
1398b7a
Compare
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Re-review after rebase + "Address review feedback" commit. Still LGTM.
The feedback commit makes two good changes:
-
Activity capture moved before the early return —
traceEvent()now captures activity events when(enabled || standby) && event.isLast()before theif (!enabled) { return; }guard. This ensures activity monitoring works in standby mode without requiringmessageHistoryto be enabled. Good fix — without this, the activity feature would only work when the tracer is fully enabled. -
dumpActivity()→getActivity()rename inActivityDevConsole— consistent naming.
No new concerns.
Use EventNotifier to capture exchange lifecycle events (created, sent, completed, failed) instead of traceEvent(). This captures which remote endpoints each exchange called along the way via ExchangeSentEvent. Introduces BacklogTracerActivityMessage as a lightweight API interface separate from BacklogTracerEventMessage, with EndpointSend inner interface exposing endpointUri, remoteEndpoint flag, and elapsed time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
gnodet
left a comment
There was a problem hiding this comment.
Claude Code review on behalf of @gnodet
Re-review: New commit e19cf1d6 — Rework activity queue to use EventNotifier
This is a well-designed architectural improvement. The activity queue now uses EventNotifier lifecycle events instead of piggy-backing on traceEvent(), which is cleaner and captures significantly richer data.
What improved:
- Decoupled architecture: Activity tracking no longer depends on the message tracing pipeline. The
ActivityEventNotifierlistens forExchangeCreated/Sent/Completed/Failedevents independently. - Purpose-built data model: New
BacklogTracerActivityMessageinterface replaces reuse ofBacklogTracerEventMessage. Activity entries now include a list ofEndpointSendrecords (remote endpoint URI + timing), which wasn't possible with the old approach. - Standby mode:
isDisabled()returns!enabled && !standby, so activity still works in standby mode. ✓
Thread safety looks correct:
inflightActivityusesConcurrentHashMap— concurrent exchange creation/completion is safeendpointSendsinDefaultBacklogTracerActivityMessageissynchronizedfor thread-safe accumulation during the exchange lifecycle, andgetEndpointSends()returns a defensive copycomplete()fields are set beforeactivityQueue.offer(), and theConcurrentHashMap.remove()→Queue.offer()chain provides happens-before ordering for readers
Other good details:
evictStaleInflight()with 5-minute cutoff prevents memory leaks from orphaned exchangesNonManagedServicekeeps the notifier out of JMXDefaultChannel.getOrCreateBacklogTracer()now callsaddService(tracer)to ensure lifecycle callbacks fire@since 4.22tags on the new SPI interface ✓- API change (
getActivity()/dumpActivity()return type) is fine since the activity API was introduced in the same 4.22 cycle
One minor observation: The JSON serialization logic in BacklogTracer.dumpActivityAsJSon() and ActivityDevConsole.doCallJson() is nearly identical. Not introduced by this commit (the duplication existed before), but could be extracted into a shared utility method in a follow-up.
LGTM — clean rework with good test updates.
Summary
Adds an activity queue to BacklogTracer that captures a rolling window of recent exchange lifecycle events for live monitoring dashboards (TUI Activity tab, CLI camel cmd activity).
Key design decisions
Files changed
Claude Code on behalf of davsclaus