{"action":"created","comment":{"url":"https://api.github.com/repos/linkerd/linkerd/pulls/comments/128365819","pull_request_review_id":51026899,"id":128365819,"diff_hunk":"@@ -1,83 +1,130 @@\n package io.buoyant.router.h2\n \n import com.twitter.finagle.{Status => _, _}\n-import com.twitter.finagle.buoyant.h2.{Request, Response, Stream}\n+import com.twitter.finagle.buoyant.h2.{Frame, H2ReqRep, H2ResponseClassifier, Request, Response, Stream, param => h2param}\n+import com.twitter.finagle.buoyant.h2.StreamProxy._\n+import com.twitter.finagle.service.ResponseClass.Successful\n import com.twitter.finagle.stats.StatsReceiver\n import com.twitter.util._\n \n object StreamStatsFilter {\n   val role = Stack.Role(\"StreamStatsFilter\")\n   val module: Stackable[ServiceFactory[Request, Response]] =\n-    new Stack.Module1[param.Stats, ServiceFactory[Request, Response]] {\n+    new Stack.Module2[param.Stats, h2param.H2ResponseClassifier, ServiceFactory[Request, Response]] {\n       override def role = StreamStatsFilter.role\n       override def description = \"Record stats on h2 streams\"\n-      override def make(statsP: param.Stats, next: ServiceFactory[Request, Response]) = {\n+      override def make(\n+        statsP: param.Stats,\n+        classifierP: h2param.H2ResponseClassifier,\n+        next: ServiceFactory[Request, Response]\n+      ) = {\n         val param.Stats(stats) = statsP\n-        new StreamStatsFilter(stats).andThen(next)\n+        val h2param.H2ResponseClassifier(classifier) = classifierP\n+        new StreamStatsFilter(stats, classifier).andThen(next)\n       }\n     }\n }\n \n-class StreamStatsFilter(statsReceiver: StatsReceiver)\n+class StreamStatsFilter(statsReceiver: StatsReceiver, classifier: H2ResponseClassifier)\n   extends SimpleFilter[Request, Response] {\n \n-  class StreamStats(\n-    stats: StatsReceiver,\n-    durationName: Option[String] = None,\n-    successName: Option[String] = None\n-  ) {\n+  class StreamStats(protected val stats: StatsReceiver, durationName: Option[String] = None) {\n \n     private[this] val durationMs =\n       stats.stat(s\"${durationName.getOrElse(\"stream_duration\")}_ms\")\n-    private[this] val successes =\n-      stats.counter(s\"${successName.getOrElse(\"stream_\")}successes\")\n-    private[this] val failures =\n-      stats.counter(s\"${successName.getOrElse(\"stream_\")}failures\")\n+    private[this] val successes = stats.counter(\"stream_successes\")\n+    private[this] val failures = stats.counter(\"stream_failures\")\n \n-    @inline def apply(startT: Stopwatch.Elapsed)(result: Try[_]): Unit = {\n+    @inline def onEnd(startT: Stopwatch.Elapsed)(result: Try[_]): Unit = {\n       durationMs.add(startT().inMillis)\n       result match {\n         case Return(_) => successes.incr()\n         case Throw(_) => failures.incr()\n       }\n     }\n+\n+    @inline def apply(startT: Stopwatch.Elapsed)(result: Try[_]): Unit = onEnd(startT)(result)","path":"router/h2/src/main/scala/io/buoyant/router/h2/StreamStatsFilter.scala","position":61,"original_position":61,"commit_id":"af1acd1b2b595120fd3f04bd93de152edb49a7b5","original_commit_id":"af1acd1b2b595120fd3f04bd93de152edb49a7b5","user":{"login":"adleong","id":3979810,"avatar_url":"https://avatars0.githubusercontent.com/u/3979810?v=4","gravatar_id":"","url":"https://api.github.com/users/adleong","html_url":"https://github.com/adleong","followers_url":"https://api.github.com/users/adleong/followers","following_url":"https://api.github.com/users/adleong/following{/other_user}","gists_url":"https://api.github.com/users/adleong/gists{/gist_id}","starred_url":"https://api.github.com/users/adleong/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/adleong/subscriptions","organizations_url":"https://api.github.com/users/adleong/orgs","repos_url":"https://api.github.com/users/adleong/repos","events_url":"https://api.github.com/users/adleong/events{/privacy}","received_events_url":"https://api.github.com/users/adleong/received_events","type":"User","site_admin":false},"body":"Oh, I think I see, you override apply in the subclass.\r\n\r\nI would keep this all in apply and then in the subclass you can override it and call out to super.apply.\r\n\r\nOR you can embrace \"composition over inheritance\" and have FrameStats contain a StreamStats instead of extending it.","created_at":"2017-07-19T21:00:30Z","updated_at":"2017-07-19T21:18:01Z","html_url":"https://github.com/linkerd/linkerd/pull/1517#discussion_r128365819","pull_request_url":"https://api.github.com/repos/linkerd/linkerd/pulls/1517","_links":{"self":{"href":"https://api.github.com/repos/linkerd/linkerd/pulls/comments/128365819"},"html":{"href":"https://github.com/linkerd/linkerd/pull/1517#discussion_r128365819"},"pull_request":{"href":"https://api.github.com/repos/linkerd/linkerd/pulls/1517"}}},"pull_request":{"url":"https://api.github.com/repos/linkerd/linkerd/pulls/1517","id":131361198,"html_url":"https://github.com/linkerd/linkerd/pull/1517","diff_url":"https://github.com/linkerd/linkerd/pull/1517.diff","patch_url":"https://github.com/linkerd/linkerd/pull/1517.patch","issue_url":"https://api.github.com/repos/linkerd/linkerd/issues/1517","number":1517,"state":"open","locked":false,"title":"Advanced observability for H2","user":{"login":"hawkw","id":2796466,"avatar_url":"https://avatars1.githubusercontent.com/u/2796466?v=4","gravatar_id":"","url":"https://api.github.com/users/hawkw","html_url":"https://github.com/hawkw","followers_url":"https://api.github.com/users/hawkw/followers","following_url":"https://api.github.com/users/hawkw/following{/other_user}","gists_url":"https://api.github.com/users/hawkw/gists{/gist_id}","starred_url":"https://api.github.com/users/hawkw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hawkw/subscriptions","organizations_url":"https://api.github.com/users/hawkw/orgs","repos_url":"https://api.github.com/users/hawkw/repos","events_url":"https://api.github.com/users/hawkw/events{/privacy}","received_events_url":"https://api.github.com/users/hawkw/received_events","type":"User","site_admin":false},"body":"As described in #1508, I've added a `H2ResponseClassifier` type and modified the `StreamStatsFilter` to accept a `H2ResponseClassifier` as a stack param. The `H2ResponseClassifier` is more or less the same as a `ResponseClassifier`, except that it also requires a `Try[Frame]` containing the end frame of the H2 stream. \r\n\r\nIn order to pass the final frame to the `H2ResponseClassifier` without violating flow control, I've added a `StreamProxy` class which allows an `onFrame: Try[Frame] => Unit` function to be attached to an H2 stream. I've also used this to add the following per-frame metrics:\r\n\r\n+ `response/stream/data_frame/`\r\n  - `bytes`: the size (in bytes) of each data frame in the response stream \r\n  - `count`: the number of  data frames in the response stream \r\n+ `request/stream/data_frame/`\r\n  - `bytes`: the size (in bytes) of each data frame in the request stream \r\n  - `count`: the number of  data frames in the request stream \r\n\r\nI've added unit tests for the new functionality to `StreamStatsFilterTest`.","created_at":"2017-07-19T16:09:47Z","updated_at":"2017-07-19T21:18:01Z","closed_at":null,"merged_at":null,"merge_commit_sha":null,"assignee":{"login":"hawkw","id":2796466,"avatar_url":"https://avatars1.githubusercontent.com/u/2796466?v=4","gravatar_id":"","url":"https://api.github.com/users/hawkw","html_url":"https://github.com/hawkw","followers_url":"https://api.github.com/users/hawkw/followers","following_url":"https://api.github.com/users/hawkw/following{/other_user}","gists_url":"https://api.github.com/users/hawkw/gists{/gist_id}","starred_url":"https://api.github.com/users/hawkw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hawkw/subscriptions","organizations_url":"https://api.github.com/users/hawkw/orgs","repos_url":"https://api.github.com/users/hawkw/repos","events_url":"https://api.github.com/users/hawkw/events{/privacy}","received_events_url":"https://api.github.com/users/hawkw/received_events","type":"User","site_admin":false},"assignees":[{"login":"hawkw","id":2796466,"avatar_url":"https://avatars1.githubusercontent.com/u/2796466?v=4","gravatar_id":"","url":"https://api.github.com/users/hawkw","html_url":"https://github.com/hawkw","followers_url":"https://api.github.com/users/hawkw/followers","following_url":"https://api.github.com/users/hawkw/following{/other_user}","gists_url":"https://api.github.com/users/hawkw/gists{/gist_id}","starred_url":"https://api.github.com/users/hawkw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hawkw/subscriptions","organizations_url":"https://api.github.com/users/hawkw/orgs","repos_url":"https://api.github.com/users/hawkw/repos","events_url":"https://api.github.com/users/hawkw/events{/privacy}","received_events_url":"https://api.github.com/users/hawkw/received_events","type":"User","site_admin":false}],"requested_reviewers":[{"login":"olix0r","id":240738,"avatar_url":"https://avatars3.githubusercontent.com/u/240738?v=4","gravatar_id":"","url":"https://api.github.com/users/olix0r","html_url":"https://github.com/olix0r","followers_url":"https://api.github.com/users/olix0r/followers","following_url":"https://api.github.com/users/olix0r/following{/other_user}","gists_url":"https://api.github.com/users/olix0r/gists{/gist_id}","starred_url":"https://api.github.com/users/olix0r/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/olix0r/subscriptions","organizations_url":"https://api.github.com/users/olix0r/orgs","repos_url":"https://api.github.com/users/olix0r/repos","events_url":"https://api.github.com/users/olix0r/events{/privacy}","received_events_url":"https://api.github.com/users/olix0r/received_events","type":"User","site_admin":false}],"milestone":null,"commits_url":"https://api.github.com/repos/linkerd/linkerd/pulls/1517/commits","review_comments_url":"https://api.github.com/repos/linkerd/linkerd/pulls/1517/comments","review_comment_url":"https://api.github.com/repos/linkerd/linkerd/pulls/comments{/number}","comments_url":"https://api.github.com/repos/linkerd/linkerd/issues/1517/comments","statuses_url":"https://api.github.com/repos/linkerd/linkerd/statuses/af1acd1b2b595120fd3f04bd93de152edb49a7b5","head":{"label":"linkerd:eliza/advanced-h2-stats","ref":"eliza/advanced-h2-stats","sha":"af1acd1b2b595120fd3f04bd93de152edb49a7b5","user":{"login":"linkerd","id":25301026,"avatar_url":"https://avatars3.githubusercontent.com/u/25301026?v=4","gravatar_id":"","url":"https://api.github.com/users/linkerd","html_url":"https://github.com/linkerd","followers_url":"https://api.github.com/users/linkerd/followers","following_url":"https://api.github.com/users/linkerd/following{/other_user}","gists_url":"https://api.github.com/users/linkerd/gists{/gist_id}","starred_url":"https://api.github.com/users/linkerd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/linkerd/subscriptions","organizations_url":"https://api.github.com/users/linkerd/orgs","repos_url":"https://api.github.com/users/linkerd/repos","events_url":"https://api.github.com/users/linkerd/events{/privacy}","received_events_url":"https://api.github.com/users/linkerd/received_events","type":"Organization","site_admin":false},"repo":{"id":49605596,"name":"linkerd","full_name":"linkerd/linkerd","owner":{"login":"linkerd","id":25301026,"avatar_url":"https://avatars3.githubusercontent.com/u/25301026?v=4","gravatar_id":"","url":"https://api.github.com/users/linkerd","html_url":"https://github.com/linkerd","followers_url":"https://api.github.com/users/linkerd/followers","following_url":"https://api.github.com/users/linkerd/following{/other_user}","gists_url":"https://api.github.com/users/linkerd/gists{/gist_id}","starred_url":"https://api.github.com/users/linkerd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/linkerd/subscriptions","organizations_url":"https://api.github.com/users/linkerd/orgs","repos_url":"https://api.github.com/users/linkerd/repos","events_url":"https://api.github.com/users/linkerd/events{/privacy}","received_events_url":"https://api.github.com/users/linkerd/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/linkerd/linkerd","description":"Resilient service mesh for cloud native apps","fork":false,"url":"https://api.github.com/repos/linkerd/linkerd","forks_url":"https://api.github.com/repos/linkerd/linkerd/forks","keys_url":"https://api.github.com/repos/linkerd/linkerd/keys{/key_id}","collaborators_url":"https://api.github.com/repos/linkerd/linkerd/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/linkerd/linkerd/teams","hooks_url":"https://api.github.com/repos/linkerd/linkerd/hooks","issue_events_url":"https://api.github.com/repos/linkerd/linkerd/issues/events{/number}","events_url":"https://api.github.com/repos/linkerd/linkerd/events","assignees_url":"https://api.github.com/repos/linkerd/linkerd/assignees{/user}","branches_url":"https://api.github.com/repos/linkerd/linkerd/branches{/branch}","tags_url":"https://api.github.com/repos/linkerd/linkerd/tags","blobs_url":"https://api.github.com/repos/linkerd/linkerd/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/linkerd/linkerd/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/linkerd/linkerd/git/refs{/sha}","trees_url":"https://api.github.com/repos/linkerd/linkerd/git/trees{/sha}","statuses_url":"https://api.github.com/repos/linkerd/linkerd/statuses/{sha}","languages_url":"https://api.github.com/repos/linkerd/linkerd/languages","stargazers_url":"https://api.github.com/repos/linkerd/linkerd/stargazers","contributors_url":"https://api.github.com/repos/linkerd/linkerd/contributors","subscribers_url":"https://api.github.com/repos/linkerd/linkerd/subscribers","subscription_url":"https://api.github.com/repos/linkerd/linkerd/subscription","commits_url":"https://api.github.com/repos/linkerd/linkerd/commits{/sha}","git_commits_url":"https://api.github.com/repos/linkerd/linkerd/git/commits{/sha}","comments_url":"https://api.github.com/repos/linkerd/linkerd/comments{/number}","issue_comment_url":"https://api.github.com/repos/linkerd/linkerd/issues/comments{/number}","contents_url":"https://api.github.com/repos/linkerd/linkerd/contents/{+path}","compare_url":"https://api.github.com/repos/linkerd/linkerd/compare/{base}...{head}","merges_url":"https://api.github.com/repos/linkerd/linkerd/merges","archive_url":"https://api.github.com/repos/linkerd/linkerd/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/linkerd/linkerd/downloads","issues_url":"https://api.github.com/repos/linkerd/linkerd/issues{/number}","pulls_url":"https://api.github.com/repos/linkerd/linkerd/pulls{/number}","milestones_url":"https://api.github.com/repos/linkerd/linkerd/milestones{/number}","notifications_url":"https://api.github.com/repos/linkerd/linkerd/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/linkerd/linkerd/labels{/name}","releases_url":"https://api.github.com/repos/linkerd/linkerd/releases{/id}","deployments_url":"https://api.github.com/repos/linkerd/linkerd/deployments","created_at":"2016-01-13T22:14:52Z","updated_at":"2017-07-19T16:12:39Z","pushed_at":"2017-07-19T21:03:23Z","git_url":"git://github.com/linkerd/linkerd.git","ssh_url":"git@github.com:linkerd/linkerd.git","clone_url":"https://github.com/linkerd/linkerd.git","svn_url":"https://github.com/linkerd/linkerd","homepage":"https://linkerd.io","size":15654,"stargazers_count":2381,"watchers_count":2381,"language":"Scala","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":210,"mirror_url":null,"open_issues_count":161,"forks":210,"open_issues":161,"watchers":2381,"default_branch":"master"}},"base":{"label":"linkerd:master","ref":"master","sha":"e031751b445e9029aedebca1cc0eab0ca0837e68","user":{"login":"linkerd","id":25301026,"avatar_url":"https://avatars3.githubusercontent.com/u/25301026?v=4","gravatar_id":"","url":"https://api.github.com/users/linkerd","html_url":"https://github.com/linkerd","followers_url":"https://api.github.com/users/linkerd/followers","following_url":"https://api.github.com/users/linkerd/following{/other_user}","gists_url":"https://api.github.com/users/linkerd/gists{/gist_id}","starred_url":"https://api.github.com/users/linkerd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/linkerd/subscriptions","organizations_url":"https://api.github.com/users/linkerd/orgs","repos_url":"https://api.github.com/users/linkerd/repos","events_url":"https://api.github.com/users/linkerd/events{/privacy}","received_events_url":"https://api.github.com/users/linkerd/received_events","type":"Organization","site_admin":false},"repo":{"id":49605596,"name":"linkerd","full_name":"linkerd/linkerd","owner":{"login":"linkerd","id":25301026,"avatar_url":"https://avatars3.githubusercontent.com/u/25301026?v=4","gravatar_id":"","url":"https://api.github.com/users/linkerd","html_url":"https://github.com/linkerd","followers_url":"https://api.github.com/users/linkerd/followers","following_url":"https://api.github.com/users/linkerd/following{/other_user}","gists_url":"https://api.github.com/users/linkerd/gists{/gist_id}","starred_url":"https://api.github.com/users/linkerd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/linkerd/subscriptions","organizations_url":"https://api.github.com/users/linkerd/orgs","repos_url":"https://api.github.com/users/linkerd/repos","events_url":"https://api.github.com/users/linkerd/events{/privacy}","received_events_url":"https://api.github.com/users/linkerd/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/linkerd/linkerd","description":"Resilient service mesh for cloud native apps","fork":false,"url":"https://api.github.com/repos/linkerd/linkerd","forks_url":"https://api.github.com/repos/linkerd/linkerd/forks","keys_url":"https://api.github.com/repos/linkerd/linkerd/keys{/key_id}","collaborators_url":"https://api.github.com/repos/linkerd/linkerd/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/linkerd/linkerd/teams","hooks_url":"https://api.github.com/repos/linkerd/linkerd/hooks","issue_events_url":"https://api.github.com/repos/linkerd/linkerd/issues/events{/number}","events_url":"https://api.github.com/repos/linkerd/linkerd/events","assignees_url":"https://api.github.com/repos/linkerd/linkerd/assignees{/user}","branches_url":"https://api.github.com/repos/linkerd/linkerd/branches{/branch}","tags_url":"https://api.github.com/repos/linkerd/linkerd/tags","blobs_url":"https://api.github.com/repos/linkerd/linkerd/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/linkerd/linkerd/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/linkerd/linkerd/git/refs{/sha}","trees_url":"https://api.github.com/repos/linkerd/linkerd/git/trees{/sha}","statuses_url":"https://api.github.com/repos/linkerd/linkerd/statuses/{sha}","languages_url":"https://api.github.com/repos/linkerd/linkerd/languages","stargazers_url":"https://api.github.com/repos/linkerd/linkerd/stargazers","contributors_url":"https://api.github.com/repos/linkerd/linkerd/contributors","subscribers_url":"https://api.github.com/repos/linkerd/linkerd/subscribers","subscription_url":"https://api.github.com/repos/linkerd/linkerd/subscription","commits_url":"https://api.github.com/repos/linkerd/linkerd/commits{/sha}","git_commits_url":"https://api.github.com/repos/linkerd/linkerd/git/commits{/sha}","comments_url":"https://api.github.com/repos/linkerd/linkerd/comments{/number}","issue_comment_url":"https://api.github.com/repos/linkerd/linkerd/issues/comments{/number}","contents_url":"https://api.github.com/repos/linkerd/linkerd/contents/{+path}","compare_url":"https://api.github.com/repos/linkerd/linkerd/compare/{base}...{head}","merges_url":"https://api.github.com/repos/linkerd/linkerd/merges","archive_url":"https://api.github.com/repos/linkerd/linkerd/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/linkerd/linkerd/downloads","issues_url":"https://api.github.com/repos/linkerd/linkerd/issues{/number}","pulls_url":"https://api.github.com/repos/linkerd/linkerd/pulls{/number}","milestones_url":"https://api.github.com/repos/linkerd/linkerd/milestones{/number}","notifications_url":"https://api.github.com/repos/linkerd/linkerd/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/linkerd/linkerd/labels{/name}","releases_url":"https://api.github.com/repos/linkerd/linkerd/releases{/id}","deployments_url":"https://api.github.com/repos/linkerd/linkerd/deployments","created_at":"2016-01-13T22:14:52Z","updated_at":"2017-07-19T16:12:39Z","pushed_at":"2017-07-19T21:03:23Z","git_url":"git://github.com/linkerd/linkerd.git","ssh_url":"git@github.com:linkerd/linkerd.git","clone_url":"https://github.com/linkerd/linkerd.git","svn_url":"https://github.com/linkerd/linkerd","homepage":"https://linkerd.io","size":15654,"stargazers_count":2381,"watchers_count":2381,"language":"Scala","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":210,"mirror_url":null,"open_issues_count":161,"forks":210,"open_issues":161,"watchers":2381,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/linkerd/linkerd/pulls/1517"},"html":{"href":"https://github.com/linkerd/linkerd/pull/1517"},"issue":{"href":"https://api.github.com/repos/linkerd/linkerd/issues/1517"},"comments":{"href":"https://api.github.com/repos/linkerd/linkerd/issues/1517/comments"},"review_comments":{"href":"https://api.github.com/repos/linkerd/linkerd/pulls/1517/comments"},"review_comment":{"href":"https://api.github.com/repos/linkerd/linkerd/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/linkerd/linkerd/pulls/1517/commits"},"statuses":{"href":"https://api.github.com/repos/linkerd/linkerd/statuses/af1acd1b2b595120fd3f04bd93de152edb49a7b5"}}}}
{"actor":{"display_login":"adleong"}}