Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

#1130: Diagnostics/debug info for AsyncHttpClient #1255

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

Merged
slandelle merged 4 commits into AsyncHttpClient:master from Diagoras:1130
Nov 14, 2016

Conversation

@Diagoras
Copy link
Contributor

@Diagoras Diagoras commented Sep 26, 2016

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.

Spikhalskiy reacted with thumbs up emoji

for (ListenableFuture<Response> future : futures) {
future.get();
}
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'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?

Copy link
Contributor

@slandelle slandelle Sep 27, 2016

Choose a reason for hiding this comment

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

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.

Diagoras reacted with thumbs up emoji
assertEquals(idleStats.getTotalConnectionCount(), 5);
assertEquals(idleStats.toString(), "There are 5 total connections, 0 are active and 5 are idle.");

Thread.sleep(5000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bleh. :(

Copy link
Contributor

@slandelle slandelle Sep 27, 2016

Choose a reason for hiding this comment

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

I don't think there's any better solution, as you want to wait for the pool idle timeout to kick in.

Diagoras reacted with thumbs up emoji

for (ListenableFuture<Response> future : futures) {
future.get();
}
Copy link
Contributor

@slandelle slandelle Sep 27, 2016

Choose a reason for hiding this comment

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

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.

Diagoras reacted with thumbs up emoji
assertEquals(idleStats.getTotalConnectionCount(), 5);
assertEquals(idleStats.toString(), "There are 5 total connections, 0 are active and 5 are idle.");

Thread.sleep(5000);
Copy link
Contributor

@slandelle slandelle Sep 27, 2016

Choose a reason for hiding this comment

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

I don't think there's any better solution, as you want to wait for the pool idle timeout to kick in.

Diagoras reacted with thumbs up emoji
@@ -0,0 +1,56 @@
package org.asynchttpclient;
Copy link
Contributor

@slandelle slandelle Sep 27, 2016

Choose a reason for hiding this comment

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

missing file header

Diagoras reacted with thumbs up emoji
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

still missing

Diagoras reacted with thumbs up emoji
Copy link
Contributor

Thanks!
Please address comments and ping once you're done.


/**
* Created by grenville on 9/24/16.
*/
Copy link
Contributor

@slandelle slandelle Oct 5, 2016

Choose a reason for hiding this comment

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

please remove

Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

javadoc

Copy link
Contributor

@Diagoras gentle ping

@slandelle slandelle force-pushed the master branch 3 times, most recently from f0b7cde to b398e21 Compare October 5, 2016 13:38
Copy link
Contributor Author

Diagoras commented Oct 5, 2016

@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.
*/
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

javadoc

this.idleConnectionCount = idleConnectionCount;
}

public long getTotalConnectionCount() {
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

javadoc

*/
public class ClientStats {

private final long totalConnectionCount;
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

I don't think it's worth storing


public ClientStats(final long activeConnectionCount,
final long idleConnectionCount) {
this.totalConnectionCount = activeConnectionCount + idleConnectionCount;
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

not worth storing

}

public long getTotalConnectionCount() {
return totalConnectionCount;
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

return sum here instead

}

public ClientStats getClientStats() {
final ChannelManager channelManager = requestSender.getChannelManager();
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

You don't need that, DefaultAsyncHttpClient has the ChannelManager.

Channels.setAttribute(channel, newExecuteNextRequestCallback(future, nextRequest));
}

public ChannelManager getChannelManager() {
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

Useless, DefaultAsyncHttpClient has the ChannelManager.

return eventLoopGroup;
}

public ChannelGroup getOpenChannels() {
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

No, this shouldn't leak

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();
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

It should be possible to only scan once, not twice.

Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

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.";
}
Copy link
Contributor

@slandelle slandelle Oct 6, 2016

Choose a reason for hiding this comment

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

implement equals and hashcode

Copy link
Contributor

No pro, thanks!

Copy link
Contributor Author

Diagoras commented Oct 9, 2016

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.

Copy link
Contributor

No hurries

Copy link
Contributor Author

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();
Copy link
Contributor Author

@Diagoras Diagoras Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

Given that the ChannelManager can be set in the config

No, it can't.

Diagoras reacted with thumbs up emoji
Copy link
Contributor Author

@Diagoras Diagoras Nov 2, 2016

Choose a reason for hiding this comment

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

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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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(
Copy link
Contributor Author

@Diagoras Diagoras Oct 31, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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().

Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

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.

Diagoras reacted with thumbs up emoji
@@ -0,0 +1,78 @@
/*
* Copyright 2010 Ning, Inc.
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

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

Diagoras reacted with thumbs up emoji
}

public ClientStats getClientStats() {
final long totalConnectionCount = openChannels.size();
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

Given that the ChannelManager can be set in the config

No, it can't.

Diagoras reacted with thumbs up emoji

@Override
public long getIdleChannelCount() {
return partitions.reduceValuesToLong(
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

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.

Diagoras reacted with thumbs up emoji
@@ -0,0 +1,56 @@
package org.asynchttpclient;
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

still missing

Diagoras reacted with thumbs up emoji
private final long idleConnectionCount;

public ClientStats(final long activeConnectionCount,
final long idleConnectionCount) {
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

don't really need final here

Copy link
Contributor Author

@Diagoras Diagoras Nov 2, 2016

Choose a reason for hiding this comment

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

I always try to make my references immutable by default, but I can change it.

Copy link
Contributor

@slandelle slandelle Nov 2, 2016

Choose a reason for hiding this comment

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

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)...

Diagoras reacted with thumbs up emoji
Copy link
Contributor Author

@Diagoras Diagoras Nov 3, 2016

Choose a reason for hiding this comment

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

You can say that again.


@Override
public ClientStats getClientStats() {
return null;
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

throw new UnsupportedOperationException

Diagoras reacted with thumbs up emoji

@Override
public ClientStats getClientStats() {
return null;
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

throw new UnsupportedOperationException

Diagoras reacted with thumbs up emoji
assertEquals(emptyStats.getTotalConnectionCount(), 0);

final List<ListenableFuture<Response>> futures = new ArrayList<>();
for (int i = 0; i < 5; i++) {
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

stream

// 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++) {
Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

stream

return 0;
}


Copy link
Contributor

@slandelle slandelle Oct 31, 2016

Choose a reason for hiding this comment

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

kill empty lines

Diagoras reacted with thumbs up emoji
Copy link
Contributor

Hey! Thanks for updating! Sorry, I still have some comments...
Diagoras reacted with thumbs up emoji

*
* @return a {@link ClientStats}
*/
ClientStats getClientStats();
Copy link
Contributor

@slandelle slandelle Nov 1, 2016

Choose a reason for hiding this comment

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

Let's aim for AHC 2.1 so we can change this API.

Diagoras reacted with thumbs up emoji
Copy link
Contributor Author

@Diagoras Diagoras Nov 2, 2016

Choose a reason for hiding this comment

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

Then I should reopen this PR against branch "2.1", right?

Copy link
Contributor

@slandelle slandelle Nov 2, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Diagoras commented Nov 6, 2016

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);
Copy link
Contributor Author

@Diagoras Diagoras Nov 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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() {
Copy link
Contributor Author

@Diagoras Diagoras Nov 6, 2016

Choose a reason for hiding this comment

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

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() {
Copy link
Contributor Author

@Diagoras Diagoras Nov 6, 2016

Choose a reason for hiding this comment

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

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)
Copy link
Contributor Author

@Diagoras Diagoras Nov 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@slandelle slandelle Nov 6, 2016

Choose a reason for hiding this comment

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

I think it's pretty safe to cast

Copy link
Contributor Author

@Diagoras Diagoras Nov 6, 2016

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

@slandelle slandelle Nov 6, 2016

Choose a reason for hiding this comment

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

I think it's pretty safe to cast

Copy link
Contributor Author

Diagoras commented Nov 7, 2016

So, are we good to merge? And if so, should I squash my commits?

Copy link
Contributor

I hope to be able to review your latest changes in length this week.
As they break the API, they'll have to target AHC 2.1. I plan on switching master next week.
Thanks for your patience.
Diagoras reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slandelle slandelle slandelle requested changes

Assignees

No one assigned

Projects

None yet

Milestone

2.1.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /