-
Notifications
You must be signed in to change notification settings - Fork 4
Moved workbench-metrics from rawls #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# Changelog | ||
|
||
This file documents changes to the `workbench-metrics` library, including notes on how to upgrade to new versions. | ||
|
||
## 0.1 | ||
|
||
SBT dependency: `"org.broadinstitute.dsde.workbench" %% "workbench-metrics" % "0.1-xxxxxxx"` | ||
|
||
### Added | ||
|
||
- This library | ||
- `WorkbenchInstrumented`, a mixin trait for instrumenting arbitrary code | ||
- `InstrumentationDirectives.instrumentRequest`, an akka-http directive for instrumenting incoming HTTP requests |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
package org.broadinstitute.dsde.workbench.metrics | ||
|
||
import java.util.UUID | ||
import akka.http.scaladsl.model._ | ||
import scala.annotation.implicitNotFound | ||
|
||
/** | ||
* Typeclass for something that can be converted into a metric name fragment. | ||
* Metric name fragments can be combined via ExpandedMetricBuilder to generate an "expanded" metric name. | ||
* By default this just calls toString on the object of type A, but this can be overridden. | ||
*/ | ||
@implicitNotFound(msg = "Cannot expand instances of type ${A}") | ||
trait Expansion[A] { | ||
def makeName(a: A): String = a.toString | ||
|
||
final def makeNameWithKey(key: String, a: A) = | ||
s"$key.${makeName(a)}" | ||
} | ||
|
||
object Expansion { | ||
|
||
// Typeclass instances: | ||
|
||
/** | ||
* Implicit expansion for UUID using the default makeName. | ||
*/ | ||
implicit object UUIDExpansion extends Expansion[UUID] | ||
|
||
/** | ||
* Implicit expansion for HttpMethod. | ||
*/ | ||
implicit object HttpMethodExpansion extends Expansion[HttpMethod] { | ||
override def makeName(m: HttpMethod): String = m.value.toLowerCase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note this used to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} | ||
|
||
/** | ||
* Implicit expansion for Uri. | ||
* Statsd doesn't allow slashes in metric names, so we override makeName to override | ||
* the default toString based implementation. | ||
*/ | ||
implicit object UriExpansion extends Expansion[Uri] { | ||
override def makeName(uri: Uri): String = { | ||
val path = if (uri.path.startsWithSlash) uri.path.tail.toString else uri.path | ||
path.toString.replace('/', '.') | ||
} | ||
} | ||
|
||
/** | ||
* Implicit expansion for a StatusCode. | ||
*/ | ||
implicit object StatusCodeExpansion extends Expansion[StatusCode] { | ||
override def makeName(statusCode: StatusCode): String = statusCode.intValue.toString | ||
} | ||
|
||
// Implicit expansions for String and Int. | ||
// It's preferable to use more specific types when possible, but sometimes expanding | ||
// primitive types into metric names is needed. | ||
implicit object StringExpansion extends Expansion[String] | ||
implicit object IntExpansion extends Expansion[Int] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package org.broadinstitute.dsde.workbench.metrics | ||
|
||
import scala.concurrent.duration._ | ||
|
||
import akka.http.scaladsl.server.Directive0 | ||
import akka.http.scaladsl.server.Directives.{extractRequest, mapResponse} | ||
|
||
trait InstrumentationDirectives extends WorkbenchInstrumented { | ||
def instrumentRequest: Directive0 = extractRequest flatMap { request => | ||
val timeStamp = System.currentTimeMillis | ||
mapResponse { response => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
it seems to work, according to tests. |
||
httpRequestCounter(ExpandedMetricBuilder.empty)(request, response).inc() | ||
httpRequestTimer(ExpandedMetricBuilder.empty)(request, response).update(System.currentTimeMillis - timeStamp, MILLISECONDS) | ||
response | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
package org.broadinstitute.dsde.workbench.metrics | ||
|
||
import akka.http.scaladsl.model.{HttpRequest, HttpResponse} | ||
import nl.grons.metrics.scala._ | ||
import org.broadinstitute.dsde.workbench.metrics.Expansion._ | ||
|
||
/** | ||
* Mixin trait for instrumentation. | ||
* Extends metrics-scala [[DefaultInstrumented]] and provides additional utilties for generating | ||
* metric names for Workbench. | ||
*/ | ||
trait WorkbenchInstrumented extends DefaultInstrumented { | ||
/** | ||
* Base name for all metrics. This will be prepended to all generated metric names. | ||
* Example: dev.firecloud.rawls | ||
*/ | ||
protected val workbenchMetricBaseName: String | ||
override lazy val metricBaseName = MetricName(workbenchMetricBaseName) | ||
|
||
/** | ||
* Utility for building expanded metric names in a typesafe way. Example usage: | ||
* {{{ | ||
* val counter: Counter = | ||
* ExpandedMetricBuilder | ||
* .expand(WorkspaceMetric, workspaceName) | ||
* .expand(SubmissionMetric, submissionId) | ||
* .expand(WorkflowStatusMetric, status) | ||
* .asCounter("count") | ||
* // counter has name: | ||
* // <baseName>.workspace.<workspaceNamespace>.<workspaceName>.submission.<submissionId>.workflowStatus.<workflowStatus>.count | ||
* counter += 1000 | ||
* }}} | ||
* | ||
* Note the above will only compile if there are [[Expansion]] instances for the types passed to the expand method. | ||
*/ | ||
protected class ExpandedMetricBuilder private (m: String = "") { | ||
def expand[A: Expansion](key: String, a: A): ExpandedMetricBuilder = { | ||
new ExpandedMetricBuilder( | ||
(if (m == "") m else m + ".") + implicitly[Expansion[A]].makeNameWithKey(key, a)) | ||
} | ||
|
||
def asCounter(name: String): Counter = | ||
metrics.counter(makeName(name)) | ||
|
||
def asGauge[T](name: String)(fn: => T): Gauge[T] = | ||
metrics.gauge(makeName(name))(fn) | ||
|
||
def asTimer(name: String): Timer = | ||
metrics.timer(makeName(name)) | ||
|
||
private def makeName(name: String): String = s"$m.$name" | ||
|
||
override def toString: String = m | ||
} | ||
|
||
object ExpandedMetricBuilder { | ||
def expand[A: Expansion](key: String, a: A): ExpandedMetricBuilder = { | ||
new ExpandedMetricBuilder().expand(key, a) | ||
} | ||
|
||
def empty: ExpandedMetricBuilder = { | ||
new ExpandedMetricBuilder() | ||
} | ||
} | ||
|
||
// Keys for expanded metric fragments | ||
final val HttpRequestMethodMetricKey = "httpRequestMethod" | ||
final val HttpRequestUriMetricKey = "httpRequestUri" | ||
final val HttpResponseStatusCodeMetricKey = "httpResponseStatusCode" | ||
|
||
// Handy definitions which can be used by implementing classes: | ||
|
||
protected def httpRequestMetricBuilder(builder: ExpandedMetricBuilder): (HttpRequest, HttpResponse) => ExpandedMetricBuilder = { | ||
(httpRequest, httpResponse) => builder | ||
.expand(HttpRequestMethodMetricKey, httpRequest.method) | ||
.expand(HttpRequestUriMetricKey, httpRequest.uri) | ||
.expand(HttpResponseStatusCodeMetricKey, httpResponse.status) | ||
} | ||
|
||
protected implicit def httpRequestCounter(implicit builder: ExpandedMetricBuilder): (HttpRequest, HttpResponse) => Counter = | ||
httpRequestMetricBuilder(builder)(_, _).asCounter("request") | ||
|
||
protected implicit def httpRequestTimer(implicit builder: ExpandedMetricBuilder): (HttpRequest, HttpResponse) => Timer = | ||
httpRequestMetricBuilder(builder)(_, _).asTimer("latency") | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package org.broadinstitute.dsde.workbench | ||
|
||
import java.util.concurrent.TimeUnit | ||
|
||
import com.codahale.metrics.SharedMetricRegistries | ||
import com.readytalk.metrics.StatsDReporter | ||
import com.typesafe.scalalogging.LazyLogging | ||
|
||
import scala.concurrent.duration.Duration | ||
|
||
package object metrics extends LazyLogging { | ||
def startStatsDReporter(host: String, port: Int, period: Duration, registryName: String = "default"): Unit = { | ||
logger.info(s"Starting statsd reporter writing to [$host:$port] with period [${period.toMillis} ms]") | ||
val reporter = StatsDReporter.forRegistry(SharedMetricRegistries.getOrCreate(registryName)) | ||
.convertRatesTo(TimeUnit.SECONDS) | ||
.convertDurationsTo(TimeUnit.MILLISECONDS) | ||
.build(host, port) | ||
reporter.start(period.toSeconds, period.toSeconds, TimeUnit.SECONDS) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
package org.broadinstitute.dsde.workbench.metrics | ||
|
||
import java.util.UUID | ||
|
||
import akka.http.scaladsl.model._ | ||
import org.broadinstitute.dsde.workbench.metrics.Expansion._ | ||
import org.scalatest.{FlatSpec, Matchers} | ||
|
||
/** | ||
* Created by rtitle on 7/16/17. | ||
*/ | ||
class ExpansionSpec extends FlatSpec with Matchers { | ||
|
||
"the Expansion typeclass" should "expand UUIDs" in { | ||
val test = UUID.randomUUID | ||
assertResult(test.toString) { | ||
implicitly[Expansion[UUID]].makeName(test) | ||
} | ||
} | ||
|
||
it should "expand HttpMethods" in { | ||
val test = HttpMethods.PATCH | ||
assertResult("patch") { | ||
implicitly[Expansion[HttpMethod]].makeName(test) | ||
} | ||
} | ||
|
||
it should "expand StatusCodes" in { | ||
val test = StatusCodes.Forbidden | ||
assertResult("403") { | ||
implicitly[Expansion[StatusCode]].makeName(test) | ||
} | ||
} | ||
|
||
it should "expand Uris" in { | ||
val test = Uri("/workspace/broad-dsde-dev/myspace") | ||
assertResult("workspace.broad-dsde-dev.myspace") { | ||
implicitly[Expansion[Uri]].makeName(test) | ||
} | ||
} | ||
|
||
it should "expand primitives" in { | ||
val str = "A String" | ||
val int = 42 | ||
assertResult(str) { | ||
implicitly[Expansion[String]].makeName(str) | ||
} | ||
assertResult("42") { | ||
implicitly[Expansion[Int]].makeName(int) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
package org.broadinstitute.dsde.workbench.metrics | ||
|
||
import akka.http.scaladsl.model.StatusCodes | ||
import akka.http.scaladsl.server | ||
import akka.http.scaladsl.server.Directives.{complete, get, pathEndOrSingleSlash, pathPrefix} | ||
import akka.http.scaladsl.testkit.ScalatestRouteTest | ||
import org.broadinstitute.dsde.workbench.util.MockitoTestUtils | ||
import org.scalatest.concurrent.Eventually | ||
import org.scalatest.{FlatSpec, Matchers} | ||
|
||
class InstrumentationDirectivesSpec extends FlatSpec with InstrumentationDirectives with Matchers with StatsDTestUtils with ScalatestRouteTest with Eventually with MockitoTestUtils { | ||
|
||
override val workbenchMetricBaseName = "test" | ||
|
||
def testRoute: server.Route = | ||
instrumentRequest { | ||
pathPrefix("ping") { | ||
pathEndOrSingleSlash { | ||
get { | ||
complete { | ||
StatusCodes.OK | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
"Instrumentation directives" should "capture metrics" in { | ||
withStatsD { | ||
Get("/ping") ~> testRoute ~> check { | ||
status shouldEqual StatusCodes.OK | ||
} | ||
} { capturedMetrics => | ||
val expected = expectedHttpRequestMetrics("get", "ping", StatusCodes.OK.intValue, 1) | ||
capturedMetrics should contain allElementsOf expected | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the code in this module is agnostic to the backend reporter -- our code is just concerned with writing to the in-memory
MetricRegistry
(provided by metrics-scala/dropwizard). Then we happen to usemetrics-scala
to pull off the MetricsRegistry and write to statsd, but we could conceivably use something else besides statsd.As I'm thinking about this -- the
workbench-metrics
includes themetrics-statsd
dependency but it's only used in Rawls here. So technically we could remove themetrics-statsd
dependency fromworkbench-metrics
. But since we're standardizing on statsd, it might be better to leave it in this module for convenience sake.Sorry for the long-winded way of saying "this is fine".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funnily enough I was wondering if that block of code in Rawls should be moved up into this library. We should decide one way or another if this library should be "drop in and you get statsd reporting" or "drop in and you get metrics but wire up your own backend", and do whichever the right thing is based on that.
Opinions? I lean toward the former.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - I also like the idea of moving
into this library somewhere. It would be a convenience for other modules depending on this library (they wouldn't need to wire up statsd themselves).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything else that needs to be yanked over?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's it in terms of existing code.
One other thought, Rawls has this config:
https://github.com/broadinstitute/firecloud-develop/blob/dev/configs/rawls/rawls.conf.ctmpl#L134-L145
Currently it's all parsed in
Boot.scala
, but maybe we could make aMetricsConfig
case class and put it inworkbench-metrics
. Maybe even add logic for parsing it from a .conf file, though I'm not sure if we should assume all modules are using typesafe-config.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's reasonable to let users fill in
startStatsDReporter
themselves. Similarly providing prefix etc.