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

Add facility to specify a custom MetricRegistry in Bootstrap #1015

Merged
merged 1 commit into from May 19, 2015

Conversation

Projects
None yet
4 participants
@arteam
Member

arteam commented Apr 26, 2015

  • Add a setter for the metric registry in Bootstrap, so users can provide own customized instance during initialization.
  • Move system metrics registration into a separate method from the constructor.
  • Invoke the method after Application#initialize, where users can customize Bootstrap.

Resolve #956

@arteam

This comment has been minimized.

Member

arteam commented May 9, 2015

@dropwizard/committers, It would be great, if someone reviewed this patch.

@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented May 11, 2015

@arteam sorry it took so long to look. Thanks for looking at this.

The only thing I'm concerned with is the reflection. It's easier to find references when it's statically referenced.

What is the exact security concern with making registerMetrics public? If Application needs to call it, then I think it should be part of the Bootstrap interface.

@joschi

This comment has been minimized.

Member

joschi commented May 11, 2015

I also have some trouble with using reflection in this case. The problem is that it would fail only at runtime if things broke.

Maybe registerMetrics() should be called implicitly when providing an alternative MetricRegistry, i. e. rename setMetricRegistry() to registerMetricRegistry() or initializeMetricRegistry() and call registerMetrics() in that method. The can still be a (package protected) setMetricRegistry() if that would be required for tests.

@joschi joschi added this to the 0.9.0 milestone May 11, 2015

@vanDonselaar

This comment has been minimized.

vanDonselaar commented May 11, 2015

Another option could be to use the SharedMetricRegistries. It's not as nice as proper dependency injection (it's a singleton class), but at least it doesn't require reflection.

@arteam

This comment has been minimized.

Member

arteam commented May 11, 2015

Maybe registerMetrics() should be called implicitly when providing an alternative MetricRegistry, > > i.e. rename setMetricRegistry() to registerMetricRegistry() or initializeMetricRegistry() and call
registerMetrics() in that method. The can still be a (package protected) setMetricRegistry() if that
would be required for tests.

The problem is that we need to stop the default JMX reporter and create a new one, which adds complexity.

What is the exact security concern with making registerMetrics public? If Application needs to call > it, then I think it should be part of the Bootstrap interface.

I thought that registering system metrics should be transparent to user code. Basically users should
not see this method, because it must be called only by Application. For example, someone can call this method in Application#initialize and break the startup process.

Anyway, I am more fond to make registerMetrics public. But I think we can make it idempotent,
by returning sucessfully if the system metrics are already registered.

Add facility to specify a custom MetricRegistry in Bootstrap
* Add a setter for the metric registry in Bootstrap, so users
 can provide own customized instance during initialization.
* Move system metrics registration into a separate method
from the constructor.
* Invoke the method after Application#initialize, where users
can customize Bootstrap.

Resolve #956
@carlo-rtr

This comment has been minimized.

Member

carlo-rtr commented May 19, 2015

Thanks @arteam

carlo-rtr added a commit that referenced this pull request May 19, 2015

Merge pull request #1015 from arteam/custom-metric-registry
Add facility to specify a custom MetricRegistry in Bootstrap

@carlo-rtr carlo-rtr merged commit 5da6c02 into dropwizard:master May 19, 2015

@arteam arteam deleted the arteam:custom-metric-registry branch Jan 24, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment