Croway commented on PR #28729:
URL: https://github.com/apache/camel/pull/28729#issuecomment-6576343651
The gauge lookup removed here is only one of several registry scans per
exchange, so this PR alone barely changes the cost. Two more lookups in
`MicrometerExchangeEventNotifier` are where the time goes:
1. `getOrCreateTimer()` calls `find(name).tags(tags).timer()` on every sent
and completed event. Each call copies and filters every registered meter.
`Timer.builder(...).register(registry)` returns the same timer through a map
lookup.
2. `notify()` calls `CamelContext.getRoute(routeId)` on every event to check
for kamelet/template routes. That call copies the route list under
`routesLock`. `addInflightMeter()` already makes the same decision once per
route, so the excluded route ids can be kept in a set.
I ran and verified these performance tests: JMH, Camel 4.22.1 + Micrometer
1.17.2, JDK 21. Routes are `from(direct)` + 3 × `to(log)`, wired like the
camel-micrometer-prometheus defaults. Each cell shows **4.22.1 / + this PR / +
this PR + the change below**. The first and last values come from built jars,
the middle one from an exact copy of this PR's class:
| routes (meters) | µs per exchange, 1 thread | exchanges/s, 4 threads |
|---|---|---|
| 1 (23) | 6.6 / 6.4 / **3.9** | – |
| 10 (131) | 20.6 / 19.3 / **4.3** | 160k / 170k / **551k** |
| 50 (611) | 90.4 / 89.6 / **4.3** | 40k / 37k / **666k** |
| 200 (2,411) | 386 / 359 / **4.9** | 10k / 10k / **599k** |
Without the exchange notifier it is ~2.2 µs and ~1.1M exchanges/s. With the
change, the notifier costs ~2 µs per exchange regardless of route count, and
allocation at 200 routes drops from 360 KB to 8 KB per exchange. The timers,
counts and gauge recorded are identical to 4.22.1.
<details>
<summary>Suggested change (applies on this PR, main and
camel-4.22.1)</summary>
```diff
@@ -18,6 +18,8 @@ package
org.apache.camel.component.micrometer.eventnotifier;
import java.util.HashMap;
import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
@@ -47,6 +49,9 @@ public class MicrometerExchangeEventNotifier extends
AbstractMicrometerEventNoti
private final Map<String, Meter> meterMap = new HashMap<>();
+ // ids of routes not to meter (created by kamelets/templates), decided
once when the route is added so the
+ // exchange events do not need CamelContext.getRoute(id), which copies
the route list on every call
+ private final Set<String> skippedRouteIds =
ConcurrentHashMap.newKeySet();
private Predicate<Exchange> ignoreExchanges = exchange -> false;
@@ -143,6 +148,9 @@ public class MicrometerExchangeEventNotifier extends
AbstractMicrometerEventNoti
private void addInflightMeter(Route route) {
boolean skip = (route.isCreatedByKamelet() && !registerKamelets)
|| (route.isCreatedByRouteTemplate() && !registerTemplates);
+ if (skip) {
+ skippedRouteIds.add(route.getRouteId());
+ }
if (!skip) {
@@ -156,6 +164,7 @@ public class MicrometerExchangeEventNotifier extends
AbstractMicrometerEventNoti
private void removeInflightMeter(String routeId) {
+ skippedRouteIds.remove(routeId);
Meter meter = meterMap.remove(routeId);
@@ -169,6 +178,7 @@ public class MicrometerExchangeEventNotifier extends
AbstractMicrometerEventNoti
meterMap.values().forEach(m -> getMeterRegistry().remove(m));
meterMap.clear();
+ skippedRouteIds.clear();
}
@@ -182,12 +192,8 @@ public class MicrometerExchangeEventNotifier extends
AbstractMicrometerEventNoti
routeId = ExchangeHelper.getAtRouteId(ee.getExchange());
}
- if (routeId != null) {
- Route route =
ee.getExchange().getContext().getRoute(routeId);
- if (route != null) {
- skip = (route.isCreatedByKamelet() && !registerKamelets)
- || (route.isCreatedByRouteTemplate() &&
!registerTemplates);
- }
+ if (routeId != null && skippedRouteIds.contains(routeId)) {
+ skip = true;
}
@@ -231,11 +237,8 @@ public class MicrometerExchangeEventNotifier extends
AbstractMicrometerEventNoti
private Timer getOrCreateTimer(final String name, final Tags tags,
final String description) {
- Timer timer = getMeterRegistry().find(name).tags(tags).timer();
- if (timer == null) {
- timer =
Timer.builder(name).tags(tags).description(description).register(getMeterRegistry());
- }
- return timer;
+ // register() returns the existing timer with a map lookup; find()
would copy and scan every meter
+ return
Timer.builder(name).tags(tags).description(description).register(getMeterRegistry());
}
```
</details>
I also have a test for it, `MicrometerExchangeEventNotifierHotPathTest`:
- **No registry search per exchange.** This check fails on this PR's head
and passes with the change.
- **Kamelet routes not metered,** both at startup and when added at runtime.
Nothing covered the kamelet exclusion before.
camel-micrometer is green both on this PR's head and on 4.22.1 with the
change. I can push it as a commit for you to pick, or open a follow-up PR. The
same per-event `getRoute()` lookup is in camel-opentelemetry-metrics'
`OpenTelemetryExchangeEventNotifier`.
_Drafted with Claude Code._
--
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]
