Add barebones Netty event-loop metrics #4750

vkostyukov · 2023-03-11T04:20:11Z

Motivation:

We want to export some very basic metrics about the underlying Netty runtime. Let's start with the number of event-loops and the number of IO tasks waiting to be executed.

Modifications:

Added EventLoopMetrics, which implements MeterBinder.
The approach to count pending-io-tasks is highly inspired by this gauge from Finagle. It's also pretty similar how we do it internally at Databricks.
Registered CommonPools event-loop-groups for metrics collection.

Result:

Closes Add metrics about EventLoop #4675

trustin

👋 Thanks for your first contribution, @vkostyukov!

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

core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java

core/src/main/java/com/linecorp/armeria/server/Server.java

core/src/test/java/com/linecorp/armeria/internal/common/metric/NettyMetricsSupportTest.java

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

core/src/test/java/com/linecorp/armeria/internal/common/metric/NettyMetricsSupportTest.java

core/src/main/java/com/linecorp/armeria/server/Server.java

vkostyukov · 2023-03-21T16:06:52Z

@ikhoon mind taking another look when you get a chance? Ty!

vkostyukov · 2023-03-22T05:27:26Z

@ikhoon linter should be happy now. Not super sure about other failures - they seemed unrelated. I'm happy to dig more if you think otherwise.

ikhoon

I believe we are on the same page. Left comments on code style and visibility.

core/src/main/java/com/linecorp/armeria/common/metric/MoreMetrics.java

core/src/main/java/com/linecorp/armeria/common/metric/NettyMetrics.java

core/src/test/java/com/linecorp/armeria/common/metric/NettyMetricsTest.java

trustin

Thanks a lot for the solid work, @vkostyukov. Left some minor comments. 🙇

core/src/main/java/com/linecorp/armeria/common/CommonPools.java

core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java

trustin · 2023-03-23T11:59:28Z

core/src/main/java/com/linecorp/armeria/common/metric/MoreMetrics.java

+     *  - "event.loops.pending.tasks" (gauge) - the total number of IO tasks waiting to be run on event loops*
+     * </p>
+     */
+    public static MeterBinder eventLoopMetrics(MeterIdPrefix idPrefix, EventLoopGroup eventLoopGroup) {


What do you think about merging this class into MoreMeters?

This would be the first method there returning MeterBinder (or metrics in that sense).

MoreMeters seems to mostly contain utilities to work with MeterRegistry, which is also suggested by its javadoc:

// Provides utilities for accessing MeterRegistry.

I'd probably vote for keeping MoreMetrics as a factory for creating MeterBinders.

IMHO, it's better to merge those classes because it can be a bit distracting for users to find Micrometer relates utilities. We can fix the Javadoc of MoreMeters if that's the problem. 😄
Anyway, we still need to add @UnstableApi because this is a newly-added public method. 😉

Added @UnstableApi annotation for eventLoopMetrics.

ikhoon

Thanks, @vkostyukov! ❤️
LGTM if @trustin comments are addressed.

vkostyukov · 2023-03-23T16:26:20Z

@trustin Just updated. PTAL once you have a chance.

minwoox

Left only style suggestions. Thanks! 😄

core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java

core/src/main/java/com/linecorp/armeria/common/metric/MoreMetrics.java

core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java

core/src/test/java/com/linecorp/armeria/common/metric/EventLoopMetricsTest.java

vkostyukov · 2023-03-24T03:29:43Z

Left only style suggestions. Thanks! 😄

Thanks for looking. Just pushed an update. PTAL again

minwoox

Great! Thanks a lot for adding these nice metrics! 😄

jrhee17

Left a minor #4750 (comment) but looks good to me! Thanks a lot @vkostyukov ! 🙇 👍 🚀

core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java

jrhee17 · 2023-03-24T16:32:34Z

core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java

+            parent.gauge(pendingTasks, idPrefix.tags(), this, Self::pendingTasks);
+        }
+
+        // This runs every 3 seconds at most.


Note: I wondered for a second whether it's worth caching these stats since:

The computing logic doesn't seem too expensive and most likely will run on a separate thread

The collector implementation has more transparency on when metrics are computed

The current implementation doesn't guarantee stats are cached for concurrent requests anyways

Having said this, I think the current implementation is fine 👍

I think I carried over this pattern from the Caffeine metrics, which are admittedly more expensive to collect. I'm happy to drop the caching if that also works for @ikhoon and @minwoox. I don't want to do it before we have a consensus.

That's a good point. @jrhee17 👍
I thought caching is okay because this type of information does not need to be immediately reflected in actual values.
Of course, we can revisit this later if we need to fix it. 😄

I'd down to simplify it. I think pendingTask is pretty cheap to run. Let me make the change.

Updated! PTAL

Still LGTM. Thanks!

jrhee17

Thanks @vkostyukov ! Looks really nice 🚀 👍 🙇

trustin

Thanks a lot, @vkostyukov!

trustin · 2023-03-28T06:56:18Z

Let me make the following changes and merge:

Rename MoreMetrics to MoreMeterBinders to make it clear that it provides MeterBinders.
Reorder the parameters of .eventLoopMetrics() and EventLoopMetrics() so that name comes after EventLoopGroup, so it's consistent with the official binder implementations such as ExecutorServiceMetrics.
Fix a Checkstyle violation.

minwoox · 2023-03-29T07:02:05Z

core/src/main/java/com/linecorp/armeria/common/metric/EventLoopMetrics.java

+ * exported per registered {@link MeterIdPrefix}.
+ *
+ * <ul>
+ *   <li>"event.loop.num.workers" (gauge) - the total number of Netty's event loops</li>


Note: Let me remove num in the release note.

vkostyukov marked this pull request as ready for review March 11, 2023 04:23

vkostyukov requested review from ikhoon, jrhee17, minwoox and trustin as code owners March 11, 2023 04:23

vkostyukov mentioned this pull request Mar 11, 2023

Add metrics about EventLoop #4675

Closed

trustin reviewed Mar 13, 2023

View reviewed changes

ikhoon added the new feature label Mar 13, 2023

ikhoon reviewed Mar 13, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/internal/common/metric/NettyMetricSupport.java Outdated Show resolved Hide resolved

vkostyukov force-pushed the vk/event-loop-metrics branch from e377ccf to b25f638 Compare March 13, 2023 17:44

vkostyukov requested review from trustin and ikhoon and removed request for jrhee17, minwoox, trustin and ikhoon March 13, 2023 17:57

ikhoon reviewed Mar 17, 2023

View reviewed changes

vkostyukov force-pushed the vk/event-loop-metrics branch 2 times, most recently from af936c2 to 3db0fc1 Compare March 18, 2023 00:17

vkostyukov requested a review from ikhoon March 18, 2023 02:46

vkostyukov force-pushed the vk/event-loop-metrics branch from 3db0fc1 to 59624c3 Compare March 18, 2023 03:15

vkostyukov force-pushed the vk/event-loop-metrics branch 2 times, most recently from 4ebf151 to 9cca856 Compare March 22, 2023 05:26

ikhoon reviewed Mar 22, 2023

View reviewed changes

core/src/test/java/com/linecorp/armeria/common/metric/NettyMetricsTest.java Outdated Show resolved Hide resolved

vkostyukov force-pushed the vk/event-loop-metrics branch from 9cca856 to ce03bc1 Compare March 22, 2023 15:54

vkostyukov requested a review from ikhoon March 22, 2023 15:59

minwoox added this to the 1.23.0 milestone Mar 23, 2023

trustin reviewed Mar 23, 2023

View reviewed changes

ikhoon approved these changes Mar 23, 2023

View reviewed changes

vkostyukov force-pushed the vk/event-loop-metrics branch from ce03bc1 to 238fe6f Compare March 23, 2023 16:24

minwoox reviewed Mar 24, 2023

View reviewed changes

vkostyukov force-pushed the vk/event-loop-metrics branch from 238fe6f to 23ac366 Compare March 24, 2023 03:29

minwoox approved these changes Mar 24, 2023

View reviewed changes

jrhee17 approved these changes Mar 24, 2023

View reviewed changes

vkostyukov force-pushed the vk/event-loop-metrics branch from 23ac366 to 7600fd8 Compare March 25, 2023 02:24

jrhee17 approved these changes Mar 27, 2023

View reviewed changes

Add barebones Netty event-loop metrics

f4a84ec

vkostyukov force-pushed the vk/event-loop-metrics branch from 7600fd8 to f4a84ec Compare March 28, 2023 00:48

trustin approved these changes Mar 28, 2023

View reviewed changes

trustin added 2 commits March 28, 2023 15:56

Rename and reorder

009f0c3

Merge branch 'main' into vk/event-loop-metrics

e59e015

trustin merged commit 2dccda3 into line:main Mar 28, 2023

minwoox added improvement and removed new feature labels Mar 29, 2023

minwoox reviewed Mar 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add barebones Netty event-loop metrics #4750

Add barebones Netty event-loop metrics #4750

vkostyukov commented Mar 11, 2023 •

edited by trustin

Loading

trustin left a comment

vkostyukov commented Mar 21, 2023

vkostyukov commented Mar 22, 2023

ikhoon left a comment

trustin left a comment

trustin Mar 23, 2023

vkostyukov Mar 23, 2023

minwoox Mar 27, 2023

vkostyukov Mar 28, 2023

ikhoon left a comment

vkostyukov commented Mar 23, 2023

minwoox left a comment

vkostyukov commented Mar 24, 2023

minwoox left a comment

jrhee17 left a comment •

edited

Loading

jrhee17 Mar 24, 2023

vkostyukov Mar 24, 2023 •

edited

Loading

minwoox Mar 25, 2023 •

edited

Loading

vkostyukov Mar 25, 2023

vkostyukov Mar 25, 2023

minwoox Mar 27, 2023

jrhee17 left a comment

trustin left a comment

trustin commented Mar 28, 2023

minwoox Mar 29, 2023

Add barebones Netty event-loop metrics #4750

Add barebones Netty event-loop metrics #4750

Conversation

vkostyukov commented Mar 11, 2023 • edited by trustin Loading

trustin left a comment

Choose a reason for hiding this comment

vkostyukov commented Mar 21, 2023

vkostyukov commented Mar 22, 2023

ikhoon left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

vkostyukov commented Mar 23, 2023

minwoox left a comment

Choose a reason for hiding this comment

vkostyukov commented Mar 24, 2023

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkostyukov Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

minwoox Mar 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

trustin commented Mar 28, 2023

Choose a reason for hiding this comment

vkostyukov commented Mar 11, 2023 •

edited by trustin

Loading

jrhee17 left a comment •

edited

Loading

vkostyukov Mar 24, 2023 •

edited

Loading

minwoox Mar 25, 2023 •

edited

Loading