finagle-redis: Add the RedisTracingFilter and RedisLoggingFilter back into the default Redis Stack

[rpless]

Problem

I was experimenting with the Tracing module and instrumented a Redis Client. I noticed that Redis rpc calls did not get named unlike the Http calls that were being traced. After some discussion on gitter, @mosesn pointed out that a commit where the RedisLoggingFilter and RedisTracingFilter had been ported during the move from Netty 3 -> Netty 4, but had not been added back to the stack.

Solution

I added the RedisLoggingFilter and the RedisTracingFilter to the default client stack.

Result

This will add stats and tracing back to the Redis Client by default.

I followed the mysql client for adding the role and module definitions. If there as a better way to do this let me know and I can change it.

[CLAassistant]

All committers have signed the CLA.

[codecov-commenter]

Codecov Report

Merging #866 into develop will increase coverage by 0.01%. The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           develop     #866      +/-   ##
===========================================
+ Coverage    78.19%   78.20%   +0.01%     
===========================================
  Files          829      829              
  Lines        24452    24468      +16     
  Branches      1580     1584       +4     
===========================================
+ Hits         19120    19135      +15     
- Misses        5332     5333       +1     

Impacted Files	Coverage Δ
...tter/finagle/redis/filter/RedisLoggingFilter.scala	33.33% <62.50%> (+33.33%)	:arrow_up:
...tter/finagle/redis/filter/RedisTracingFilter.scala	38.46% <83.33%> (+38.46%)	:arrow_up:
...dis/src/main/scala/com/twitter/finagle/Redis.scala	32.65% <100.00%> (+2.86%)	:arrow_up:
.../com/twitter/finagle/tracing/BroadcastTracer.scala	60.41% <0.00%> (-10.42%)	:arrow_down:
...om/twitter/finagle/dispatch/ServerDispatcher.scala	85.10% <0.00%> (+2.12%)	:arrow_up:
...2/transport/common/Http2StreamMessageHandler.scala	94.87% <0.00%> (+2.56%)	:arrow_up:
...r/finagle/dispatch/GenSerialClientDispatcher.scala	82.75% <0.00%> (+3.44%)	:arrow_up:
...rverset2/client/apache/ApacheKeeperException.scala	14.81% <0.00%> (+3.70%)	:arrow_up:
...m/twitter/finagle/partitioning/zk/ZkMetadata.scala	95.83% <0.00%> (+4.16%)	:arrow_up:
.../http/exp/GenStreamingSerialServerDispatcher.scala	77.08% <0.00%> (+6.25%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c9107f6...d45d770. Read the comment docs.

[rpless]

Realized I pushed with an old work account which is why the CLA wasn't showing up as signed. Fixed by pushing the same commit over the original one but associated with the correct email.

[jyanJing]

Hi @rpless , @enbnt and I have been having troubles pulling in this request, we both see the following error:

error: while searching for:
import com.twitter.util.{Duration, Monitor}
import java.net.SocketAddress

import com.twitter.finagle.redis.param.{Database, Password}

trait RedisRichClient { self: Client[Command, Reply] =>

error: patch failed: finagle/finagle-redis/src/main/scala/com/twitter/finagle/Redis.scala:23
error: finagle/finagle-redis/src/main/scala/com/twitter/finagle/Redis.scala: patch does not apply
Checking patch finagle/finagle-redis/src/main/scala/com/twitter/finagle/redis/filter/RedisLoggingFilter.scala...
Checking patch finagle/finagle-redis/src/main/scala/com/twitter/finagle/redis/filter/RedisTracingFilter.scala...
Checking patch finagle/finagle-redis/src/main/scala/com/twitter/finagle/redis/filter/RedisLoggingFilter.scala...
Checking patch finagle/finagle-redis/src/main/scala/com/twitter/finagle/Redis.scala...
error: while searching for:
      .insertBefore(DefaultPool.Role, RedisPool.module)
      .insertAfter(StackClient.Role.prepConn, ConnectionInitCommand.module)
      .replace(StackClient.Role.protoTracing, RedisTracingFilter.module)
      .insertAfter(RedisLoggingFilter.role, RedisLoggingFilter.module)

    private[finagle] val hashRingStack: Stack[ServiceFactory[Command, Reply]] =
      stack.insertAfter(BindingFactory.role, RedisPartitioningService.module)

error: patch failed: finagle/finagle-redis/src/main/scala/com/twitter/finagle/Redis.scala:74
error: finagle/finagle-redis/src/main/scala/com/twitter/finagle/Redis.scala: patch does not apply

@enbnt has tried to do a merge on Github but the error persists.

Would you please do a rebase then I can try to pull it again? Thank you!

[rpless]

Hi @jyanJing I believe that happens because one of the commits in this PR conflicts with this commit in develop. I have rebased this Pull Request against develop. Please let me know if there are any issues.

[jyanJing]

Hi @rpless,

Thank you for rebasing the branch! I am able to pull it in and get the team's review. Since this change is made to the stack module with the tracing and logging filter, we are very careful with changes in that area because it might bring up latencies for services that use finagle-redis. So I am working with 2 other teams to canary the change now, to make sure we capture any behavior changes. It is not about your change, it is a critical area to be precautious about.

I wanted to let you know that it will take longer to merge this change, but I am actively working on it! Really appreciate for making this change!

Best regards, Jing

[jyanJing]

Hi @rpless

canaries looks good, the change is merged,

Thank you for the excellent work and your patience!

[rpless]

Thanks for setting up the canary to test the change @jyanJing!