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

HTTPCLIENT-2233- Add Metrics Collection for IOReactor Thread Pool #490

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

arturobernalg
Copy link
Member

This PR addresses HTTPCLIENT-2233 by implementing a mechanism for collecting metrics on the IOReactor thread pool in Apache HttpClient. The changes introduce an IOReactorMetricsListener interface and a LoggingIOReactorMetricsListener implementation to monitor and report key metrics for better insight into reactor performance and load.

@ok2c
Copy link
Member

ok2c commented Oct 1, 2024

@arturobernalg

  1. The original reporter of HTTPCLIENT-2233 never properly specified what kind of metrics they needed. Can we be sure this is what users need or we end up adding more complexity with little benefit?
  2. If you decide to proceed with this PR please add missing @since tags. There is a lot of new methods
  3. Please note the release series of core is 5.4 not 5.5. Please correct @since 5.5 tags

@arturobernalg
Copy link
Member Author

@arturobernalg

1. The original reporter of [HTTPCLIENT-2233](https://issues.apache.org/jira/browse/HTTPCLIENT-2233) never properly specified what kind of metrics they needed. Can we be sure this is what users need or we end up adding more complexity with little benefit?

2. If you decide to proceed with this PR please add missing ` @since` tags. There is a lot of new methods

3. Please note the release series of core is 5.4 not 5.5. Please correct ` @since 5.5` tags

@ok2c
I understand the original request was not very specific, so I tried to include metrics that would be valuable for users monitoring I/O reactor performance. The metrics added focus on active thread count, pending connections, thread pool saturation, and queue wait time, which are key indicators for system health and performance in an async environment. I believe these metrics can provide meaningful insights for users needing more control over their thread pool's performance.

@ok2c
Copy link
Member

ok2c commented Oct 2, 2024

I believe these metrics can provide meaningful insights for users needing more control over their thread pool's performance.

@arturobernalg All right. Please do add missing @since tags, though.

@@ -315,6 +326,18 @@ private void validateAddress(final SocketAddress address) throws UnknownHostExce
private void processPendingConnectionRequests() {
IOSessionRequest sessionRequest;
for (int i = 0; i < MAX_CHANNEL_REQUESTS && (sessionRequest = this.requestQueue.poll()) != null; i++) {

// Calculate wait time safely without keeping long-lived state
final long waitTimeMillis = System.currentTimeMillis() - sessionRequest.getEnqueueTime();
Copy link
Member

Choose a reason for hiding this comment

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

@arturobernalg Would you agree to move metrics collection inside if (threadPoolListener != null) {? There is no need to collect them if there is no listener.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ok2c done

…oring

This change enhances monitoring capabilities for the IOReactor thread pool, providing better insights into performance and potential bottlenecks.
@arturobernalg arturobernalg merged commit 0a69668 into apache:master Oct 3, 2024
10 checks passed
ok2c pushed a commit that referenced this pull request Oct 3, 2024
…oring (#490)

This change enhances monitoring capabilities for the IOReactor thread pool, providing better insights into performance and potential bottlenecks.
@ok2c
Copy link
Member

ok2c commented Oct 3, 2024

@arturobernalg I added missing @since tags and tweaked the sequence of constructor parameters a tiny bit.

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

Successfully merging this pull request may close these issues.

2 participants