Redis command listener #1972
Redis command listener #1972
Conversation
|
@gkorland , any update here? I believe I've addressed all your concerns. |
|
gr8routdoors did you check my latest comments? |
|
@gkorland which comments? AFAICT they've all been addressed. |
|
Any update here @gkorland ? |
|
Hey, @gkorland! Would you be able to take a look at the PR? I believe it's almost there, just need to agree on some minor points :) |
|
Hey, @gkorland, @sazzad16! Would you be able to take a look? There is clearly a need for this, #1791 (comment). |
|
@gr8routdoors can you please rebase the PR |
|
OK, I've updated to the latest from master @gkorland |
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
|
|
||
| public class ConnectionTest { |
There was a problem hiding this comment.
Connection operation or connection methods are not going to be tested. Move the test methods in a new test class.
| Protocol.DEFAULT_TIMEOUT, Protocol.DEFAULT_TIMEOUT, null, sslSocketFactory, sslParameters, | ||
| hostnameVerifier, listeners), new GenericObjectPoolConfig()); |
| Protocol.DEFAULT_PORT, Protocol.DEFAULT_TIMEOUT, Protocol.DEFAULT_TIMEOUT, null, | ||
| Protocol.DEFAULT_DATABASE, null, false, null, null, null, | ||
| listeners), new GenericObjectPoolConfig()); |
| } | ||
| } | ||
|
|
||
|
|
|
|
||
| JedisFactory(final URI uri, final int connectionTimeout, final int soTimeout, | ||
| final String clientName, final SSLSocketFactory sslSocketFactory, | ||
| final SSLParameters sslParameters, final HostnameVerifier hostnameVerifier) { |
There was a problem hiding this comment.
This breaks backward compatibility. Create a new constructor.
| public void setListeners(List<JedisCommandListener> listeners) { | ||
| if (listeners == null) { | ||
| if (!this.listeners.isEmpty()) { // No reason to re-create an already empty list | ||
| this.listeners = new LinkedList<>(); |
| } | ||
|
|
||
| public void addListener(JedisCommandListener listener) { | ||
| this.listeners.add(listener); |
There was a problem hiding this comment.
We need to think if we don't need to protect here from concurrency issues, especially when use in a pool
|
|
||
| public void sendCommand(final ProtocolCommand cmd, final byte[]... args) { | ||
| try { | ||
| notifyCommandStarted(cmd, args); |
There was a problem hiding this comment.
should we have more "lightweight" solution that will not add this overhead in case no listeners were set.
4 participants
This builds on #1791 with the following:
JedisCommandListener.commandConnected()to allow tracking of connection time metrics separately from command execution timeMore details from the PR #1791 description:
As per https://groups.google.com/forum/?fromgroups#!topic/jedis_redis/UaJubt42f_0,
This PR adds a command listener that allows for
sane integration with tracing libraries like http://opentracing.io/
capturing metrics and integration with metric libraries like http://metrics.dropwizard.io/4.0.0/
integration with logging libraries
None of the public signatures was changed, so the change should be fully backwards compatible.