Metrics Enhancement #471
Metrics Enhancement #471
Conversation
zhenlineo
force-pushed
the
1.6-metrics-enhance
branch
from
f10aad4 to
f0f8dd7
Compare
zhenlineo
force-pushed
the
1.6-metrics-enhance
branch
from
f0f8dd7 to
16a1b06
Compare
…nnection pool metrics Change the histogram only record value generated by successful requests.
…for a connection from the pool. Fixed the bug where the pool status was not showing the correct status of each small pool.
zhenlineo
force-pushed
the
1.6-metrics-enhance
branch
from
eb89a5e to
42cfee8
Compare
lutovich
left a comment
lutovich
left a comment
There was a problem hiding this comment.
@zhenlineo changes look good. Made couple comments.
| log.debug( "Channel %s created", channel ); | ||
| incrementInUse( channel ); | ||
| metricsListener.afterCreated( serverAddress( channel ) ); | ||
| throw new IllegalStateException( "A fatal error happened in this driver as this method should never be called. " + |
There was a problem hiding this comment.
Maybe just throw new UnsupportedOperationException()? Current message reminds me of ThisShouldNotHappenError :)
|
|
||
| long elapsed(); | ||
|
|
||
| interface PoolListenerEvent extends ListenerEvent |
There was a problem hiding this comment.
Why are these empty interfaces needed? Can't ListenerEvent be used everywhere?
There was a problem hiding this comment.
For
MetricsListener#afterAcquiredOrCreated( BoltServerAddress serverAddress, PoolListenerEvent acquireEvent );
and
MetricsListener#void afterAcquiredOrCreated( BoltServerAddress serverAddress, ConnectionListenerEvent acquireEvent );
It is also more clear API to use.
There was a problem hiding this comment.
ListenerEvent.PoolListenerEvent and ListenerEvent.ConnectionListenerEvent are the same as ListenerEvent. I do not really see the reason for having them right now. They only force us to type more :)
| Open( 0 ), | ||
| Closed( 1 ); | ||
|
|
||
| private final int value; |
There was a problem hiding this comment.
Can this field be removed? It's really up to the users how to interpret the returned status
There was a problem hiding this comment.
But I kind of do not want to have a if-else just for this conversion when publishing the result.
metrics.add( new Metric( path( serverName, "poolStatus" ), metric.poolStatus().value(), date ) );
vs.
PoolStatus staus = metric.poolStatus()
int statusNum = -1;
if( status == PoolStatus.OPEN)
{
statusNum = 0;
}
else if( status == PoolStatus.CLOSED)
{
statusNum = 1;
}
metrics.add( new Metric( path( serverName, "poolStatus"), statusNum, date ) );
I feel for a metrics collector, it does not matter what value you assigned to each PoolStatus.
It is more important to record all possible PoolStatus. I mean it would be good to know the real value, but it will not be a big issue if a user does not know as long as they see they are different. So I thought it would be helpful to have a simple value to use directly.
| public enum PoolStatus | ||
| { | ||
| Open, Closed | ||
| Open( 0 ), |
There was a problem hiding this comment.
I'd vote for renaming enum values to OPEN and CLOSED which follows the recommended java code style
zhenlineo
force-pushed
the
1.6-metrics-enhance
branch
from
c98deb3 to
bef44eb
Compare
Change to unmodified map to return pool and connection metrics to users
zhenlineo
force-pushed
the
1.6-metrics-enhance
branch
from
bef44eb to
c889fa1
Compare
2 participants
Based on #470
Added connection
acquiring,acquiredcount andtimedOutToAcquirecount on connection pool metricsChange the histogram only record value generated by successful requests.
Fixed a bug where the pool status was showing the big pool rather than each small pool status.