Skip to content

Add MetricRegistry factory#35

Merged
kristomasette merged 2 commits intoComcast:masterfrom
derjust:master
Dec 15, 2015
Merged

Add MetricRegistry factory#35
kristomasette merged 2 commits intoComcast:masterfrom
derjust:master

Conversation

@derjust
Copy link
Member

@derjust derjust commented Dec 10, 2015

This patchset allows to configure a different way on how
MetricRegistry objects are created:

Up to now these were created within money itself with no exposition to
the rest of the system. That makes it impossible to register other
Dropwizzard reporters or exporters on it.

With this patchset it is now possible to provide a custom MetricRegistry
factory and do the required initialization of/with the MetricRegistry
object.

It is compatible to Java so even Java clients (using the Scala money)
can implement and configure a custom factory.

@codecov-io
Copy link

Current coverage is 95.33%

Merging #35 into master will increase coverage by +0.05% as of 1af58d3

@@            master     #35   diff @@
======================================
  Files           35      36     +1
  Stmts          891     901    +10
  Branches        91      91       
  Methods          0       0       
======================================
+ Hit            849     859    +10
  Partial          0       0       
  Missed          42      42       

Review entire Coverage Diff as of 1af58d3

Powered by Codecov. Updated on successful CI builds.

This patchset allows to configure a different way on how
MetricRegistry objects are created:
Up to now these were created within money itself with no exposition to
the rest of the system. That makes it impossible to register other
Dropwizzard reporters or exporters on it.
With this patchset it is now possible to provide a custom MetricRegistry
factory and do the required initialization of/with the MetricRegistry
object.
It is compatible to Java so even Java clients (using the Scala money)
can implement and configure a custom factory.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking since we defined the Default in reference.conf and ensure the DefaultMetricRegistryFactory is in the classpath by including it in the library we should probably log loudly and move on if there is an error.

There should be no way we hit that Fauilure case unless something is really wrong right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Got your point.
Followed the pattern from the overall Money initialization: https://github.com/Comcast/money/pull/19/files#r44557079

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I guess it makes sense to stop and yell loudly

Copy link
Contributor

Choose a reason for hiding this comment

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

actually can this lazy val realFactory be at the MetricRegistryFactory level?

That way it gets built once the first time its accessed.

@kristomasette
Copy link
Contributor

👍

kristomasette added a commit that referenced this pull request Dec 15, 2015
Add MetricRegistry factory
@kristomasette kristomasette merged commit 464e453 into Comcast:master Dec 15, 2015
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.

3 participants