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

More logging around Apache client creation & close #655

Merged
merged 5 commits into from
Apr 15, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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-655.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: improvement
improvement:
description: '`ApacheHttpClientChannels` prints some debug log lines around creation
& close'
links:
- https://github.com/palantir/dialogue/pull/655
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
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 @@ -145,21 +146,37 @@ private static MetricName clientPoolSizeMetricName(String clientName, String sta

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

@Nullable
private final ExecutorService executor;

CloseableClient(
CloseableHttpClient client, ResponseLeakDetector leakDetector, @Nullable ExecutorService executor) {
String name,
CloseableHttpClient client,
PoolingHttpClientConnectionManager pool,
ResponseLeakDetector leakDetector,
@Nullable ExecutorService executor) {
log.debug("Apache client created", SafeArg.of("name", name));
Copy link
Contributor

@ferozco ferozco Apr 15, 2020

Choose a reason for hiding this comment

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

we already produce a meter with this information internally, not sure if this is necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Metrics come out at a 30second granularity. Also the semantics of the metric produced by WC could change and we wouldn't be able to tell from the graph. This is as close to the 'ground truth' as I think we can get.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think info level is reasonable here

Copy link
Contributor

Choose a reason for hiding this comment

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

ya we'd expect this to happen incredibly infrequently right?

Copy link
Contributor Author

@iamdanfox iamdanfox Apr 15, 2020

Choose a reason for hiding this comment

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

yep - most services don't try to live-reload their security / ssl config, so I'd expect to see this log line once in the lifetime of a server.

this.name = name;
this.client = client;
this.pool = pool;
this.leakDetector = leakDetector;
this.executor = executor;
}

@Override
public void close() throws IOException {
PoolStats poolStats = pool.getTotalStats();
log.debug(
"Closing Apache client",
SafeArg.of("name", name),
SafeArg.of("idle", poolStats.getAvailable()),
SafeArg.of("leased", poolStats.getLeased()),
SafeArg.of("pending", poolStats.getPending()));
client.close();
}

Expand Down Expand Up @@ -301,7 +318,11 @@ public CloseableClient build() {
});

return new CloseableClient(
builder.build(), ResponseLeakDetector.of(name, conf.taggedMetricRegistry()), executor);
name,
builder.build(),
connectionManager,
ResponseLeakDetector.of(name, conf.taggedMetricRegistry()),
executor);
}
}

Expand Down