From 03f8277f17c46c74060e5728f92ad6c974d3edca Mon Sep 17 00:00:00 2001 From: Sam Starling Date: Fri, 16 Dec 2016 16:53:23 +0100 Subject: [PATCH] Add the concept of RequestLabeller for metric labels --- .../example/TestServer.scala | 2 -- .../filter/HttpLatencyMonitoringFilter.scala | 13 ++++--- .../filter/HttpMonitoringFilter.scala | 12 +++---- .../filter/HttpRequestLabeller.scala | 34 ++++++++++++++++++ .../filter/StatusClass.scala | 9 ----- .../prometheusfinagle/UnitTest.scala | 11 +++++- .../HttpLatencyMonitoringFilterSpec.scala | 27 ++++---------- .../filter/HttpMonitoringFilterSpec.scala | 28 ++++----------- .../filter/HttpRequestLabellerSpec.scala | 35 +++++++++++++++++++ 9 files changed, 103 insertions(+), 68 deletions(-) create mode 100644 src/main/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabeller.scala delete mode 100644 src/main/scala/com/samstarling/prometheusfinagle/filter/StatusClass.scala create mode 100644 src/test/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabellerSpec.scala diff --git a/src/main/scala/com/samstarling/prometheusfinagle/example/TestServer.scala b/src/main/scala/com/samstarling/prometheusfinagle/example/TestServer.scala index 979dad3..527ae87 100644 --- a/src/main/scala/com/samstarling/prometheusfinagle/example/TestServer.scala +++ b/src/main/scala/com/samstarling/prometheusfinagle/example/TestServer.scala @@ -43,5 +43,3 @@ object TestServer extends App { .name("HttpServer") .build(latencyMonitoringFilter andThen monitoringFilter andThen routingService) } - - diff --git a/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilter.scala b/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilter.scala index 78f38c4..154bb4e 100644 --- a/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilter.scala +++ b/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilter.scala @@ -5,23 +5,22 @@ import com.twitter.finagle.http.{Request, Response} import com.twitter.finagle.{Service, SimpleFilter} import com.twitter.util.{Future, Stopwatch} -class HttpLatencyMonitoringFilter(telemetry: Telemetry, buckets: Seq[Double]) extends SimpleFilter[Request, Response] { +class HttpLatencyMonitoringFilter(telemetry: Telemetry, + buckets: Seq[Double], + labeller: RequestLabeller = new HttpRequestLabeller) extends SimpleFilter[Request, Response] { private val histogram = telemetry.histogram( name = "incoming_http_request_latency_seconds", help = "A histogram of the response latency for HTTP requests", - labelNames = Seq("method", "status", "statusClass"), + labelNames = labeller.keys, buckets = buckets ) override def apply(request: Request, service: Service[Request, Response]): Future[Response] = { val stopwatch = Stopwatch.start() service(request) onSuccess { response => - histogram.labels( - request.method.toString, - response.status.code.toString, - StatusClass.forStatus(response.status) - ).observe(stopwatch().inMilliseconds / 1000.0) + val labels = labeller.labelsFor(request, response) + histogram.labels(labels: _*).observe(stopwatch().inMilliseconds / 1000.0) } } } diff --git a/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilter.scala b/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilter.scala index ba98d56..958a88d 100644 --- a/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilter.scala +++ b/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilter.scala @@ -5,21 +5,19 @@ import com.twitter.finagle.http.{Request, Response} import com.twitter.finagle.{Service, SimpleFilter} import com.twitter.util.Future -class HttpMonitoringFilter(telemetry: Telemetry) extends SimpleFilter[Request, Response] { +class HttpMonitoringFilter(telemetry: Telemetry, + labeller: RequestLabeller = new HttpRequestLabeller) extends SimpleFilter[Request, Response] { private val counter = telemetry.counter( name = "incoming_http_requests_total", help = "The number of incoming HTTP requests", - labelNames = Seq("method", "status", "statusClass") + labelNames = labeller.keys ) override def apply(request: Request, service: Service[Request, Response]): Future[Response] = { service(request) onSuccess { response => - counter.labels( - request.method.toString, - response.status.code.toString, - StatusClass.forStatus(response.status) - ).inc() + val labels = labeller.labelsFor(request, response) + counter.labels(labels: _*).inc() } } } diff --git a/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabeller.scala b/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabeller.scala new file mode 100644 index 0000000..af15b53 --- /dev/null +++ b/src/main/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabeller.scala @@ -0,0 +1,34 @@ +package com.samstarling.prometheusfinagle.filter + +import com.twitter.finagle.http.{Request, Response} + +trait RequestLabeller { + + /** + * TODO Explain why this is modelled like this + * @return + */ + def keys: Seq[String] + + /** + * TODO Explain why this is modelled like this + * @return + */ + def labelsFor(request: Request, response: Response): Seq[String] +} + +// TODO Include response in name +class HttpRequestLabeller extends RequestLabeller { + + def keys: Seq[String] = Seq( + "status", + "statusClass", + "method" + ) + + def labelsFor(request: Request, response: Response): List[String] = List( + response.status.code.toString, + s"${response.status.code.toString.charAt(0)}xx", + request.method.toString + ) +} diff --git a/src/main/scala/com/samstarling/prometheusfinagle/filter/StatusClass.scala b/src/main/scala/com/samstarling/prometheusfinagle/filter/StatusClass.scala deleted file mode 100644 index 653f92c..0000000 --- a/src/main/scala/com/samstarling/prometheusfinagle/filter/StatusClass.scala +++ /dev/null @@ -1,9 +0,0 @@ -package com.samstarling.prometheusfinagle.filter - -import com.twitter.finagle.http.Status - -protected object StatusClass { - def forStatus(status: Status): String = { - status.code.toString.charAt(0) + "xx" - } -} diff --git a/src/test/scala/com/samstarling/prometheusfinagle/UnitTest.scala b/src/test/scala/com/samstarling/prometheusfinagle/UnitTest.scala index 3c41ff0..da0da7b 100644 --- a/src/test/scala/com/samstarling/prometheusfinagle/UnitTest.scala +++ b/src/test/scala/com/samstarling/prometheusfinagle/UnitTest.scala @@ -1,6 +1,15 @@ package com.samstarling.prometheusfinagle +import com.samstarling.prometheusfinagle.filter.RequestLabeller +import com.twitter.finagle.http.{Request, Response} import org.specs2.mock.Mockito import org.specs2.mutable.Specification -trait UnitTest extends Specification with Mockito +trait UnitTest extends Specification with Mockito { + + class TestLabeller extends RequestLabeller { + override def keys: List[String] = List("foo") + override def labelsFor(request: Request, response: Response): List[String] = List("bar") + } + +} diff --git a/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilterSpec.scala b/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilterSpec.scala index 0993bfd..c521a53 100644 --- a/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilterSpec.scala +++ b/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpLatencyMonitoringFilterSpec.scala @@ -1,7 +1,7 @@ package com.samstarling.prometheusfinagle.filter import com.samstarling.prometheusfinagle.UnitTest -import com.samstarling.prometheusfinagle.helper.CollectorRegistryHelper +import com.samstarling.prometheusfinagle.helper.{CollectorHelper, CollectorRegistryHelper} import com.samstarling.prometheusfinagle.metrics.Telemetry import com.twitter.finagle.Service import com.twitter.finagle.http.{Method, Request, Response, Status} @@ -26,7 +26,8 @@ class HttpLatencyMonitoringFilterSpec extends UnitTest { val registryHelper = CollectorRegistryHelper(registry) val telemetry = new Telemetry(registry, "test") val buckets = Seq(1.0, 2.0) - val filter = new HttpLatencyMonitoringFilter(telemetry, buckets) + val labeller = new TestLabeller + val filter = new HttpLatencyMonitoringFilter(telemetry, buckets, labeller) val service = mock[Service[Request, Response]] val slowService = new SlowService val request = Request(Method.Get, "/foo/bar") @@ -54,28 +55,12 @@ class HttpLatencyMonitoringFilterSpec extends UnitTest { .map(_.map(_.value).sum) ==== Some(1.0) } - "records a method label" in new Context { - Await.result(filter.apply(request, slowService)) - - registryHelper.samples - .get("test_incoming_http_request_latency_seconds_count") - .map(_.head.dimensions.get("method").get) ==== Some("GET") - } - - "records a status label" in new Context { - Await.result(filter.apply(request, slowService)) - - registryHelper.samples - .get("test_incoming_http_request_latency_seconds_count") - .map(_.head.dimensions.get("status").get) ==== Some("200") - } - - "records a statusClass label" in new Context { - Await.result(filter.apply(request, slowService)) + "increments the counter with the labels from the labeller" in new Context { + Await.result(filter.apply(request, service)) registryHelper.samples .get("test_incoming_http_request_latency_seconds_count") - .map(_.head.dimensions.get("statusClass").get) ==== Some("2xx") + .map(_.head.dimensions.get("foo").get) ==== Some("bar") } "categorises requests into the correct bucket" in new Context { diff --git a/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilterSpec.scala b/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilterSpec.scala index cc35701..d02e83e 100644 --- a/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilterSpec.scala +++ b/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpMonitoringFilterSpec.scala @@ -16,7 +16,8 @@ class HttpMonitoringFilterSpec extends UnitTest { trait Context extends Scope { val registry = new CollectorRegistry(true) val telemetry = new Telemetry(registry, "test") - val filter = new HttpMonitoringFilter(telemetry) + val labeller = new TestLabeller + val filter = new HttpMonitoringFilter(telemetry, labeller) val service = mock[Service[Request, Response]] val request = Request(Method.Get, "/foo/bar") val serviceResponse = Response(Status.Created) @@ -50,26 +51,11 @@ class HttpMonitoringFilterSpec extends UnitTest { } } - "increments the counter with" >> { - "a method label" in new Context { - Await.result(filter.apply(request, service)) - CollectorHelper.firstSampleFor(counter).map { sample => - sample.labelValues.asScala(0) ==== "GET" - } - } - - "a status label" in new Context { - Await.result(filter.apply(request, service)) - CollectorHelper.firstSampleFor(counter).map { sample => - sample.labelValues.asScala(1) ==== "201" - } - } - - "a statusClass label" in new Context { - Await.result(filter.apply(request, service)) - CollectorHelper.firstSampleFor(counter).map { sample => - sample.labelValues.asScala(2) ==== "2xx" - } + "increments the counter with the labels from the labeller" in new Context { + Await.result(filter.apply(request, service)) + CollectorHelper.firstSampleFor(counter).map { sample => + sample.labelNames.asScala(0) ==== "foo" + sample.labelValues.asScala(0) ==== "bar" } } } diff --git a/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabellerSpec.scala b/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabellerSpec.scala new file mode 100644 index 0000000..3309d4d --- /dev/null +++ b/src/test/scala/com/samstarling/prometheusfinagle/filter/HttpRequestLabellerSpec.scala @@ -0,0 +1,35 @@ +package com.samstarling.prometheusfinagle.filter + +import com.samstarling.prometheusfinagle.UnitTest +import com.twitter.finagle.http.{Method, Request, Response, Status} +import org.specs2.specification.Scope + +class HttpRequestLabellerSpec extends UnitTest { + + trait Context extends Scope { + val request = Request(Method.Get, "/foo/bar") + val response = Response(Status.Ok) + val labeller = new HttpRequestLabeller() + val labels = labeller.labelsFor(request, response) + } + + "keys" >> { + "returns the keys in the correct order" in new Context { + labeller.keys ==== Seq("status", "statusClass", "method") + } + } + + "labelsFor" >> { + "returns the status code of the response" in new Context { + labels(0) ==== "200" + } + + "returns the status class of the request" in new Context { + labels(1) ==== "2xx" + } + + "returns the method of the request" in new Context { + labels(2) ==== "GET" + } + } +}