From 257751164a371fea3d1b62746512b515be6969b3 Mon Sep 17 00:00:00 2001 From: Sebastian Just Date: Fri, 18 Dec 2015 15:43:55 -0500 Subject: [PATCH 1/2] Improve MetricRegistryFactory configuration The current appraoch requires two properties to be set to enable a custom MetricRegistryFactory: ``` metrics-registry { class-name = "com.acme.CustomMetricRegistryFactory" } emitter { emitters = [ { name = "span-metrics-emitter" class-name = "com.comcast.money.metrics.SpanMetricsCollector" subscriptions = [Trace] configuration = { metrics-registry { class-name = "com.acme.CustomMetricRegistryFactory" } } } ] } ``` With this change, the configuration simplifies down to ``` metrics-registry { class-name = "com.acme.CustomMetricRegistryFactory" configuration = { } } ``` for the money system and providing a custom 'configuration' section for the factory. Also, as it is now an Akka extension, the factory is created only once for the whole system. --- money-core/src/main/resources/reference.conf | 2 ++ .../money/metrics/MetricRegistryFactory.scala | 30 +++++++++++++---- .../comcast/money/metrics/MoneyMetrics.scala | 2 +- .../comcast/money/metrics/SpanMetrics.scala | 3 +- .../metrics/MetricRegistryFactorySpec.scala | 32 +++++++++---------- 5 files changed, 44 insertions(+), 25 deletions(-) diff --git a/money-core/src/main/resources/reference.conf b/money-core/src/main/resources/reference.conf index 3b123fc0..16927cee 100644 --- a/money-core/src/main/resources/reference.conf +++ b/money-core/src/main/resources/reference.conf @@ -41,6 +41,8 @@ money { metrics-registry { class-name = "com.comcast.money.metrics.DefaultMetricRegistryFactory" + configuration = { + } } emitter { diff --git a/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala b/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala index 3bba1d9b..ce009fa6 100644 --- a/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala +++ b/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala @@ -17,7 +17,7 @@ package com.comcast.money.metrics import com.typesafe.config.Config -import akka.actor.{ ActorSystem } +import akka.actor.{ ActorSystem, ExtendedActorSystem, Extension, ExtensionId, ExtensionIdProvider } import com.codahale.metrics.{ MetricRegistry, JmxReporter } import org.slf4j.LoggerFactory import scala.util.{ Try, Success, Failure } @@ -35,18 +35,19 @@ import scala.util.{ Try, Success, Failure } * Note: This trait should be kept as simple as possible so that the resulting interface can also be implemented * by a Java client custom factory. */ -object MetricRegistryFactory { - private val logger = LoggerFactory.getLogger("com.comcast.money.metrics.MetricRegistryFactory") - def metricRegistry(config: Config): MetricRegistry = { +class MetricRegistryExtensionImpl(config: Config) extends Extension { + private val logger = LoggerFactory.getLogger("com.comcast.money.metrics.MetricRegistryExtension") + + val metricRegistry: MetricRegistry = { Try({ // Try to create an instance of the custom factory, configured via 'metricRegistryFactory' lazy val realFactory = Class.forName(config.getString("metrics-registry.class-name")) .newInstance.asInstanceOf[MetricRegistryFactory] - // Ask the custom factory for an MetricRegistry - and pass in our configuration so that an implementation + // Ask the custom factory for an MetricRegistry - and pass in the configuration so that an implementation // can add their settings in the application.conf, too. - realFactory.metricRegistry(config) + realFactory.metricRegistry(config.getConfig("metrics-registry.configuration")) }) match { case Success(metricRegistry) => metricRegistry case Failure(e) => { @@ -59,6 +60,23 @@ object MetricRegistryFactory { } } +object MetricRegistryExtension extends ExtensionId[MetricRegistryExtensionImpl] with ExtensionIdProvider { + //The lookup method is required by ExtensionIdProvider, + // so we return ourselves here, this allows us + // to configure our extension to be loaded when + // the ActorSystem starts up + override def lookup = MetricRegistryExtension + + //This method will be called by Akka + // to instantiate our Extension + override def createExtension(system: ExtendedActorSystem) = new MetricRegistryExtensionImpl(system.settings.config) + + /** + * Java API: retrieve the extension for the given system. + */ + override def get(system: ActorSystem): MetricRegistryExtensionImpl = super.get(system) +} + class DefaultMetricRegistryFactory extends MetricRegistryFactory { override def metricRegistry(config: Config): MetricRegistry = { val registry = new MetricRegistry diff --git a/money-core/src/main/scala/com/comcast/money/metrics/MoneyMetrics.scala b/money-core/src/main/scala/com/comcast/money/metrics/MoneyMetrics.scala index da4fbc56..984a9fc4 100644 --- a/money-core/src/main/scala/com/comcast/money/metrics/MoneyMetrics.scala +++ b/money-core/src/main/scala/com/comcast/money/metrics/MoneyMetrics.scala @@ -50,7 +50,7 @@ object MoneyMetrics // to instantiate our Extension override def createExtension(system: ExtendedActorSystem) = { - val registry = MetricRegistryFactory.metricRegistry(system.settings.config) + val registry = MetricRegistryExtension(system).metricRegistry // register metrics val activeSpans = registry.counter("active.spans") val timedOutSpans = registry.counter("timed.out.spans") diff --git a/money-core/src/main/scala/com/comcast/money/metrics/SpanMetrics.scala b/money-core/src/main/scala/com/comcast/money/metrics/SpanMetrics.scala index aaeedec0..ca985a4e 100644 --- a/money-core/src/main/scala/com/comcast/money/metrics/SpanMetrics.scala +++ b/money-core/src/main/scala/com/comcast/money/metrics/SpanMetrics.scala @@ -52,10 +52,9 @@ class SpanMetricsCollector(conf: Config) extends Actor with ActorMaker with Acto case Some(spanMetrics) => spanMetrics forward t case None => - val metricRegistry = MetricRegistryFactory.metricRegistry(conf) val escapedName = t.spanName.replace(' ', '.') log.debug(s"Creating span metrics for span $escapedName") - makeActor(SpanMetrics.props(escapedName, metricRegistry), escapedName) forward t + makeActor(SpanMetrics.props(escapedName, MetricRegistryExtension(context.system).metricRegistry), escapedName) forward t } } } diff --git a/money-core/src/test/scala/com/comcast/money/metrics/MetricRegistryFactorySpec.scala b/money-core/src/test/scala/com/comcast/money/metrics/MetricRegistryFactorySpec.scala index 4f20384a..138752aa 100644 --- a/money-core/src/test/scala/com/comcast/money/metrics/MetricRegistryFactorySpec.scala +++ b/money-core/src/test/scala/com/comcast/money/metrics/MetricRegistryFactorySpec.scala @@ -33,7 +33,7 @@ class MockMetricRegistryFactory extends MetricRegistryFactory { def metricRegistry(config: Config): MetricRegistry = MockMetricRegistryFactory.mockRegistry } -class MetricRegistryFactorySpec extends WordSpecLike with BeforeAndAfter with MockitoSugar { +class MetricRegistryExtensionSpec extends WordSpecLike with BeforeAndAfter with MockitoSugar { private val conf = mock[Config] @@ -43,33 +43,33 @@ class MetricRegistryFactorySpec extends WordSpecLike with BeforeAndAfter with Mo doReturn("com.comcast.money.metrics.DefaultMetricRegistryFactory").when(conf).getString("metrics-registry.class-name") - val registry = MetricRegistryFactory.metricRegistry(conf) + val registry = new MetricRegistryExtensionImpl(conf).metricRegistry registry shouldNot be(null) } } - } - "fall back to the DefaultMetricRegistryFactory" should { - "when the config is broken" in { + "fall back to the DefaultMetricRegistryFactory" should { + "when the config is broken" in { - doReturn("lorem ipsum").when(conf).getString("metrics-registry.class-name") + doReturn("lorem ipsum").when(conf).getString("metrics-registry.class-name") - intercept[ClassNotFoundException] { - val registry = MetricRegistryFactory.metricRegistry(conf) + intercept[ClassNotFoundException] { + val registry = new MetricRegistryExtensionImpl(conf).metricRegistry + } } } - } - "use the MockMetricRegistryFactory" should { - "when configured so" in { + "use the MockMetricRegistryFactory" should { + "when configured so" in { + doReturn("com.comcast.money.metrics.MockMetricRegistryFactory").when(conf).getString("metrics-registry.class-name") - doReturn("com.comcast.money.metrics.MockMetricRegistryFactory").when(conf).getString("metrics-registry.class-name") + val ext = new MetricRegistryExtensionImpl(conf) + val registry1 = ext.metricRegistry + val registry2 = ext.metricRegistry - val registry1 = MetricRegistryFactory.metricRegistry(conf) - val registry2 = MetricRegistryFactory.metricRegistry(conf) - - registry1 should be theSameInstanceAs registry2 + registry1 should be theSameInstanceAs registry2 + } } } } From 3551ab33b891af339fe432598f434a8cc754ff4e Mon Sep 17 00:00:00 2001 From: Sebastian Just Date: Wed, 6 Jan 2016 13:26:49 -0500 Subject: [PATCH 2/2] Updated comment formatting --- .../money/metrics/MetricRegistryFactory.scala | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala b/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala index ce009fa6..a82d5982 100644 --- a/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala +++ b/money-core/src/main/scala/com/comcast/money/metrics/MetricRegistryFactory.scala @@ -35,9 +35,8 @@ import scala.util.{ Try, Success, Failure } * Note: This trait should be kept as simple as possible so that the resulting interface can also be implemented * by a Java client custom factory. */ - class MetricRegistryExtensionImpl(config: Config) extends Extension { - private val logger = LoggerFactory.getLogger("com.comcast.money.metrics.MetricRegistryExtension") + private val logger = LoggerFactory.getLogger("com.comcast.money.metrics.MetricRegistryExtensionImpl") val metricRegistry: MetricRegistry = { Try({ @@ -45,8 +44,11 @@ class MetricRegistryExtensionImpl(config: Config) extends Extension { lazy val realFactory = Class.forName(config.getString("metrics-registry.class-name")) .newInstance.asInstanceOf[MetricRegistryFactory] - // Ask the custom factory for an MetricRegistry - and pass in the configuration so that an implementation - // can add their settings in the application.conf, too. + /* + * Ask the custom factory for an MetricRegistry - and pass in the + * configuration so that an implementation can add their settings in the + * application.conf, too. + */ realFactory.metricRegistry(config.getConfig("metrics-registry.configuration")) }) match { case Success(metricRegistry) => metricRegistry @@ -61,19 +63,18 @@ class MetricRegistryExtensionImpl(config: Config) extends Extension { } object MetricRegistryExtension extends ExtensionId[MetricRegistryExtensionImpl] with ExtensionIdProvider { - //The lookup method is required by ExtensionIdProvider, - // so we return ourselves here, this allows us - // to configure our extension to be loaded when - // the ActorSystem starts up + /* + * The lookup method is required by ExtensionIdProvider, + * so we return ourselves here, this allows us + * to configure our extension to be loaded when + * the ActorSystem starts up + */ override def lookup = MetricRegistryExtension - //This method will be called by Akka - // to instantiate our Extension + // This method will be called by Akka to instantiate our Extension override def createExtension(system: ExtendedActorSystem) = new MetricRegistryExtensionImpl(system.settings.config) - /** - * Java API: retrieve the extension for the given system. - */ + // Java API: retrieve the extension for the given system. override def get(system: ActorSystem): MetricRegistryExtensionImpl = super.get(system) }