Skip to content
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

Opt-in default shared registry #801

Merged
merged 3 commits into from Jun 12, 2015

Conversation

ARentz07
Copy link

In #334, it was mentioned that the default metric registry has disappeared. While it makes sense to disallow a "default" default registry to prevent having a dumping ground for all metrics, this unfortunately creates a bit more complexity for multilayered services. If there's a shared registry of metrics between them, for instance, each of the pieces that make up a service would need to be cognisant of this default registry's name, even if the name has no semantic meaning.

In order to reduce complexity, we are aiming to expose a method to set a default registry name and allow consumers to call into SharedMetricRegistries to access this default registry. This enables a single orchestrator (such as a service container) to initialize the default registry with whatever name is suitable, leaving other pieces to just call SharedMetricRegistries.getDefault() in order to access this default registry.

This comes with a couple of caveats obviously:

  1. This method should only be called once (hence the IllegalStateException). It should be recommended somewhere in the contract, but as this class has no Javadoc on the methods, I have not included that recommendation yet.
  2. The name must be set before calling getDefault(). This is akin to the above in which we are trying to guard against consumers making bad assumptions about the state of their metrics registries. The server container, for instance, should set up the default metric registry before spinning up any workers that would access the default registry.
  3. Currently, clear() still clears the default registry. To me this is dubious, since this functionality gives the caller power to blow away the shared default registry that might be needed by other threads, etc. Comments on this are more than welcome as we need to decide what to do in this situation.

Tests are written. I'll add them to the pull request once this implementation code is reviewed.

@mattnelson
Copy link

+1

This is something that I was sorely missing when attempting to uplift to 3.x. I have quite a few instrumented libraries that I do not have the option of injecting the default registry without considerable refactoring.

@ARentz07
Copy link
Author

I've added the unit tests for this. Any chance it can get reviewed/merged?

@ryantenney
Copy link
Contributor

Thanks! I've reviewed it, and it is under consideration for the next major version (v4, no eta as of yet).

@@ -41,4 +43,20 @@ public static MetricRegistry getOrCreate(String name) {
}
return existing;
}

public synchronized static MetricRegistry setDefault(String name) {

Choose a reason for hiding this comment

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

Should there be another method for instances when you already have the MetricRegistry? I am using Dropwizard where a metrics registry is created by the framework and I would like to make that instance the default.

As proposed:

SharedMetricRegistries.add("default", bootstrap.getMetricRegistry());
SharedMetricRegistries.setDefault("default");

With the overloaded setDefault method.

SharedMetricRegistries.setDefault("default", bootstrap.getMetricRegistry());

Copy link
Author

Choose a reason for hiding this comment

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

Probably so, will add that...

Copy link
Author

Choose a reason for hiding this comment

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

Added the new method + tests.

@ryantenney ryantenney added this to the 4.0.0 milestone Jun 12, 2015
ryantenney added a commit that referenced this pull request Jun 12, 2015
@ryantenney ryantenney merged commit 5946475 into dropwizard:master Jun 12, 2015
@ryantenney
Copy link
Contributor

Merged, thanks!

@mattnelson
Copy link

@ryantenney It looks like there is going to be a 3.2 release to coincide with dropwizard 0.9. Do you think this feature could be added to the 3.2 milestone?

dropwizard/dropwizard#1137 (comment)

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.

None yet

4 participants