palominolabs/metrics-guice

Documentation not clear about module load order

bstempi opened this issue · 5 comments

Hi,
I have a case where I want the MetricRegistry to be managed by a provider (in case someone else other than the MetricsInstrumentationModule needs it). In order to achieve this, I first create an injector without the MetricsInstrumentationModule, I get the MetricRegistry from the injector, and then I create a child injector, adding the MetricsInstrumentationModule with the MetricRegistry that I pulled from the parent injector. Here's a code example:

    Injector injector = Guice.createInjector(
        new AModule(),
        new BModule()
    );

    // We need stuff from the current injector to install the next module
    MetricRegistry registry = injector.getInstance(MetricRegistry.class);
    injector = injector.createChildInjector(new MetricsInstrumentationModule(registry));

Will this work? Will I bump into issues due to the load-order?

That looks like it should work fine to me. However, if your MetricRegistry configuration is straightforward, you could create it manually and then simply bind the instance so that you could both provide it for other Guice injectables and use it MIModule.

If that wouldn't work well for you, I'd be happy to hear your thoughts on how I can better support this use case. This isn't something that I can really do with a @Provides method, unfortunately, since the work is really in binding type listeners, not in providing bindings, so I couldn't simply depend on a MetricRegistry instance being passed to a @Provides method.

RE: My approach working: It doesn't. My metrics registry is empty and I'm not sure why. I'll have to investigate further or see if I can find some other example code. I'll try nesting the MIModule within the module that provides the MR, etc.

RE: Better support for existing MRs: I've only been using Guice for a few months, so I don't think I can comment too much. I think the main problem is that I usually try not to load modules from within other modules (keeping it flat makes it much easier to reason about), and that's just not doable here without some other wizardry that'd be even more confusing. Perhaps in this case, nesting isn't a bad thing.

Ok, so after another attempt, I have some feedback.

The reason that I need a reference to the MetricRegistry is so I can report to Graphite. Sure, I could instantiate a MR in outside of my modules and pass it in as an argument, but this just doesn't seem right -- why am I instantiating things for my DI framework?

RE: My second approach: I wraped the MIModule with another module that has a static MR and a provider that returns that MR. For whatever reason, that MR is always empty, as if the Listeners that the MIModule binds aren't being invoked. The MIModule is the first module that loads, so the Listeners should be bound before the others load. I'm not sure what's going on there. I'll keep digging.

Ok, I got it to work. Here's my module to wrap the MIModule:

public class MetricModule extends AbstractModule {

private static MetricRegistry METRIC_REGISTRY = new MetricRegistry();

@Override
protected void configure() {
    install(new MetricsInstrumentationModule(METRIC_REGISTRY));
}

@Provides
@Singleton
public MetricRegistry providesMetricRegistry() {
    return METRIC_REGISTRY;
}

RE: Better support for sharing the MR: Having a shared dependency in the constructor is a killer. More specifically, this whole thing is made difficult by the fact that the Listeners expect to have a reference to an MR. Perhaps when constructing the interceptors we can use the TypeEncounter within the Listeners to get a provider for the MR. I have to dig a bit further into the Guice API and experiment a bit to see if this works. I'm not entirely sure when I'm allowed to get Providers since I'm not sure where Listeners and the like fall into the Guice life cycle.

I'm glad you got it to work!

A few comments:

  • There's no reason why you need a static field there, and static or not it should probably be final. :)
  • I agree that creating things yourself outside of your IoC container is kind of a smell. It's often a smell that I just live with rather than torturing my module structure too badly, but it's not necessarily a bad thing to have multiple levels of injectors to handle that sort of problem (as you were working towards in your initial approach).
  • Really the core difficulty here is that there isn't a good way to register those listeners aside from in a configure() method. It would be nice if Guice exposed a way to participate in the dependency graph in the way that @Provides methods can via their method arguments for listeners. I'm not sure what that would look like, which is probably the same reason why they didn't do it. ;)