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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Bue-von-hon
Copy link
Contributor

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.
this close #2633.

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.

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
3 participants