#1130: Diagnostics/debug info for AsyncHttpClient #1255
Conversation
|
|
||
| for (ListenableFuture<Response> future : futures) { | ||
| future.get(); | ||
| } |
There was a problem hiding this comment.
I'd very much like to add an assert here when the connections are active, but the request/response cycle happens so fast I haven't been able to pull the ClientStats when the requests are still in flight. Is there a way to configure the server that's created by this test to delay responses?
There was a problem hiding this comment.
Have a look at EchoHandler, which is the default handler used in the Jetty servers.
It can use a LockThread header to block the response on the server side. It current performs a hard coded 40s sleep, but it's actually no longer used. It could be used to pass the sleep value instead.
| assertEquals(idleStats.getTotalConnectionCount(), 5); | ||
| assertEquals(idleStats.toString(), "There are 5 total connections, 0 are active and 5 are idle."); | ||
|
|
||
| Thread.sleep(5000); |
There was a problem hiding this comment.
I don't think there's any better solution, as you want to wait for the pool idle timeout to kick in.
|
|
||
| for (ListenableFuture<Response> future : futures) { | ||
| future.get(); | ||
| } |
There was a problem hiding this comment.
Have a look at EchoHandler, which is the default handler used in the Jetty servers.
It can use a LockThread header to block the response on the server side. It current performs a hard coded 40s sleep, but it's actually no longer used. It could be used to pass the sleep value instead.
| assertEquals(idleStats.getTotalConnectionCount(), 5); | ||
| assertEquals(idleStats.toString(), "There are 5 total connections, 0 are active and 5 are idle."); | ||
|
|
||
| Thread.sleep(5000); |
There was a problem hiding this comment.
I don't think there's any better solution, as you want to wait for the pool idle timeout to kick in.
| @@ -0,0 +1,56 @@ | |||
| package org.asynchttpclient; | |||
|
Thanks! |
|
|
||
| /** | ||
| * Created by grenville on 9/24/16. | ||
| */ |
|
@Diagoras gentle ping |
slandelle
force-pushed
the
master
branch
3 times, most recently
from
f0b7cde to
b398e21
Compare
|
@slandelle Sorry, things have been a bit crazy IRL. Should push a commit addressing your comments sometime this weekend. |
|
|
||
| /** | ||
| * Created by grenville on 9/24/16. | ||
| */ |
| this.idleConnectionCount = idleConnectionCount; | ||
| } | ||
|
|
||
| public long getTotalConnectionCount() { |
| */ | ||
| public class ClientStats { | ||
|
|
||
| private final long totalConnectionCount; |
There was a problem hiding this comment.
I don't think it's worth storing
|
|
||
| public ClientStats(final long activeConnectionCount, | ||
| final long idleConnectionCount) { | ||
| this.totalConnectionCount = activeConnectionCount + idleConnectionCount; |
| } | ||
|
|
||
| public long getTotalConnectionCount() { | ||
| return totalConnectionCount; |
| } | ||
|
|
||
| public ClientStats getClientStats() { | ||
| final ChannelManager channelManager = requestSender.getChannelManager(); |
There was a problem hiding this comment.
You don't need that, DefaultAsyncHttpClient has the ChannelManager.
| Channels.setAttribute(channel, newExecuteNextRequestCallback(future, nextRequest)); | ||
| } | ||
|
|
||
| public ChannelManager getChannelManager() { |
There was a problem hiding this comment.
Useless, DefaultAsyncHttpClient has the ChannelManager.
| return eventLoopGroup; | ||
| } | ||
|
|
||
| public ChannelGroup getOpenChannels() { |
| public ClientStats getClientStats() { | ||
| final ChannelManager channelManager = requestSender.getChannelManager(); | ||
| final long activeConnectionCount = channelManager.getOpenChannels().stream().filter(channel -> !channel.isOpen()).count(); | ||
| final long idleConnectionCount = channelManager.getOpenChannels().stream().filter(Channel::isOpen).count(); |
There was a problem hiding this comment.
It should be possible to only scan once, not twice.
There was a problem hiding this comment.
I really wonder if it's possible to get idleConnectionCount different from zero.
| return "There are " + totalConnectionCount + | ||
| " total connections, " + activeConnectionCount + | ||
| " are active and " + idleConnectionCount + " are idle."; | ||
| } |
There was a problem hiding this comment.
implement equals and hashcode
|
No pro, thanks! |
|
Hey, thanks for all the comments! I caught something nasty on Friday and have been out of commission this weekend. Hoping to address them on Monday/Tuesday. |
|
No hurries |
|
So, Monday somehow became a month, but I guess better late than never. I believe I addressed all of your comments, but poke me if I missed one. I also have some questions/comments of my own which I'll add to this PR. |
| } | ||
|
|
||
| public ClientStats getClientStats() { | ||
| final long totalConnectionCount = openChannels.size(); |
There was a problem hiding this comment.
Do we need to filter both this count and and the idle channel count below to ensure we don't count instances of ServerChannel?
Given that the ChannelManager can be set in the config, I imagine someone using Netty as their server could pass their ChannelManager in here. "openChannels" also explicitly contains two Sets, one of which is for server channels.
There was a problem hiding this comment.
Given that the ChannelManager can be set in the config
No, it can't.
There was a problem hiding this comment.
Hmm, okay. So I guess there's no need to filter out ServerChannel instances. Good to know.
| @Override | ||
| public long getIdleChannelCount() { | ||
| return partitions.reduceValuesToLong( | ||
| Long.MAX_VALUE, |
There was a problem hiding this comment.
Since this parameter decides how many elements are needed to activate parallelism, Long.MAX_VALUE pretty much switches off parallel reduction of the elements. Do you think that's too conservative?
|
|
||
| @Override | ||
| public long getIdleChannelCount() { | ||
| return partitions.reduceValuesToLong( |
There was a problem hiding this comment.
This method is neat, but examining the implementation shows that it involves blocking on a ForkJoinTask. Unless there's a particular reason to want it, it might be better to just call:
partitions.values().stream().map(ConcurrentLinkedDeque::size).sum().
There was a problem hiding this comment.
Yeah, I'd go with summing ConcurrentLinkedDeque::size. Those stats would just be an estimate anyway, you can't get a proper snapshot without freezing, which we of course don't want.
| @@ -0,0 +1,78 @@ | |||
| /* | |||
| * Copyright 2010 Ning, Inc. | |||
There was a problem hiding this comment.
Nope. Ning hasn't been involved in here for years. I've never even been in touch with them.
Code that goes back to their sponsorship still have their copyright, but all new code goes under AsyncHttpClient Project
| } | ||
|
|
||
| public ClientStats getClientStats() { | ||
| final long totalConnectionCount = openChannels.size(); |
There was a problem hiding this comment.
Given that the ChannelManager can be set in the config
No, it can't.
|
|
||
| @Override | ||
| public long getIdleChannelCount() { | ||
| return partitions.reduceValuesToLong( |
There was a problem hiding this comment.
Yeah, I'd go with summing ConcurrentLinkedDeque::size. Those stats would just be an estimate anyway, you can't get a proper snapshot without freezing, which we of course don't want.
| @@ -0,0 +1,56 @@ | |||
| package org.asynchttpclient; | |||
| private final long idleConnectionCount; | ||
|
|
||
| public ClientStats(final long activeConnectionCount, | ||
| final long idleConnectionCount) { |
There was a problem hiding this comment.
don't really need final here
There was a problem hiding this comment.
I always try to make my references immutable by default, but I can change it.
There was a problem hiding this comment.
Just a small nit. But considering the code below, the finals are pretty useless here. God, Scala gets it right here (immutability is the default)...
There was a problem hiding this comment.
You can say that again.
|
|
||
| @Override | ||
| public ClientStats getClientStats() { | ||
| return null; |
There was a problem hiding this comment.
throw new UnsupportedOperationException
|
|
||
| @Override | ||
| public ClientStats getClientStats() { | ||
| return null; |
There was a problem hiding this comment.
throw new UnsupportedOperationException
| assertEquals(emptyStats.getTotalConnectionCount(), 0); | ||
|
|
||
| final List<ListenableFuture<Response>> futures = new ArrayList<>(); | ||
| for (int i = 0; i < 5; i++) { |
| // Let's make sure the active count is correct when reusing cached connections. | ||
|
|
||
| final List<ListenableFuture<Response>> repeatedFutures = new ArrayList<>(); | ||
| for (int i = 0; i < 3; i++) { |
| return 0; | ||
| } | ||
|
|
||
|
|
|
Hey! Thanks for updating! Sorry, I still have some comments... |
| * | ||
| * @return a {@link ClientStats} | ||
| */ | ||
| ClientStats getClientStats(); |
There was a problem hiding this comment.
Let's aim for AHC 2.1 so we can change this API.
There was a problem hiding this comment.
Then I should reopen this PR against branch "2.1", right?
There was a problem hiding this comment.
No. The 2.1 branch will be dropped by a few weeks when I'll move the master to Netty 4.1. This branch was solely for the Play people so they can make progress on their own upgrade. It lags behind master in terms for bug fixes and refactoring.
|
I added per host stats, so I think we're feature complete unless you can think of anything else people might want. |
| private final Map<String, HostStats> statsPerHost; | ||
|
|
||
| public ClientStats(Map<String, HostStats> statsPerHost) { | ||
| this.statsPerHost = Collections.unmodifiableMap(statsPerHost); |
There was a problem hiding this comment.
Java doesn't have immutable collections and Guava (with its ImmutableMap) isn't in scope, so I went with this. Sadly, there's no way to have the type reflect that this Map is immutable, so it's noted in the Javadoc instead.
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
Went ahead and kept the old toString, but I'm wondering if it should be changed to also call toString on all the HostStatistics in the "statsPerHost" Map, or if that would be too verbose for a default.
| return eventLoopGroup; | ||
| } | ||
|
|
||
| public ClientStats getClientStats() { |
There was a problem hiding this comment.
The implementation of this method does not make me happy, but I really don't see a better way to do this with Java 8 Streams. If you think there's a better approach, let me know.
| .stream() | ||
| .map(Channel::remoteAddress) | ||
| .filter(a -> a.getClass() == InetSocketAddress.class) | ||
| .map(a -> (InetSocketAddress) a) |
There was a problem hiding this comment.
So this sucks, but I didn't really see another way out. Are there any other types of socket that are likely to live here or in DefaultChannelPool, or is this a mostly "safe" thing to do?
There was a problem hiding this comment.
I think it's pretty safe to cast
There was a problem hiding this comment.
Yeah, the filter beforehand means we're not going to see an exception. I was just worried about leaving out other kinds of sockets from the stats, but it sounds like we're good.
| .stream() | ||
| .map(Channel::remoteAddress) | ||
| .filter(a -> a.getClass() == InetSocketAddress.class) | ||
| .map(a -> (InetSocketAddress) a) |
There was a problem hiding this comment.
I think it's pretty safe to cast
|
So, are we good to merge? And if so, should I squash my commits? |
|
I hope to be able to review your latest changes in length this week. |
2 participants
This is a first crack at addressing that issue, with only information on the total number of connections and how many are active or idle. At the very least, it'd be nice to also see the same data on a per host basis, but I thought I'd get what I've done so far looked at first.