Give high priority to the innermost MetricCollectingService

[ikhoon]

Motivation:

If duplicate MetricCollectingServices are used to decorate a service, the first MetricCollectingService encountered will be selected to collect metrics.

armeria/core/src/main/java/com/linecorp/armeria/internal/common/metric/RequestMetricSupport.java

Lines 57 to 59 in 0df139a

	if (Boolean.TRUE.equals(isRequestMetricsSet)) {
	return;
	}

The downside to this strategy is that users can't override a globally used MetricCollectingService with a local decorator that is even directly applied to a service.

If we give high priority to the MetricCollectingService closest to a service, users will be able to easily override the global one.

Modifications:

• Fixed MetricCollectingService to look for an inner MetricCollectingService before setting up request metrics.
  • If an inner MetricCollectingService is found, delegate the decorator to collect metrics.
• Moved RouteDecoratingService to internal package.
• Added RouteDecoratingService.as(ctx, serviceClass) to find a matching decorator from the internal service chain.

Result:

• The innermost MetricCollectingService is now chosen to measure metrics for a service.
  • Previously, the outermost one is used to collect metrics.

[trustin]

Can we avoid calling .as() and other potentially expensive operations only once, so we don't have to call it on every request?

[ikhoon]

RouteDecoratingServices for a request are dynamically resolved depending on RoutingContext.

armeria/core/src/main/java/com/linecorp/armeria/internal/server/RouteDecoratingService.java

Lines 146 to 150 in 379e29f

	router.findAll(ctx.routingContext()).forEach(routed -> {
	if (routed.isPresent()) {
	serviceChain.add(routed.value().decorator());
	}
	});

I'm not sure there is a simple way to cache the result. We may cache a set of Routes that a MetricCollingService should handle.

private Set<Route> routeCache;
private Set<Route> negativeCache;

private boolean shouldRecordMetrics(ServiceRequestContext ctx) {
    Route route = ctx.routingContext().result().route();

    if (routeCache.contains(route)) {
        return true;
    }
    if (negativeCache.contains(route) {
        return false;
    }

    // Look for an inner MetricCollectingService and update the caches 
    // ...

[minwoox]

Looks nice!

[minwoox]

I'm a bit worried that this might affect the performance in the worst case but this shouldn't be a problem in most cases.
We might add a cache later if it becomes a problem.

[ikhoon]

Most applications, even complex business logic, may use less than 20~30 decorators.
I'm not sure how efficient the cache strategy is for them.

Do you want some numbers for the performance, I will try to run some benchmarks.

[ikhoon]

I added a cache layer to check before looking for decorators with .as(). The size of Route is fixed when a server is built and it never changes until it is reconfigured.
All requests will hit the cache. Cache operations are not free though.

[minwoox]

Yeah, I thought it was not a problem in most cases so I didn't ask you the numbers because I didn't want to bother you. 😅
Anyway, because you've already done some testing, could you share the numbers?

[minwoox]

it never changes until it is reconfigured.

How about clearing the map in the serviceAdded method?

[ikhoon]

It was added already at L138. 😉

[minwoox]

because you've already done some testing, could you share the numbers?

Oops, I misread your comments. 😓 Please ignore this. Thanks!

[jrhee17]

Looks good to me in terms of implementation correctness 👍 Thanks for the quick fix @ikhoon 🙇 👍 🚀

[trustin]

Nice work!