search

line / armeria

Your go-to microservice framework for any situation, from the creator of Netty et al. You can build any type of microservice leveraging your favorite technologies, including gRPC, Thrift, Kotlin, Retrofit, Reactive Streams, Spring Boot and Dropwizard.
https://armeria.dev
Apache License 2.0
4.83k stars 922 forks source link

Add metrics about PooledByteBufAllocator #5916

Open Bue-von-hon opened 2 months ago

Bue-von-hon commented 2 months ago

Motivation: Add metrics related to PooledByteBufAllocator that are already exposed by the netty.

Modifications:

Result: Enables the PooledByteBufAllocator metric. this close #2633.

github-actions[bot] commented 2 months ago

🔍 Build Scan® (commit: 5b3a182e9c27bb688d0224cf3e0e75dc02941ac8)

Job name Status Build Scan®
build-ubicloud-standard-8-jdk-8 ✅ https://ge.armeria.dev/s/isinntry5h4va
build-ubicloud-standard-8-jdk-21-snapshot-blockhound ✅ https://ge.armeria.dev/s/ich4rgpyci2qk
build-ubicloud-standard-8-jdk-17-min-java-17-coverage ❌ (failure) https://ge.armeria.dev/s/4otvl7e6c5y4o
build-ubicloud-standard-8-jdk-17-min-java-11 ✅ https://ge.armeria.dev/s/rztcz47ndxcyg
build-ubicloud-standard-8-jdk-17-leak ✅ https://ge.armeria.dev/s/sdwcokqsqa2by
build-ubicloud-standard-8-jdk-11 ✅ https://ge.armeria.dev/s/l2gtlaxrg3vvc
build-macos-12-jdk-21 ✅ https://ge.armeria.dev/s/6lsrkb66obsjm
jrhee17 commented 2 months ago

Micrometer already provides bindings to Netty Metrics (ref: https://docs.micrometer.io/micrometer/reference/reference/netty.html)

I prefer that we either: 1) We close this issue and allow users to add metric bindings themselves. Adding the metric bindings is not difficult and the scope of applying this metric seems ambiguous 2) We add bindings for servers only by default. The name will probably be a combination of local address + ports.

Let me know what you think @line/dx

ikhoon commented 2 months ago

I didn't know that Netty metrics were supported by Micrometer finally.

I prefer automatically setting the metric for the default one (ByteBufAllocator.DEFAULT) when the MoreMeterBinders class is loaded.

Bue-von-hon commented 3 days ago

@ikhoon @jrhee17 @trustin @minwoox I trust this implementation satisfies your requirements. If the goal was to consolidate PooledByteBufAllocatorMetrics with eventLoopMetrics or certificateMetrics that would involve substantial modifications. (However, I don't believe that was the intended direction 😅)