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

Recording metrics could cause errors in operational code #2187

Closed
ChristianLMI opened this issue May 11, 2022 · 4 comments · Fixed by #2237
Closed

Recording metrics could cause errors in operational code #2187

ChristianLMI opened this issue May 11, 2022 · 4 comments · Fixed by #2237
Labels
good first issue Ideal for a new contributor, we'll help help wanted We need contributions on this type/enhancement A general enhancement
Milestone

Comments

@ChristianLMI
Copy link
Contributor

Motivation

I recently ran into an issue where an exception thrown in Micrometer caused my request to break. The actual issue I ran into is not in reactor although reactor was used. However as reactor uses Micrometer as well and e.g. [here|https://github.com/reactor/reactor-netty/blob/main/reactor-netty-core/src/main/java/reactor/netty/channel/AbstractChannelMetricsHandler.java] there also doesn't seem to be any protection of the main functionality against errors in metrics recording, this might be good to look at in reactor as well.

Desired solution

IMHO metrics should be of lower priority than production results. Therefor a failing metric registration should write a log entry but not abort the current operation.

Additional context

In my case a misconfigured a MeterFilter that caused NullPointerExceptions when recording a filtered metric.

@ChristianLMI ChristianLMI added status/need-triage A new issue that still need to be evaluated as a whole type/enhancement A general enhancement labels May 11, 2022
@violetagg violetagg removed the status/need-triage A new issue that still need to be evaluated as a whole label May 13, 2022
@violetagg violetagg added this to the 1.0.x Backlog milestone May 13, 2022
@violetagg violetagg added good first issue Ideal for a new contributor, we'll help help wanted We need contributions on this labels May 13, 2022
@violetagg
Copy link
Member

@ChristianLMI Would you like to provide a PR?

@ChristianLMI
Copy link
Contributor Author

I'll try to find some time to at least get a first step done. However as I'm not too familiar with the project I might miss edges ;)

@violetagg
Copy link
Member

@ChristianLMI It will be great if you can find time. If you decide to work on this please do the changes against 1.0.x branch.

The classes that need to have this enhancement are:
reactor-netty-core

  • reactor.netty.channel.AbstractChannelMetricsHandler
  • reactor.netty.channel.ChannelMetricsHandler
  • reactor.netty.channel.ContextAwareChannelMetricsHandler
  • reactor.netty.transport.AddressResolverGroupMetrics

reactor-netty-http

  • reactor.netty.http.client.AbstractHttpClientMetricsHandler
  • reactor.netty.http.client.ContextAwareHttpClientMetricsHandler
  • reactor.netty.http.server.AbstractHttpServerMetricsHandler
  • reactor.netty.http.server.ContextAwareHttpServerMetricsHandler

ChristianLMI added a commit to ChristianLMI/reactor-netty that referenced this issue May 23, 2022
ChristianLMI added a commit to ChristianLMI/reactor-netty that referenced this issue May 23, 2022
ChristianLMI added a commit to ChristianLMI/reactor-netty that referenced this issue May 23, 2022
ChristianLMI added a commit to ChristianLMI/reactor-netty that referenced this issue May 23, 2022
@ChristianLMI
Copy link
Contributor Author

I created a PR. However now I need to figure out how to get an approval for the CLA.

@violetagg violetagg modified the milestones: 1.0.x Backlog, 1.0.20 Jun 2, 2022
@violetagg violetagg linked a pull request Jun 2, 2022 that will close this issue
@violetagg violetagg modified the milestones: 1.0.20, 1.0.21 Jun 9, 2022
ChristianLMI added a commit to ChristianLMI/reactor-netty that referenced this issue Jun 21, 2022
ChristianLMI added a commit to ChristianLMI/reactor-netty that referenced this issue Jun 21, 2022
pderop added a commit that referenced this issue Jun 28, 2022
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
Labels
good first issue Ideal for a new contributor, we'll help help wanted We need contributions on this type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants