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.

Sign up for GitHub

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

Jump to bottom

Deregister connection pool metrics from Micrometer (or alternate registry) when disposing the connection pool #2608

Merged
merged 4 commits into from Dec 12, 2022
Merged

Deregister connection pool metrics from Micrometer (or alternate registry) when disposing the connection pool #2608

merged 4 commits into from Dec 12, 2022

Conversation

manolama
Copy link
Contributor

@manolama manolama commented Dec 9, 2022 •
edited by violetagg

Deregister pool metrics from Micrometer (or alternate registry) in order to release resources when there is a large churn of connection pools and endpoints.

@manolama
Deregister pool metrics from Micrometer (or alternate registry) in or…
8a2cf35 
…der to

release resources when there is a large churn of connection pools and endpoints.
violetagg
violetagg requested changes Dec 9, 2022
View reviewed changes
Copy link
Member

@violetagg violetagg left a comment •
edited

Choose a reason for hiding this comment

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

@manolama Thanks for the PR!

I have several comments.

Also I want to add the link below for completeness
https://micrometer.io/docs/concepts#_why_is_my_gauge_reporting_nan_or_disappearing

Once Reactor Netty disposes a connection pool there is no strong reference anymore, so NaN is expected to be returned by the Gauges.

reactor-netty-core/src/main/java/reactor/netty/resources/ConnectionProvider.java Outdated Show resolved Hide resolved
reactor-netty-core/src/test/java/reactor/netty/resources/ConnectionProviderTest.java Outdated Show resolved Hide resolved
...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
...src/main/java/reactor/netty/http/client/MicrometerHttp2ConnectionProviderMeterRegistrar.java Show resolved Hide resolved
...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
@violetagg violetagg added the type/enhancement A general enhancement label Dec 9, 2022
@violetagg violetagg added this to the 1.0.26 milestone Dec 9, 2022
@violetagg violetagg changed the title Deregister pool metrics from Micrometer (or alternate registry) in or… Deregister connection pool metrics from Micrometer (or alternate registry) when disposing the connection pool Dec 9, 2022
@manolama
Address code review. Provide a default deRegister implementation to a…
9138d34 
…void interface

changes for now. Remove unecessary Connection return from the unit metrics unit test.
Thanks @violetagg
@manolama
Copy link
Contributor Author

manolama commented Dec 9, 2022

Thanks!

Once Reactor Netty disposes a connection pool there is no strong reference anymore, so NaN is expected to be returned by the Gauges.
Right, that works as expected. Unfortunately the Meter and Id stick around in the Micrometer map so we want to remove those to release the Id strings and stop reporting.

Context for the PR (from internal chats):
We have some services that exhibit a mesh behavior where an app may connect to multiple, individual instances of other server apps in a group. A connection pool is instantiated for each of those instances. We also have frequent deployments of the server apps and each new instance picks up a different IP. So if a client app is long lived and multiple deployments occur for the server apps, we wind up with stale Meters in Micrometer. This will help remove stale Meters.

@manolama
Fix the testConnectionPoolPendingAcquireSize test to detect deregis…
e58a81c 
…tration

of the pool metrics after disposal.
@violetagg violetagg requested a review from a team December 9, 2022 20:13
@violetagg
Copy link
Member

@reactor/netty-team PTAL

the failing tests on Windows OS are not related
the build on Mac OS with native transport seems to be hanging but I don't think it is related

@violetagg violetagg merged commit f53f4c6 into reactor:main Dec 12, 2022
violetagg pushed a commit that referenced this pull request Dec 12, 2022
@manolama @violetagg
Deregister connection pool metrics from Micrometer (or alternate regi…
851f2e0 
…stry) when disposing the connection pool (#2608)

Adjust #2608 backport to 1.0.x
violetagg added a commit that referenced this pull request Dec 12, 2022
@violetagg
Merge-ignore 851f2e0 into 1.1.1
1416e59 
The change is already available via #2608
violetagg added a commit that referenced this pull request Dec 12, 2022
@violetagg
Merge #2608 into 2.0.0-M4
d779904
@violetagg
Copy link
Member

@manolama I back ported this change to 1.0.x for 1.0.26

@violetagg violetagg changed the title Deregister connection pool metrics from Micrometer (or alternate registry) when disposing the connection pool Deregister connection pool metrics from Micrometer (or alternate registry) when disposing the connection pool Dec 12, 2022
@violetagg violetagg mentioned this pull request Aug 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pderop pderop pderop left review comments

@violetagg violetagg violetagg approved these changes

Assignees
No one assigned
Labels
type/enhancement A general enhancement
Projects
None yet
Milestone
1.0.26
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@manolama @violetagg @pderop