measure the entire Jedis roundtrip time, fixes #1082 #1117
Conversation
|
|
||
| class JedisInstrumentation extends InstrumentationBuilder { | ||
| onType("redis.clients.jedis.Protocol") | ||
| .advise(method("sendCommand").and(isPublic[MethodDescription]), classOf[SendCommandAdvice]) | ||
| onType("redis.clients.jedis.Jedis") |
There was a problem hiding this comment.
redis.clients.jedis.Jedis is only one of the implementations of redis.clients.jedis.JedisCommands. I guess we should add tests for the other classes...
There was a problem hiding this comment.
I noticed that there is Jedis and ShardedJedis, but the sharded version uses a regular Jedis under the hood so it should work fine. There are other variants too, like JedisCluster and in 4.x there is a JedisPool too. I'll leave all of those for a separate PR.
There was a problem hiding this comment.
Follow up for this: #1121
|
I managed to test it locally and add a workaround for this to work with Jedis 4.1, at least at a very basic level. We still need to figure out support for some 4.x-only components (see #1121 for follow ups). This is ready to review. |
...umentation/kamon-redis/src/main/scala/kamon/instrumentation/jedis/JedisInstrumentation.scala
Outdated
Show resolved
Hide resolved
ivantopo
changed the title
Turns out that our current Jedis instrumentation is only measuring the time it takes to send a command to Redis, without taking into account how long will it take to get an answer back. This PR fixes that. Thanks to @jtjeferreira for bringing this up!
TODO:
toString,hashCode, andcloseare ignored)