Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatically register the metics of PooledByteBufAllocator.DEFAULT #5916

Merged
merged 9 commits into from
Nov 25, 2024

Conversation

Bue-von-hon
Copy link
Contributor

@Bue-von-hon Bue-von-hon commented Sep 18, 2024

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

Modifications:

  • NettyAllocatorMetrics are registered when MoreMeterBinders is initialized.

Result:

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

Modifications:
- The PooledByteBufAllocator metric is now available in MoreMeterBinders.
- To expose the PooledByteBufAllocator metrics, created the PooledByteBufAllocatorMetrics class.

Result:
Enables the PooledByteBufAllocator metric.
@Bue-von-hon Bue-von-hon marked this pull request as draft September 18, 2024 06:10
@jrhee17
Copy link
Contributor

jrhee17 commented Sep 19, 2024

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
Copy link
Contributor

ikhoon commented Sep 20, 2024

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.

@jrhee17 jrhee17 added this to the 1.32.0 milestone Nov 5, 2024
@Bue-von-hon
Copy link
Contributor Author

@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 😅)

@Bue-von-hon Bue-von-hon marked this pull request as ready for review November 16, 2024 13:05
@Bue-von-hon
Copy link
Contributor Author

@ikhoon @jrhee17 @trustin @minwoox gentle ping.

@ikhoon
Copy link
Contributor

ikhoon commented Nov 21, 2024

I think my intention was not conveyed. Unlike when you first created this PR, Micrometer now supports Netty metrics. Let me add a commit to apply that.

@ikhoon ikhoon changed the title Add metrics about PooledByteBufAllocator Automatically register the metics of PooledByteBufAllocator.DEFAULT Nov 21, 2024
@ikhoon
Copy link
Contributor

ikhoon commented Nov 21, 2024

Changed the title of this PR to reflect the purpose. 🙇‍♂️

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @Bue-von-hon 👍🙏

@Bue-von-hon
Copy link
Contributor Author

@jrhee17 @trustin @minwoox gentle ping.

@Bue-von-hon
Copy link
Contributor Author

@jrhee17 @trustin @minwoox gentle ping.

import io.netty.channel.EventLoopGroup;

/**
* Provides useful {@link MeterBinder}s to monitor various Armeria components.
*/
public final class MoreMeterBinders {

static {
// Bind the default Netty allocator metrics to the default MeterRegistry.
new NettyAllocatorMetrics(PooledByteBufAllocator.DEFAULT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: armeria always initializes this class when a client or server is built

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

@ikhoon ikhoon merged commit 7a6ddd7 into line:main Nov 25, 2024
14 checks passed
@ikhoon
Copy link
Contributor

ikhoon commented Nov 25, 2024

@Bue-von-hon Thanks for your effort. That said maintainers are working on various issues and are aware of the status of this PR, so you may not need to keep mentioning us. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metrics about PooledByteBufAllocator
4 participants