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

Apache connection pool instrumentation #457

Closed
wants to merge 5 commits into from
Closed

Apache connection pool instrumentation #457

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da57615
Apache connection pool instrumentation
iamdanfox Feb 28, 2020
344fb30
real types instead of tritium
iamdanfox Feb 28, 2020
b20e460
Add generated changelog entries
iamdanfox Feb 28, 2020
930be65
Needless indirection
iamdanfox Feb 28, 2020
37886cd
test after response closed
iamdanfox Feb 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/@unreleased/pr-457.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: improvement
improvement:
description: Each closeable apache client now exposes methods which reveal statistics
about the connection pool.
links:
- https://github.com/palantir/dialogue/pull/457
54 changes: 40 additions & 14 deletions ...e-apache-hc4-client/src/main/java/com/palantir/dialogue/hc4/ApacheHttpClientChannels.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,18 @@
import org.apache.http.client.config.RequestConfig;
import org.apache.http.config.RegistryBuilder;
import org.apache.http.config.SocketConfig;
import org.apache.http.conn.socket.ConnectionSocketFactory;
import org.apache.http.conn.socket.PlainConnectionSocketFactory;
import org.apache.http.conn.ssl.DefaultHostnameVerifier;
import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
import org.apache.http.impl.auth.BasicSchemeFactory;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.http.impl.client.HttpClients;
import org.apache.http.impl.client.ProxyAuthenticationStrategy;
import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
import org.apache.http.impl.conn.SystemDefaultRoutePlanner;
import org.apache.http.pool.PoolStats;
import org.apache.http.protocol.HttpContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -96,6 +100,26 @@ public static CloseableClient createCloseableHttpClient(ClientConfiguration conf
long socketTimeoutMillis =
Math.max(conf.readTimeout().toMillis(), conf.writeTimeout().toMillis());
int connectTimeout = Ints.checkedCast(conf.connectTimeout().toMillis());

SocketConfig socketConfig = SocketConfig.custom().setSoKeepAlive(true).build();
SSLConnectionSocketFactory sslSocketFactory = new SSLConnectionSocketFactory(
conf.sslSocketFactory(),
new String[] {"TLSv1.2"},
conf.enableGcmCipherSuites()
? jvmSupportedCipherSuites(CipherSuites.allCipherSuites())
: jvmSupportedCipherSuites(CipherSuites.fastCipherSuites()),
new DefaultHostnameVerifier());

PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(
RegistryBuilder.<ConnectionSocketFactory>create()
.register("http", PlainConnectionSocketFactory.getSocketFactory())
.register("https", sslSocketFactory)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can wrap these to add a meter of connection rate, and wrap the pool manager to capture the rate connections requested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm that would be cool, so dividing one by the other tells us how effectively we're re-using... Do we not already get that connection rate from the tritium tls.handshakes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tritium handshake instrumentation would work assuming https, but I don’t think we wire it up yet.

.build());

connectionManager.setDefaultSocketConfig(socketConfig);
connectionManager.setMaxTotal(Integer.MAX_VALUE);
connectionManager.setDefaultMaxPerRoute(Integer.MAX_VALUE);

HttpClientBuilder builder = HttpClients.custom()
.setDefaultRequestConfig(RequestConfig.custom()
.setSocketTimeout(Ints.checkedCast(socketTimeoutMillis))
Expand All @@ -106,11 +130,10 @@ public static CloseableClient createCloseableHttpClient(ClientConfiguration conf
.setRedirectsEnabled(false)
.setRelativeRedirectsAllowed(false)
.build())
.setDefaultSocketConfig(
SocketConfig.custom().setSoKeepAlive(true).build())
.setDefaultSocketConfig(socketConfig)
.evictIdleConnections(55, TimeUnit.SECONDS)
.setMaxConnPerRoute(Integer.MAX_VALUE)
.setMaxConnTotal(Integer.MAX_VALUE)
.setConnectionManagerShared(false) // will be closed when the client is closed
.setConnectionManager(connectionManager)
// TODO(ckozak): proxy credentials
.setRoutePlanner(new SystemDefaultRoutePlanner(null, conf.proxy()))
.disableAutomaticRetries()
Expand All @@ -120,14 +143,7 @@ public static CloseableClient createCloseableHttpClient(ClientConfiguration conf
.disableCookieManagement()
// Dialogue handles content-compression with ContentDecodingChannel
.disableContentCompression()
.setSSLSocketFactory(
new SSLConnectionSocketFactory(
conf.sslSocketFactory(),
new String[] {"TLSv1.2"},
conf.enableGcmCipherSuites()
? jvmSupportedCipherSuites(CipherSuites.allCipherSuites())
: jvmSupportedCipherSuites(CipherSuites.fastCipherSuites()),
new DefaultHostnameVerifier()))
.setSSLSocketFactory(sslSocketFactory)
.setDefaultCredentialsProvider(NullCredentialsProvider.INSTANCE)
.setTargetAuthenticationStrategy(NullAuthenticationStrategy.INSTANCE)
.setProxyAuthenticationStrategy(NullAuthenticationStrategy.INSTANCE)
Expand All @@ -141,15 +157,17 @@ public static CloseableClient createCloseableHttpClient(ClientConfiguration conf
.build());
});

return new CloseableClient(builder.build());
return new CloseableClient(builder.build(), connectionManager);
}

/** Intentionally opaque wrapper type - we don't want people using the inner Apache client directly. */
public static final class CloseableClient implements Closeable {
private final CloseableHttpClient client;
private final PoolingHttpClientConnectionManager connectionManager;

CloseableClient(CloseableHttpClient client) {
CloseableClient(CloseableHttpClient client, PoolingHttpClientConnectionManager connectionManager) {
this.client = client;
this.connectionManager = connectionManager;
}

@Override
Expand All @@ -161,6 +179,14 @@ public void close() throws IOException {
public String toString() {
return "CloseableClient{client=" + client + '}';
}

public PoolStats getPoolStats() {
return connectionManager.getTotalStats();
}

public int getNumRoutes() {
return connectionManager.getRoutes().size();
}
}

/**
Expand Down
21 changes: 21 additions & 0 deletions ...ache-hc4-client/src/test/java/com/palantir/dialogue/hc4/ApacheHttpClientChannelsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.palantir.dialogue.hc4;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import com.google.common.util.concurrent.Futures;
Expand Down Expand Up @@ -58,6 +59,26 @@ public void close_works() throws Exception {
assertThatThrownBy(() -> again.get()).hasMessageContaining("Connection pool shut down");
}

@Test
public void metrics() throws Exception {
ClientConfiguration conf = TestConfigurations.create("http://unused");

try (ApacheHttpClientChannels.CloseableClient client =
ApacheHttpClientChannels.createCloseableHttpClient(conf)) {

Channel channel = ApacheHttpClientChannels.createSingleUri("http://neverssl.com", client);
ListenableFuture<Response> response =
channel.execute(new TestEndpoint(), Request.builder().build());
assertThat(Futures.getUnchecked(response).code()).isEqualTo(200);

assertThat(client.getNumRoutes()).describedAs("routes").isEqualTo(1);
assertThat(client.getPoolStats().getAvailable())
.describedAs("available")
.isEqualTo(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think graphing the number available will be kinda interesting, as this should tell us how quickly we could handle bursts of traffic

 /**
     * Gets the number idle persistent connections.
     * <p>
     * The total number of connections in the pool is equal to {@code available} plus {@code leased}.
     * </p>
     *
     * @return number idle persistent connections.
     */
    public int getAvailable() {

assertThat(client.getPoolStats().getLeased()).describedAs("leased").isEqualTo(1);
}
}

private static final class TestEndpoint implements Endpoint {
@Override
public void renderPath(Map<String, String> _params, UrlBuilder _url) {}
Expand Down