Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions core/src/main/java/com/linecorp/armeria/common/CommonPools.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,16 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.google.common.annotations.VisibleForTesting;

import com.linecorp.armeria.client.ClientFactoryBuilder;
import com.linecorp.armeria.common.metric.MeterIdPrefix;
import com.linecorp.armeria.common.metric.MoreMeterBinders;
import com.linecorp.armeria.common.util.BlockingTaskExecutor;
import com.linecorp.armeria.common.util.EventLoopGroups;
import com.linecorp.armeria.server.ServerBuilder;

import io.micrometer.core.instrument.binder.netty4.NettyEventExecutorMetrics;
import io.netty.channel.EventLoopGroup;

/**
Expand All @@ -45,11 +48,7 @@ public final class CommonPools {
EventLoopGroups.newEventLoopGroup(Flags.numCommonWorkers(), "armeria-common-worker", true);

static {
// Bind EventLoopMetrics for the common worker group.
MoreMeterBinders
.eventLoopMetrics(WORKER_GROUP, new MeterIdPrefix("armeria.netty.common"))
.bindTo(Flags.meterRegistry());

bindEventLoopMetricsForWorkerGroup(WORKER_GROUP);
try {
final Class<?> aClass = Class.forName("reactor.core.scheduler.Schedulers",
true, CommonPools.class.getClassLoader());
Expand All @@ -65,6 +64,18 @@ public final class CommonPools {
}
}

@VisibleForTesting
static void bindEventLoopMetricsForWorkerGroup(EventLoopGroup evGroup) {
// Bind EventLoopMetrics for the common worker group.
MoreMeterBinders
.eventLoopMetrics(evGroup, new MeterIdPrefix("armeria.netty.common"))
.bindTo(Flags.meterRegistry());
// Bind the common event loop metrics emitted automatically by micrometer.
// These metrics intentionally duplicate those emitted by EventLoopMetrics to preserve
// backward compatibility.
new NettyEventExecutorMetrics(evGroup).bindTo(Flags.meterRegistry());
}

/**
* Returns the default common blocking task {@link BlockingTaskExecutor} which is used for
* potentially long-running tasks which may block I/O threads.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ void remove(EventLoopGroup eventLoopGroup) {
return result;
}

@Deprecated // use the same metric with micrometer namespace instead
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add @Deprecated to the class. 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

Woud you also deprecate MoreMeterBinders.eventLoopMetrics() that takes name and meterIdPrefix?

Copy link
Author

Choose a reason for hiding this comment

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

I did not deprecate the whole class because it emits a metric (num event loop workers) which is not available in Micrometer right now. We will continue using it until we are able to add that metric in Micrometer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not deprecate the whole class because it emits a metric (num event loop workers) which is not available in Micrometer right now

What do you think of first adding the metric to Micrometer first before proceeding with this PR?
I think the direction of this PR will be clearer if we can consider NettyEventExecutorMetrics as a replacement of MoreMeterBinders.eventLoopMetrics.

Also, it would be nice if the meter name prefix can also be specified like ExecutorServiceMetrics does - especially given that thread names are not necessarily unique. It can also allow users to group armeria related metrics easily depending on the prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of first adding the metric to Micrometer first before proceeding with this PR?
I think the direction of this PR will be clearer if we can consider NettyEventExecutorMetrics as a replacement of MoreMeterBinders.eventLoopMetrics.

I agree with you. Initially, I thought users could simply count the number of event loops using a count query with the name tag,
but since there's some additional calculation involved, we probably do need to add the metric as well.

Also, it would be nice if the meter name prefix can also be specified like ExecutorServiceMetrics does - especially given that thread names are not necessarily unique. It can also allow users to group armeria related metrics easily depending on the prefix.

I thought the current approach was fine because I personally recommend using only a single event loop group.
However, there are cases where users can't unify event loop groups — for example, when using Armeria with Lettuce.
So, let's ask the Micrometer team if it's possible to customize the meter name

Copy link
Author

Choose a reason for hiding this comment

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

ok, I will get that done.

double pendingTasks() {
int result = 0;
for (EventLoopGroup group : registry) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright 2025 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.common;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import java.util.Map;

import org.junit.jupiter.api.Test;

import com.linecorp.armeria.common.metric.MoreMeters;
import com.linecorp.armeria.common.util.EventLoopGroups;

import io.micrometer.core.instrument.binder.netty4.NettyMeters;
import io.netty.channel.EventLoopGroup;

public class CommonPoolsTest {
@Test
public void testEventLoopMetricsBinding() throws Exception {
final EventLoopGroup testGroup = EventLoopGroups.newEventLoopGroup(1);

try {
CommonPools.bindEventLoopMetricsForWorkerGroup(testGroup);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that 1) Flags.meterRegistry is a static variable and 2) the jvm is forked for every gradle module I can imagine this test affecting other tests within the core module.

What do you think of adding a second parameter MeterRegistry for testing purposes? (i.e. using new SimpleMeterRegistry())


// verify that registry contains the newly added metrics
final Map<String, Double> registeredMeters = MoreMeters.measureAll(Flags.meterRegistry());
assertThat(registeredMeters.keySet()
.stream()
.anyMatch(key -> key.startsWith(
NettyMeters.EVENT_EXECUTOR_TASKS_PENDING.getName())))
.isTrue();
} finally {
testGroup.shutdownNow();
}
}
}