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

Jump to bottom

Issue 2187 #2237

Merged
merged 10 commits into from Jun 24, 2022
Merged

Issue 2187 #2237

merged 10 commits into from Jun 24, 2022

Conversation

ChristianLMI
Copy link
Contributor

Fixing issues described in #2187 by wrapping only metrics related code in try-catch and logging issues on warn level.

@pivotal-cla
Copy link

@ChristianLMI Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@ChristianLMI ChristianLMI changed the base branch from main to 1.0.x May 23, 2022 07:12
@pivotal-cla
Copy link

@ChristianLMI Thank you for signing the Contributor License Agreement!

violetagg
violetagg requested changes May 30, 2022
View reviewed changes
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

Please fix the checkstyle warnings.

...ty-core/src/test/java/reactor/netty/resources/PooledConnectionProviderCustomMetricsTest.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
violetagg
violetagg requested changes May 30, 2022
View reviewed changes
reactor-netty-core/src/test/java/reactor/netty/transport/TransportEventLoopMetricsTest.java Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
violetagg
violetagg requested changes Jun 2, 2022
View reviewed changes
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
@violetagg violetagg added the type/bug A general bug label Jun 2, 2022
@violetagg violetagg added this to the 1.0.20 milestone Jun 2, 2022
@violetagg violetagg linked an issue Jun 2, 2022 that may be closed by this pull request
Recording metrics could cause errors in operational code #2187
Closed
violetagg
violetagg requested changes Jun 2, 2022
View reviewed changes
Copy link
Member

@violetagg violetagg left a comment

Choose a reason for hiding this comment

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

I think that we need to handle also the recordings

reactor.netty.http.server.AbstractHttpServerMetricsHandler#channelActive
reactor.netty.http.server.AbstractHttpServerMetricsHandler#channelInactive

reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Show resolved Hide resolved
@violetagg violetagg modified the milestones: 1.0.20, 1.0.21 Jun 9, 2022
@ChristianLMI
Copy link
Contributor Author

The 1.0.x branch has advanced. Should I rebase to keep this in sync?

@violetagg
Copy link
Member

The 1.0.x branch has advanced. Should I rebase to keep this in sync?

Yes please rebase. Thanks

violetagg
violetagg requested changes Jun 24, 2022
View reviewed changes
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
reactor-netty-http/src/test/java/reactor/netty/http/HttpMetricsHandlerTests.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/client/AbstractHttpClientMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
...tor-netty-http/src/main/java/reactor/netty/http/server/AbstractHttpServerMetricsHandler.java Outdated Show resolved Hide resolved
@violetagg violetagg requested a review from a team June 24, 2022 08:41
@violetagg
Copy link
Member

@reactor/netty-team PTAL

pderop
pderop approved these changes Jun 24, 2022
View reviewed changes
Copy link
Member

@pderop pderop left a comment

Choose a reason for hiding this comment

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

LGTM

@violetagg
Copy link
Member

@pderop Thanks for the review

@ChristianLMI Thanks for the PR

@violetagg violetagg merged commit 86cb1db into reactor:1.0.x Jun 24, 2022
violetagg added a commit that referenced this pull request Jun 24, 2022
@violetagg
Merge #2237 into 1.1.0-M4
12544db
@violetagg violetagg added type/enhancement A general enhancement and removed type/bug A general bug labels Jun 24, 2022
violetagg added a commit that referenced this pull request Jun 24, 2022
@violetagg
Merge #2237 into netty5
c8cf6b9
@pderop pderop mentioned this pull request Jun 28, 2022
Merged
pderop added a commit that referenced this pull request Jun 28, 2022
@pderop
Ensure active connections is decremented (#2335)
d1b94b8 
This PR is an improvement for #2237.
When the AbstractHttpServerMetricsHandler.write method is getting an exception thrown by recordWrite, then the recordInactiveConnection is never called. So, this is too bad because the long adder for the active connection is then never decremented by the missed call to recordInactiveConnection, resulting in leaving the number of active connections to an inconsistent value.

Fixes #2187
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pderop pderop pderop approved these changes

@violetagg violetagg violetagg approved these changes

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

Successfully merging this pull request may close these issues.

Recording metrics could cause errors in operational code
4 participants
@ChristianLMI @pivotal-cla @violetagg @pderop