Retain and expose managed export metadata#71
Conversation
| return new Mapping(key, (exporter, objectNameGenerator, object) -> { | ||
| Class<?> type = key.getTypeLiteral().getRawType(); | ||
| if (generatedName.isPresent()) { | ||
| exporter.exportWithGeneratedName(object, type, generatedName.get()); |
There was a problem hiding this comment.
Latent seam worth a note (applies equally to SetMapping.generatedName / MapMapping.generatedName): after this refactor the generated-name paths defer name generation to exporter.exportWithGeneratedName(...), which uses the exporter's own ObjectNameGenerator and ignores the objectNameGenerator threaded through GuiceMBeanExporter. The custom-function paths (.as(...), objectNameFunction) still use the threaded one.
Today these are always the same object — both MBeanExporter and GuiceMBeanExporter resolve from the same Optional<ObjectNameGenerator> OptionalBinder — so there's no live bug. But it introduces a silent invariant: if MBeanExporter is ever bound to an instance whose generator differs from the Optional<ObjectNameGenerator> binding, the two mapping kinds would generate names from different generators with no error. Worth either a one-line comment documenting the invariant, or making the exporter the single source of truth and dropping the now-redundant generator threading on these paths. (Minor reinforcing detail: defaultObjectNameGenerator() returns a fresh lambda per call, so absent a custom binding the two are already distinct instances — harmless only because the default is stateless.)
Runnable repro
Two generators put MBeans in different JMX domains: A ("from.binding") bound via the documented OptionalBinder<ObjectNameGenerator>, B ("from.exporter") baked into the MBeanExporter instance. Two generated-name exports in one module diverge:
ObjectNameGenerator generatorA = (type, props) -> "from.binding:name=" + type.getSimpleName(); // configured
ObjectNameGenerator generatorB = (type, props) -> "from.exporter:name=" + type.getSimpleName(); // exporter's own
Injector injector = Guice.createInjector(Stage.PRODUCTION,
Modules.override(new MBeanModule()).with(new AbstractModule() {
@Override protected void configure() {
bind(MBeanServer.class).toInstance(server);
newOptionalBinder(binder(), ObjectNameGenerator.class).setBinding().toInstance(generatorA);
bind(MBeanExporter.class).toInstance(new MBeanExporter(server, Optional.of(generatorB)));
bind(Foo.class).toInstance(new Foo());
bind(Bar.class).toInstance(new Bar());
ExportBinder.newExporter(binder()).export(Foo.class).withGeneratedName(); // -> exporter's generator B
ExportBinder.newExporter(binder()).export(Bar.class).as(f -> f.generatedNameOf(Bar.class)); // -> threaded generator A
}
}));
Map<ObjectName, ?> exports = injector.getInstance(MBeanExporter.class).getManagedObjectExports();
// Foo via .withGeneratedName() -> from.exporter
// Bar via .as(f -> f.generatedNameOf(Bar)) -> from.bindingTwo equivalent generated-name exports in one module, two different generators, no error. Pre-refactor GuiceMBeanExporter computed every name itself with the threaded generator A, so Foo would also have landed in from.binding — i.e. this exact wiring was safe before and silently changes behavior after. (It does require overriding the exporter binding, hence "latent," not a live bug under plain MBeanModule usage.)
There was a problem hiding this comment.
Ah I didn't notice that. I fixed the code to use the supplied name generator.
| Map<String, String> generatedProperties) | ||
| { | ||
| try { | ||
| requireNonNull(objectName, "objectName is null"); |
There was a problem hiding this comment.
These null checks read better before the try: the resulting NPE isn't matched by any of the catch clauses below (so behavior is correct either way), and the exportWithGeneratedName overloads already validate up front. Minor.
| /** | ||
| * Original generated-name argument supplied to {@code exportWithGeneratedName}, if any. | ||
| */ | ||
| public Optional<String> getGeneratedName() |
There was a problem hiding this comment.
Worth clarifying here that this captures the name argument passed to the generator, not the fact that a name was generated. withGeneratedName(ObjectNameFunction) / MapObjectNameFunction route through as(...), so they leave this empty even though the user called a withGeneratedName method — a consumer assuming "exported via withGeneratedName ⇒ generatedName present" would be surprised. The asymmetry is defensible (an ObjectNameFunction supplies a complete ObjectName, not a name fragment), so a doc sentence rather than a code change.
There was a problem hiding this comment.
I renamed this to "originalName" and "originalProperties". I think "generated" is just too overloaded to use here and is just confusing..
Systems such as OpenMetrics and OpenTelemetry have their own metric naming conventions. They need the original Java type and generated-name inputs to build those names, while still using the final ObjectName for identity and labels. ObjectNameGenerator collapses that context into a final ObjectName, so consumers otherwise have to reverse-engineer naming intent from JMX properties. Retain the export metadata beside ManagedClass to make that intent available directly.
a0a83c8 to
c53f685
Compare
Why
ObjectName, which forces consumers to reverse-engineer naming intent from JMX properties.ObjectNamebecause configured generators may rewrite domains or add identity properties used as labels.Approach
ObjectName.ManagedClasswhen available.getManagedClasses()API as a compatibility projection.