Skip to content

CAMEL-24032: Add rolling 1-minute exchange rate metric#24658

Closed
gbhavya07 wants to merge 1 commit into
apache:mainfrom
gbhavya07:CAMEL-24032-rolling-exchange-rate
Closed

CAMEL-24032: Add rolling 1-minute exchange rate metric#24658
gbhavya07 wants to merge 1 commit into
apache:mainfrom
gbhavya07:CAMEL-24032-rolling-exchange-rate

Conversation

@gbhavya07

Copy link
Copy Markdown
Contributor

Description

This PR implements CAMEL-24032 by adding a rolling 1-minute exchange rate metric to ManagedPerformanceCounter.

The new metric tracks the number of exchanges completed or failed during the last 60 seconds and is available when the Management Statistics Level is set to Extended. When extended statistics are not enabled, the metric returns -1, consistent with the existing percentile metrics.

The metric is exposed through:

  • JMX (ExchangeRate1m)
  • XML statistics output
  • JSON statistics output
  • Camel Dev Console

A unit test has also been added to verify the new metric is exposed correctly through JMX for both route and context statistics.

Target

  • I checked that the commit is targeting the correct branch (Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change: CAMEL-24032.

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.

  • I have run mvn clean install -DskipTests locally from root folder and I have committed all auto-generated changes.
    I attempted to run ./mvnw clean install -DskipTests from the repository root, but the build failed in the unrelated camel-kserve module. The changes in camel-management and camel-console build successfully, and the relevant tests pass.

AI-assisted contributions

  • If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., Co-authored-by trailers) and the PR description identifies the AI tool used.

@oscerd oscerd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution — this is solid work that unfortunately hit an unlucky race rather than any quality problem, so please read the following in that spirit.

The headline issue: CAMEL-24032 was already implemented and merged the same morning. PR #24645 (merged 2026-07-13 08:25 UTC as 62b024b) resolved this JIRA with an EWMA-smoothed getThroughput() metric, and the JIRA is now Resolved/Fixed (fixVersion 4.22.0). Your branch was cut from a commit that predates that merge, so you almost certainly never saw it — same-day collision. As a result this PR is currently in conflicting state: both changes insert at the same point in ManagedPerformanceCounterMBean (right after getProcessingTimeP99()) and touch the same methods in ManagedPerformanceCounter and the dev consoles.

There is a real question for the maintainers here, though. The merged metric is a smoothed exchanges/second (timer-driven EWMA, available at all statistics levels); this PR is an exact trailing-60-second count (Extended-only) — arguably closer to the literal JIRA title, and the two are complementary rather than redundant. @davsclaus — is an exact 1-minute-window rate wanted alongside the EWMA throughput? If yes, the path forward would be a new JIRA, a rebase onto current main, and the notes below; if not, this PR should be closed as superseded — with no reflection on the quality of the attempt.

If the work continues under a new JIRA, the main technical feedback:

  1. Hot-path locking (ManagedPerformanceCounter.updateExchangeRate): every exchange completion acquires exchangeRateLock, and Extended stats are enabled at processor, route, route-group and context level — so one completion takes 3+ monitors and all concurrent consumers of a route contend on each counter's lock. The existing percentile ring buffer (CAMEL-23976) deliberately stays lock-free on the write path, and the merged CAMEL-24032 approach avoids per-exchange cost entirely. An AtomicLongArray with CAS on second-rollover (or LongAdder per bucket) would keep the exact-window semantics without a mutex on the completion path.
  2. reset() bypasses the lock the PR itself introduces: the two Arrays.fill(...) calls can interleave with a concurrent updateExchangeRate, leaving a bucket with a zeroed timestamp but stale count. Since the lock exists, reset should take it too (or the whole structure goes atomic per note 1).
  3. Test robustness: the exact-count assertions (assertEquals(5, rate)) on a wall-clock 60s window can fail if a loaded CI host stalls between the sends and the JMX read — the recent flaky-test sweeps have been fighting exactly this class of failure. A range assertion (rate >= 1 && rate <= 5) keeps the signal while being stall-proof. Nice that the test avoids Thread.sleep entirely and doesn't wait a minute, though.
  4. Docs: a user-visible new JMX attribute plus new XML/JSON stats keys deserves a line in the 4.22 upgrade guide / management docs.

Things done right that I want to call out explicitly: @since 4.22 Javadoc on the new public MBean method, the -1 sentinel consistent with the percentile precedent, a fixed-memory ring buffer (2×60 longs) tagged with absolute seconds that handles idle gaps with zero background threads, uniform wiring across JMX/XML/JSON and all five dev consoles, a clean single commit following the CAMEL-XXXX: convention, and no new dependencies. This is exactly the shape a metrics contribution should have — it just needs its own JIRA and a rebase to have a home.

(Note: no CI ran on this branch — likely the first-time-contributor workflow-approval gate combined with the conflict state.)

Reviewed with Claude Code (Fable 5) on behalf of oscerd. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code review on behalf of @gnodet

Thank you for working on this feature, @gbhavya07. The implementation shows a clean circular-buffer approach for tracking exchange counts. However, there are several issues that need to be addressed before this can be merged.

1. Merge conflict with PR #24645 (Critical)

PR #24645 by @davsclaus was merged earlier today under the same JIRA issue (CAMEL-24032). That PR added getThroughput() (EWMA-smoothed exchanges/second) into ManagedPerformanceCounterMBean and ManagedPerformanceCounter. Your branch is now in a CONFLICTING state with main.

You will need to:

  • Rebase your branch onto current main
  • Resolve the conflicts in ManagedPerformanceCounterMBean.java and ManagedPerformanceCounter.java
  • Verify that both metrics (getThroughput() and getExchangeRate1m()) coexist correctly
  • Consider whether both metrics are needed (see point 4 below)

2. Thread-safety inconsistency (High)

Your implementation uses synchronized(exchangeRateLock) in updateExchangeRate() and getExchangeRate1m(). However, the existing percentile window code in the same class is intentionally lock-free (no synchronization on percentileWindow, percentileIndex, or percentileCount), and the LoadThroughput class (added by PR #24645) is also lock-free.

The ManagedPerformanceCounter is designed for high-throughput hot paths (completedExchange and failedExchange are called for every exchange). Adding a synchronized block in these methods introduces contention that the rest of the class deliberately avoids. Consider adopting a lock-free circular buffer (similar to the percentile window) or justifying why synchronization is needed here when it is not needed for the percentile window.

3. Naming: "Rate" vs "Count" (Medium)

The method getExchangeRate1m() returns the count of exchanges in the last 60 seconds, not a rate (exchanges per unit time). The JMX description says "Rolling 1-minute exchange rate [exchanges/minute]" which is technically correct if we interpret it as "count per minute", but calling it a "rate" when it returns a raw count is confusing alongside getThroughput() which returns actual exchanges/second. Consider renaming to getExchangeCount1m() or dividing by 60 to return an actual per-second rate.

4. Overlap with existing throughput metric (Medium)

After PR #24645, ManagedPerformanceCounter already provides getThroughput() which returns an EWMA-smoothed exchanges/second rate with a 1-minute decay window. Your getExchangeRate1m() provides a simple count over the last 60 seconds. These are conceptually similar. It would help to discuss with @davsclaus on the JIRA issue (CAMEL-24032) whether both metrics add distinct value and are worth the additional memory and computation cost.

5. Test assertions may be timing-sensitive (Low)

The test assertions assertEquals(5, rate.longValue()) and assertEquals(3, rate.longValue()) assume all exchanges complete within the 60-second window, which is nearly guaranteed. However, the bucketing by currentSecond % 60 means there is a theoretical (though unlikely) edge case at second boundaries. Consider adding a brief comment documenting this assumption.

Good:

  • Correctly includes @since 4.22 tag on the new method — well done following project conventions.
  • The circular buffer approach is conceptually sound.

Scanner coverage: No standalone static analysis tools (PMD, Checkstyle, semgrep, SpotBugs) available. Manual review against project rules and conventions. CI pipeline runs formatter/impsort/rewrite checks.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

// rolling 1-minute exchange rate (Extended statistics only)
private static final int EXCHANGE_RATE_WINDOW_SECONDS = 60;
private final Object exchangeRateLock = new Object();
private long[] exchangeRateSeconds;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The synchronized approach here is inconsistent with the lock-free pattern used by the rest of this class (percentile window, LoadThroughput). Since updateExchangeRate() is called from completedExchange() and failedExchange() — the hottest paths in the counter — adding contention here may not be desired.

The existing percentile window uses a simple ring buffer without synchronization. Consider adopting the same approach, or at minimum documenting why synchronization is needed here but not for the percentile window.

/**
* @since 4.22
*/
@ManagedAttribute(description = "Rolling 1-minute exchange rate [exchanges/minute]. Requires Extended statistics level, returns -1 otherwise.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description says "exchange rate [exchanges/minute]" but the implementation returns a raw count, not a rate. After PR #24645 merged getThroughput() (EWMA exchanges/second) into this same interface, having both metrics with similar-sounding names but different semantics could be confusing.

Consider:

  • Renaming to getExchangeCount1m() to clarify it's a count, not a rate
  • Or dividing by 60 in the implementation to return an actual per-second rate

assertEquals(5, rate.longValue());
}

@Test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion assumes all 5 exchanges complete within the 60-second window, which is nearly guaranteed. However, the bucketing by currentSecond % 60 means there is a theoretical edge case at second boundaries. Consider adding a brief comment documenting this assumption.

@davsclaus

Copy link
Copy Markdown
Contributor

This has been implemented and the rate is per seconds in the core, and you can * 60 to get it per minute.\

Closing this PR

@davsclaus davsclaus closed this Jul 13, 2026
@gbhavya07 gbhavya07 deleted the CAMEL-24032-rolling-exchange-rate branch July 13, 2026 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants