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

rpless · 2020-07-17T13:42:13Z

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 · 2020-07-17T13:42:17Z

All committers have signed the CLA.

codecov-commenter · 2020-07-17T13:48:56Z

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%)`	⬆️
...tter/finagle/redis/filter/RedisTracingFilter.scala	`38.46% <83.33%> (+38.46%)`	⬆️
...dis/src/main/scala/com/twitter/finagle/Redis.scala	`32.65% <100.00%> (+2.86%)`	⬆️
.../com/twitter/finagle/tracing/BroadcastTracer.scala	`60.41% <0.00%> (-10.42%)`	⬇️
...om/twitter/finagle/dispatch/ServerDispatcher.scala	`85.10% <0.00%> (+2.12%)`	⬆️
...2/transport/common/Http2StreamMessageHandler.scala	`94.87% <0.00%> (+2.56%)`	⬆️
...r/finagle/dispatch/GenSerialClientDispatcher.scala	`82.75% <0.00%> (+3.44%)`	⬆️
...rverset2/client/apache/ApacheKeeperException.scala	`14.81% <0.00%> (+3.70%)`	⬆️
...m/twitter/finagle/partitioning/zk/ZkMetadata.scala	`95.83% <0.00%> (+4.16%)`	⬆️
.../http/exp/GenStreamingSerialServerDispatcher.scala	`77.08% <0.00%> (+6.25%)`	⬆️

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 · 2020-07-18T15:01:41Z

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.

bryce-anderson · 2020-08-19T20:59:06Z

finagle-redis/src/main/scala/com/twitter/finagle/redis/filter/RedisLoggingFilter.scala

+        if (statsParam.statsReceiver.isNull)
+          next
+        else
+          new RedisLoggingFilter(statsParam.statsReceiver.scope("http")).andThen(next)


I suspect the scope isn't really supposed to be "http".

Ah yes this looks copy-paste on my part, the original commit does not seem to add a scope. I can remove it. Alternatively some modules seem to scope the receiver to the protocol name like http so I could change it to that if it makes sense.

I suspect we want to follow what was originally there. It would normally be the job of whoever passes the stats receiver to us to define the scope, and if we need to go deeper this module would be in charge of naming that scope and passing it on.

hamdiallam · 2020-08-27T15:44:58Z

finagle-redis/src/main/scala/com/twitter/finagle/Redis.scala

@@ -72,6 +73,8 @@ object Redis extends Client[Command, Reply] with RedisRichClient {
    private val stack: Stack[ServiceFactory[Command, Reply]] = StackClient.newStack
      .insertBefore(DefaultPool.Role, RedisPool.module)
      .insertAfter(StackClient.Role.prepConn, ConnectionInitCommand.module)
+      .replace(StackClient.Role.protoTracing, RedisTracingFilter.module)
+      .insertAfter(RedisLoggingFilter.role, RedisLoggingFilter.module)


Does the RedisLoggingFilter.role exist in the client stack already? I believe using insertAfter here will not put it into the stack, since the role doesn't exists. Maybe before the tracing filter?

Ah yes, the role does not currently exist in the stack. Docs from insertAfter say: If no elements match the role, then an unmodified stack is returned. So looks like this is currently won't add the logging filter. I'll change it to insert before the tracing filter.

jyanJing · 2020-09-30T22:24:13Z

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!

… into the default Redis Stack

rpless · 2020-09-30T23:51:18Z

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 · 2020-10-01T20:38:22Z

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 · 2020-10-22T05:38:29Z

Hi @rpless

canaries looks good, the change is merged,

Thank you for the excellent work and your patience!

rpless · 2020-10-24T19:54:36Z

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

rpless force-pushed the redis-tracing branch from 868d0eb to 6cd51c6 Compare July 18, 2020 15:00

bryce-anderson suggested changes Aug 19, 2020

View reviewed changes

hamdiallam reviewed Aug 27, 2020

View reviewed changes

mosesn approved these changes Sep 9, 2020

View reviewed changes

bryce-anderson approved these changes Sep 9, 2020

View reviewed changes

rpless added 3 commits September 30, 2020 19:46

finagle-redis: Add the RedisTracingFilter and RedisLoggingFilter back…

1d84348

… into the default Redis Stack

Remove extra scope from the RedisLoggingFilter

0906afe

Insert the RedisLogginFilter before the tracing module

d45d770

rpless force-pushed the redis-tracing branch from bbfc453 to d45d770 Compare September 30, 2020 23:49

jyanJing closed this Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rpless commented Jul 17, 2020

CLAassistant commented Jul 17, 2020 •

edited

codecov-commenter commented Jul 17, 2020 •

edited

rpless commented Jul 18, 2020

bryce-anderson Aug 19, 2020

rpless Aug 20, 2020

bryce-anderson Aug 21, 2020

hamdiallam Aug 27, 2020

rpless Aug 29, 2020

jyanJing commented Sep 30, 2020

rpless commented Sep 30, 2020

jyanJing commented Oct 1, 2020

jyanJing commented Oct 22, 2020

rpless commented Oct 24, 2020

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

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

Conversation

rpless commented Jul 17, 2020

CLAassistant commented Jul 17, 2020 • edited

codecov-commenter commented Jul 17, 2020 • edited

Codecov Report

rpless commented Jul 18, 2020

bryce-anderson Aug 19, 2020

Choose a reason for hiding this comment

rpless Aug 20, 2020

Choose a reason for hiding this comment

bryce-anderson Aug 21, 2020

Choose a reason for hiding this comment

hamdiallam Aug 27, 2020

Choose a reason for hiding this comment

rpless Aug 29, 2020

Choose a reason for hiding this comment

jyanJing commented Sep 30, 2020

rpless commented Sep 30, 2020

jyanJing commented Oct 1, 2020

jyanJing commented Oct 22, 2020

rpless commented Oct 24, 2020

CLAassistant commented Jul 17, 2020 •

edited

codecov-commenter commented Jul 17, 2020 •

edited