Added `withHttpStats` config to Http.[Server|Client]. #557

spockz · 2016-09-22T19:56:41Z

Problem

com.twitter.finagle.http.filter.StatsFilter Is missing a corresponding module
so it cannot be conveniently added to a Stack.

Solution

Added the module, added the withHttpStats methods to Client and Server.

Result

It is now possible to effortlessly enable Http specific statistics on both
http servers and clients.

Problem com.twitter.finagle.http.filter.StatsFilter Is missing a corresponding module so it cannot be conveniently added to a Stack. Solution Added the module, added the `enableHttpStats` methods to Client and Server. Result It is now possible to effortlessly enable Http specific statistics on both http servers and clients.

kevinoliver

Looks good. Only a few small changes requested.

1 other thing we've tried to get better about is documenting metrics. Can you cover these in the HTTP section of Metrics.rst?

kevinoliver · 2016-09-22T21:01:36Z

finagle-http/src/main/scala/com/twitter/finagle/Http.scala

+    /**
+     * Enable the collection of HTTP specific metrics. See [[http.filter.StatsFilter]].
+     */
+    def enableHttpStats(): Client =


did you consider withHttpStats as a name? cc @vkostyukov (here and below)

Fair enough. I choose enable because there are no arguments passed in. Just like noFailFast.

There is noFailFast on Client/Server (it's a client builder thing). We use with-prefixed methods here instead. And it's totally fine to have with-API w/o an argument (we do that all the time). See Design Principles for the with-API (item number 2).

Also, please no ().

kevinoliver · 2016-09-22T21:02:33Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

+import com.twitter.util._
+
+object StatsFilter {
+  val role = Stack.Role("HttpStatsFilter")


i know we haven't always done a good job in the past, but i'd like to have public methods and members have type annotations.
val role: Stack.Role = ...

kevinoliver · 2016-09-22T21:03:03Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

+  def module[Req <: Request]: Stackable[ServiceFactory[Req, Response]] =
+    new Stack.Module1[param.Stats, ServiceFactory[Req, Response]] {
+      val role = StatsFilter.role
+      val description = "Register HTTP Stats"


omit "Register"?

There are other descriptions of modules containing verbs. Why should it be gone here?

i guess we aren't consistent. lets leave as is.

kevinoliver · 2016-09-22T21:03:36Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

@@ -29,7 +49,7 @@ class StatsFilter[REQUEST <: Request](stats: StatsReceiver)
    future respond {
      case Return(response) =>
        count(elapsed(), response)
-      case Throw(_) =>
+      case Throw(_)         =>


lets leave these hashrockets unaligned

kevinoliver · 2016-09-22T21:06:31Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

+      val role = StatsFilter.role
+      val description = "Register HTTP Stats"
+
+      override def make(statsParam: param.Stats,


omit override.

mosesn · 2016-09-22T21:08:39Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

+      val description = "Register HTTP Stats"
+
+      override def make(statsParam: param.Stats,
+                        next: ServiceFactory[Req, Response]): ServiceFactory[Req, Response] = {


just two spaces please.

mosesn · 2016-09-22T21:09:14Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

+
+      override def make(statsParam: param.Stats,
+                        next: ServiceFactory[Req, Response]): ServiceFactory[Req, Response] = {
+        if (statsParam.statsReceiver.isNull) {


we typically drop braces for single exprs.

mosesn · 2016-09-22T21:12:35Z

finagle-http/src/main/scala/com/twitter/finagle/Http.scala

+     * Enable the collection of HTTP specific metrics. See [[http.filter.StatsFilter]].
+     */
+    def enableHttpStats(): Server =
+      withStack(http.filter.StatsFilter.module +: stack)


I don't think we want to have non-idempotent configuration methods on StackClients. How about instead inserting a nop "http stats" module into the Http stack and then replacing it here?

It might also be useful to look at our design principles for configuration methods for StackClients.

http://twitter.github.io/finagle/guide/Configuration.html#design-principles

Good point. I'll also have a look at those principles.

vkostyukov · 2016-09-23T17:14:00Z

finagle-http/src/main/scala/com/twitter/finagle/Http.scala

+    /**
+     * Enable the collection of HTTP specific metrics. See [[http.filter.StatsFilter]].
+     */
+    def enableHttpStats(): Client =


There is noFailFast on Client/Server (it's a client builder thing). We use with-prefixed methods here instead. And it's totally fine to have with-API w/o an argument (we do that all the time). See Design Principles for the with-API (item number 2).

vkostyukov · 2016-09-23T17:14:21Z

finagle-http/src/main/scala/com/twitter/finagle/Http.scala

+    /**
+     * Enable the collection of HTTP specific metrics. See [[http.filter.StatsFilter]].
+     */
+    def enableHttpStats(): Client =


Also, please no ().

vkostyukov · 2016-09-23T17:16:32Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

+object StatsFilter {
+  val role = Stack.Role("HttpStatsFilter")
+
+  def module[Req <: Request]: Stackable[ServiceFactory[Req, Response]] =


I honestly don't think we still need this Req <: Request type-param. I think we've done this before to support something we called "rich http" (which is now just finagle-http). Please, correct me if I'm wrong. /cc @mosesn @dschobel @roanta

@vkostyukov I think you're right we don't need it here.

I've put the type-param in there because the filter itself has it as well and limiting it in the module seemed strange. Additionally, our applications use types that inherit from RequestProxy so we actually like the presence of the type-param in the filter and think the api would be less rich and flexible without it.

* enableHttpStats => withHttpStats * withHttpStats is now idempotent * API fixes

kevinoliver · 2016-09-26T16:17:54Z

doc/src/sphinx/Metrics.rst

@@ -195,6 +195,21 @@ These stats pertain to the HTTP protocol.
  A counter of the number of non-retryable HTTP 503 responses the Http server returns. Those
  responses are not automatically retried.

+**status/<statusCode>**


lets add a sub heading explaining that they come from this Filter.

kevinoliver · 2016-09-26T16:19:14Z

doc/src/sphinx/Metrics.rst

+  count as 5XX for this counter.
+
+**time/<statusCode>**
+  Metric on duration per Http status code.


for consistency with the rest of the file:
s/Metric/A histogram/

kevinoliver · 2016-09-26T16:19:19Z

doc/src/sphinx/Metrics.rst

+  Metric on duration per Http status code.
+
+**time/<statusCategory>**
+  Metric on duration per Http status code category.


kevinoliver · 2016-09-26T16:20:01Z

finagle-core/src/main/scala/com/twitter/finagle/Stack.scala

@@ -455,6 +455,11 @@ object Stack {
      Node(this, (prms, next) => Leaf(this, make(next.make(prms))), next)
  }

+  class NoOpModule[T](val role: Role, val description: String) extends Module0[T] {
+    override def make(next: T): T =


nit: we omit override for abstract methods

kevinoliver · 2016-09-26T16:21:27Z

finagle-http/src/main/scala/com/twitter/finagle/Http.scala

+    /**
+     * Enable the collection of HTTP specific metrics. See [[http.filter.StatsFilter]].
+     */
+    def withHttpStats: Client =


lets add a basic unit test that verifies these work as expected. something like a request populates at least one of the stats. same for the server side.

kevinoliver

looks good. a few small changes plus a test.

spockz · 2016-09-26T21:03:42Z

So, I've added a test-case. It also shows that the stats are directly under the client-label. Should we scope it to http? This would probably be a backwards incompatible change, unless we make it so that it is only prefixed with http when using withHttpStats and not when newing the StatsFilter manually.

kevinoliver · 2016-09-26T23:00:00Z

doc/src/sphinx/Metrics.rst

@@ -204,10 +208,10 @@ These stats pertain to the HTTP protocol.
  count as 5XX for this counter.

 **time/<statusCode>**
-  Metric on duration per Http status code.
+  A histogram on duration per Http status code.


sorry, should've noticed previously, please add that these two histograms are in milliseconds.

kevinoliver

one small request, and then lgtm.

vkostyukov · 2016-09-26T23:31:12Z

doc/src/sphinx/Metrics.rst

+  count as 5XX for this counter.
+
+**time/<statusCode>**
+  A histogram on duration per Http status code.


Please, capitalize HTTP.

vkostyukov · 2016-09-26T23:31:52Z

doc/src/sphinx/Metrics.rst

+  A histogram on duration per Http status code.
+
+**time/<statusCategory>**
+  A histogram on duration per Http status code category.


Please, capitalize HTTP.

vkostyukov · 2016-09-26T23:34:35Z

finagle-core/src/main/scala/com/twitter/finagle/Stack.scala

@@ -455,6 +455,11 @@ object Stack {
      Node(this, (prms, next) => Leaf(this, make(next.make(prms))), next)
  }

+  class NoOpModule[T](val role: Role, val description: String) extends Module0[T] {


I think you can avoid defining a new type by just passing an identity function as a stack module - it should be implicitly converted. We do that in a bunch of places.

Identity is only used in places where stack.replace() is used, not with prepend(), there is no prepend that takes both a role and a function. How do you recommend to do this?

I think you should be fine. There is an implicit conversion from A => A function to Stackable[A].

I'm not seeing it. How would we assign a role to the identity function?

vkostyukov · 2016-09-26T23:43:17Z

finagle-http/src/main/scala/com/twitter/finagle/http/filter/StatsFilter.scala

+  val role: Stack.Role = Stack.Role("HttpStatsFilter")
+  val description: String = "HTTP Stats"
+
+  def module[Req <: Request]: Stackable[ServiceFactory[Req, Response]] =


I still don't think you need this type-param.

Please, note that it has nothing to do with your custom HTTP types (either for request or response) since at that point they should be lifted to something Finagle can work with (i.e., c.t.f.http.Request and c.t.f.http.Response).

By the same reason, StatsFilter doesn't need a type param Req, but we can't remove it yet (an API break) so let's keep it (filter) as is, but remove the type-param on a module.

Ok. I'll remove it.

codecov-io · 2016-09-27T00:56:41Z

Current coverage is 68.24% (diff: 75.00%)

Merging #557 into develop will increase coverage by 1.06%

@@            develop       #557   diff @@
==========================================
  Files           566        592    +26   
  Lines         20506      21172   +666   
  Methods       18988      19568   +580   
  Messages          0          0          
  Branches       1335       1444   +109   
==========================================
+ Hits          13775      14448   +673   
+ Misses         6731       6724     -7   
  Partials          0          0

Powered by Codecov. Last update 00dc4cc...dae164b

spockz · 2016-09-27T20:12:20Z

Should we scope all the stats to start with 'http'? Otherwise we clutter the stats a bit, this will give nice grouping.

vkostyukov · 2016-09-27T21:59:10Z

@spockz I don't know about scoping. On one hand, it's reasonable and that's what we do for mux (having a mux scope). On the other hand, we don't scope for HTTP and we already have some HTTP-specific metrics. So I prefer us to be consistent and either

Do not scope new metrics
Scope new metrics and move old metrics under http/

It's quite clear that it's hard to go the second road here since it breaks the API (will likely break both internal and external dashboards). So I vote for 1.

kevinoliver · 2016-09-27T22:07:49Z

@vkostyukov "It's quite clear that it's hard to go the second road here since it breaks the API (will likely break both internal and external dashboards). So I vote for 1."

This could be done for only users of the module which is new and therefore not a breaking change.

I kinda like the scoping under http, but don't feel strongly.

spockz · 2016-09-28T18:23:26Z

I went the route @kevinoliver mentioned. The http.filter.StatsFilter itself doesn't add the http scope but the module does.

* Add http scope for StatsFilter added through module; * Just Request

kevinoliver · 2016-09-28T18:37:48Z

lgtm!

spockz · 2016-10-07T04:12:48Z

@kevinoliver, do you have any news on this?

mosesn · 2016-10-07T05:49:22Z

@spockz we're still working on merging it internally. We're hoping to get it in tomorrow. Thanks for checking in!

jcrossley · 2016-10-07T18:32:58Z

Hey @spockz ! I tried to merge in your request but ran into a couple small issues. Fixing those up now and will have it in by the end of the day, thanks!

jcrossley · 2016-10-07T19:56:47Z

All done, @spockz ! This has been merged internally and will go out with the next release :)

spockz · 2016-10-07T20:13:29Z

Great, Thank you @jcrossley!

mosesn · 2016-10-11T20:50:02Z

made it to develop here: 2b65e41. thanks for the PR!

spockz mentioned this pull request Sep 22, 2016

com.twitter.finagle.http.filter.StatsFilter is missing a corresponding module #556

Closed

kevinoliver suggested changes Sep 22, 2016

View reviewed changes

kevinoliver added the In Progress label Sep 22, 2016

mosesn suggested changes Sep 22, 2016

View reviewed changes

vkostyukov suggested changes Sep 23, 2016

View reviewed changes

spockz added 2 commits September 24, 2016 15:49

* Added NoOpModule

20fcb2c

* enableHttpStats => withHttpStats * withHttpStats is now idempotent * API fixes

Added docs on Http stats.

1647189

taylorleese changed the title ~~Added enableHttpStats config to Http.[Server|Client].~~ Added withHttpStats config to Http.[Server|Client]. Sep 25, 2016

kevinoliver reviewed Sep 26, 2016

View reviewed changes

kevinoliver suggested changes Sep 26, 2016

View reviewed changes

spockz added 2 commits September 26, 2016 22:08

Incorporated all comments except for test.

9b8fb93

Test-case for withHttpEnabled.

dae164b

kevinoliver reviewed Sep 26, 2016

View reviewed changes

kevinoliver suggested changes Sep 26, 2016

View reviewed changes

vkostyukov suggested changes Sep 26, 2016

View reviewed changes

* Milliseconds;

885fd28

* Add http scope for StatsFilter added through module; * Just Request

mosesn added Ship It and removed Ship It labels Oct 3, 2016

mosesn added the Ship It label Oct 3, 2016

jcrossley added the Merged Internally label Oct 7, 2016

mosesn closed this Oct 11, 2016

Added withHttpStats config to Http.[Server|Client]. #557

Added withHttpStats config to Http.[Server|Client]. #557

Conversation

spockz commented Sep 22, 2016 • edited by taylorleese

kevinoliver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinoliver left a comment

Choose a reason for hiding this comment

spockz commented Sep 26, 2016

Choose a reason for hiding this comment

kevinoliver left a comment

Choose a reason for hiding this comment

vkostyukov Sep 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkostyukov Sep 26, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 27, 2016

Current coverage is 68.24% (diff: 75.00%)

spockz commented Sep 27, 2016

vkostyukov commented Sep 27, 2016

kevinoliver commented Sep 27, 2016

spockz commented Sep 28, 2016

kevinoliver commented Sep 28, 2016

spockz commented Oct 7, 2016

mosesn commented Oct 7, 2016

jcrossley commented Oct 7, 2016

jcrossley commented Oct 7, 2016

spockz commented Oct 7, 2016

mosesn commented Oct 11, 2016

Added `withHttpStats` config to Http.[Server|Client]. #557

Added `withHttpStats` config to Http.[Server|Client]. #557

spockz commented Sep 22, 2016 •

edited by taylorleese

vkostyukov Sep 26, 2016 •

edited

vkostyukov Sep 26, 2016 •

edited