Use finagle.StatsFilter to add request latencies

[muuki88]

Using the a finagle.StatsFilter per method would add request latencies for each endpoint.

Expected behavior

I would like to have request latencies in my stats per method.

Actual behavior

Only success and failure counts are available

Steps to reproduce the behavior

The service.mustache file (https://github.com/twitter/scrooge/blob/2f5fc458852f7f0b8797f3dba6972cf6f65b45f8/scrooge-generator/src/main/resources/scalagen/service.mustache#L677) defines the perMethodStatsFilter.

[ryanoneill]

Hi @muuki88, sorry for the lack of response here.

For servers, this is something that we have, but only for users of Finatra. It writes these metrics via its StatsFilter. I get though that it doesn't help for folks who aren't using Finatra.

With that said, measuring latency isn't something that depends on the internal details of how a method works, nor the parameters of the method itself, so it's a prime example of something which should instead be calculated in a Filter, which is what Finatra does.

Considering this, it's unlikely that we'd add this to Scrooge generated code, considering that it would then be additional code that needs to be compiled every time scrooge generates code for a service. For this reason, we're actually looking to cut back further on the code being generated by Scrooge in order to improve overall compile times at Twitter.

[muuki88]

Thanks @ryanoneill for the explanation. My suggestion was to reuse an already existing Filter implementation so this shouldn't add too much generated. The filter we are currently adding instead of using the perMethodStatsFilter is something like this

class StatsPerMethodFilter(serviceName: String, thriftStats: StatsReceiver)
    extends Filter.TypeAgnostic {

  override def toFilter[Req, Rep]: Filter[Req, Rep, Req, Rep] = Filter.mk { (request, service) =>
    val noopFilter = Filter.mk[Req, Rep, Req, Rep]((request, service) => service(request))
    val methodFilter = new ConcurrentHashMap[String, Filter[Req, Rep, Req, Rep]]

    val statsFilter = MethodMetadata.current
      .map { meta =>
        methodFilter
          .computeIfAbsent(
            meta.methodName,
            methodName =>
              StatsFilter
                .typeAgnostic(thriftStats.scope(serviceName, methodName), StatsFilter.DefaultExceptions)
                .toFilter[Req, Rep]
          )
      }
      .getOrElse(noopFilter)

    statsFilter(request, Service.mk(service))
  }
}

I'm not sure if this is the most robust or perfomant way to implement, but it does the job :)