Skip to content

Conversation

@derjust
Copy link
Member

@derjust derjust commented Dec 18, 2015

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.

@derjust derjust force-pushed the master branch 4 times, most recently from b28fea1 to 4a08773 Compare January 4, 2016 20:46
@codecov-io
Copy link

Current coverage is 95.23%

Merging #39 into master will decrease coverage by -0.10% as of 6d57443

@@            master     #39   diff @@
======================================
  Files           36      36       
  Stmts          901     902     +1
  Branches        91      91       
  Methods          0       0       
======================================
  Hit            859     859       
  Partial          0       0       
- Missed          42      43     +1

Review entire Coverage Diff as of 6d57443

Powered by Codecov. Updated on successful CI builds.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be: getLogger("com.comcast.money.metrics.MetricRegistryExtensionImpl")

@kristomasette
Copy link
Contributor

one minor nit to pick, but otherwise, looks good

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use /* instead of multiple lines of //

@derjust
Copy link
Member Author

derjust commented Jan 6, 2016

Addressed @kristomasette and @paulcleary comments

@kristomasette
Copy link
Contributor

👍

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.
kristomasette added a commit that referenced this pull request Jan 8, 2016
Improve MetricRegistryFactory configuration
@kristomasette kristomasette merged commit 5af5648 into Comcast:master Jan 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants