feat: add custom micrometer registries via NeonBeeConfig

[s4heid]

With this change the config parameter micrometerRegistries is
introduced which allows the user to specify a list of (full qualified) class
names, which must implement the load() method of the functional interface
MicrometerRegistryLoader. The load method must return a
MeterRegistry, which will be added to the micrometer registries by
NeonBee. The PrometheusMeterRegistry will be the default
registry, which is only available when the MetricsEndpoint is loaded.

[pk-work]

This change is doing far more then only load the NeonBeeConfig earlier during the NeonBee creation process. It introduces a complete new mechanism to register metrics.

You should describe this in the commit message, or even add some docu how I can use this new mechanism. Whats the benefit of creating own registries? Can I add with this my own metrics? Are they automatically visible to the endpoint?

[kristian]

Hey everyone! Happy new year! 🥂🎆

@halber I really like where this change is going and it introduces a lot of simplifications / nicer signatures that I like. Good work so far! I was about to do my initial review on the change, but I unfortunately have to open another round of discussion here for everyone:

@pk-work, @s4heid and @halber After reviewing the whole change, what I really do dislike is, that we previously made efforts of removing all blocking code. Now with this change we are re-introducing it across multiple classes like NeonBeeConfig, the ConfigHelper etc, alongside of all the new code that is needed to test the synchronous stuff... With that being said, wouldn't be better of doing it like it was suggested in vertx-config in the first place? See [1]. More specifically (to compare):

We could introduce a empty "load()" method in NeonBeeConfig that intern loads a NeonBeeConfig using a new Vert.x instance and closes it immediately, alongside:

public static Future<NeonBeeConfig> load() {
   Vertx vertx = Vertx.vertx();
   return load(vertx).eventually(nothing -> vertx.close());
}

Advantage is that is introduces zero additional code & just a very little more test code.

Then in the NeonBee we just have to make sure, the temporary Vert.x instance is initialized after we set the logging stuff in the create method. I already spoke to @s4heid, is is anyways not nice that in create() we do static initialization here [2]. This results in that initialize multiple loggers when creating multiple NeonBee instances. Thus I would suggest, let's move the code of [2] to a "more general" place, keeping the new create(Supplier<Future<Vertx>>, NeonBeeOptions, NeonBeeConfig) signature of create. E.g.:

static volatile Logger logger;

static void initialize(NeonBeeOptions options) {
    // old code ...

    if (logger === null) {
       synchronized(NeonBee.class) {
         if (logger === null) {
           // old logging initialization here
        }
      }
   }
}

// ...

static Future<NeonBee> create(Supplier<Future<Vertx>> vertxFutureSupplier, NeonBeeOptions options) {
   // static initializer, e.g. for logging
   initialize(options);

   return NeonBeeConfig.load().compose(config -> create(vertxFutureSupplier, options, config));
}

static Future<NeonBee> create(Supplier<Future<Vertx>> vertxFutureSupplier, NeonBeeOptions options,
            NeonBeeConfig config) {
   // static initializer, e.g. for logging
   initialize(options);
}

I am a little unsure about the initialize method, or if there is a nicer way to do this? However all in all, I think doing it with a own temporary Vert.x instance could spare us a lot of otherwise "dead" code. As we would have to clearly mark the new synchronous readYAML, readJSON, etc. methods as to "NEVER USE" anyways, because the asynchronous methods must be favored at all time anyways. What do you think?

If you say we keep the current synchronous way, I will finish my review of the existing code, if we say we switch to the temporary Vert.x instance instead, I would like to wait for @halber to do the change first.

Sorry for opening another round of discussion on this, even after @halber did invest a lot of work already, but sometimes you only start to see things clearly, if you see what impact is had on the code.

[1] https://vertx.io/docs/vertx-config/java/#_configuring_vert_x_itself
[2] https://github.com/SAP/neonbee/blob/main/src/main/java/io/neonbee/NeonBee.java#L218

[s4heid]

Hi @kristian,

thanks for your detailed feedback. There's a few things which came to my mind when reading your comment.

1. blocking load methods: In my opinion, loading the NeonBeeConfig upfront (blocking) makes sense in general as it is required before startup and we have to wait anyways. However, I agree with you that having the large amount of newly introduced blocking methods is not nice. Nobody else should use them, as they are only required for loading the config.

2. initializing a temporary vertx: I would like to avoid this for multiple reasons, if possible. In one of the experiments of @halber it turned out that it takes a while until the vertx is closed again. So either we have two vertx instances running at the same time (which throws a warning from vertx), or we have to wait until the this vertx instance is closed again (I'm not aware of the actual duration, though -> needs to be checked), which might impact performance. In the first case, I'd be concerned that we run into some side-effects of having multiple vertx instances at the same time.

3. static initialization: I think it is a good idea to do this. In order keep the change small we might do this in a separate change or refactor commit. If we decide to do it separately, I will create a follow-up issue.

---

After a discussion with @halber this morning, it seems that adding registries to the CompositeMeterRegistry is also possible during runtime. With this finding, we

• don't have to introduce any blocking methods (1.) -> the changes for loading the NeonBeeConfig blocking can be reverted and we simply add the registries at a later point in time,
• don't need the temporary vertx (2.), and
• the create() method can remain as it is more or less.

@kristian If you don't mind, please wait with your review until the change has been adapted. I guess it will be quite different thereafter.

Best Regards,
Sebastian

[kristian]

I agree to all your points @s4heid and @halber.

In regards to 2., I get your point here. What Vert.x does is to close it's underlying Netty thread pools, which is why it takes some time to complete. As starting multiple Vert.x's side-by-side is actually not a problem at all and (if?) only closing Vert.x introduces the slowdown, we could also close the initial Vert.x instance "fire and forget" (most cleanly, we could just resolve the promise in our shutdown hook).

Generally I really do like to have loaded the config upfront, we will run into situations again, where we will require to have it upfront... so scrapping all of @halber 's work is maybe not the best idea (yet?). I really do like the idea, as well as the simplification in the method signatures introduced.

I just see the disadvantage of 1.: introducing one-purpose blocking methods, which should / must never to be used again. How to you like the fire & forget Vert.x close suggestion?

[s4heid]

@kristian I like your suggestion and I think that your approach could be useful for other things as well. However, I am afraid that it will blow up this change if we do not separate it into a different change / discussion. Therefore, I created #95. Feel encouraged to leave your comments there.

[kristian]

Should we rather provide the vertxFutureSupplier (renamed to vertxFactory) with the whole NeonBeeOptions and the factory can do whatever with it? this would leave the create method more clean with lines 221-227 and also resolve the naing conflict with vertxOptions and neonBeeOptions?

[halber]

Maybe we can change this if we Load NeonBee options upfront. Currently I have to set the same CompositeMeterRegistry in the VertxOptions and in the NeonBeeConfig. If we had the NeonBeeConfig before we create the Vertx object, then I could easily change this.

[kristian]

Why don't we keep this in here and rahter move the code down from the create() method into here?

[halber]

see #94 (comment)

[kristian]

remove, the NeonBeePrometheusMeterRegistry should be set as default in the NeonBeeConfig (instead of initializing the microMeterRegistries with a private List<String> registries = List.of(); initialize it with a List.of("io.neonbee.endpoint.metric.NeonBeePrometheusMeterRegistry"). This way it is clear (from a config perspective) which registies get added, it removes the requirement to (with some dubious instance variable) pass the composite metrics registry around via the getConfig() method and allows to not add the NeonBeePrometheusMeterRegistry if it is unwanted by the user. If there are no registries set in the config, the metrics endpoint should not get deployed in the NeonBee class (this way it can be disabled).

The downside of this is: How do you pass configuration from the config into the registry? I think later we should not only have microMeterRegistries be a List<String>, but add a new MicroMeterRegistryConfig object and make it a List<MicroMeterRegistryConfig>, that also allows to pass a JSON of options towards the registry implementation.

[halber]

This is kind of the exact opposite of what I had discussed with @pk-work directly. The underlying discussion was this one here #94 (comment).

We should filter by ID otr name or something like that, to ensure that we ALWAYS get the default PrometheusMeterRegistry. Otherwise we don't know which PrometheusMeterRegistry we get in case that we also use one or multiple custom regestries of type PrometheusMeterRegistry

I can't filter the PrometheusMeterRegistry at this point and would therefore suggest that if you configure your own PrometheusMeterRegistry the default PrometheusMeterRegistry in the NeonBeeConfig is not added. If someone really has the case that he has two PrometheusMeterRegistry he would have to solve this himself by writing code.

I added the NeonBeePrometheusMeterRegistry class to filter for this registry. I also moved the default initialization of the PrometheusMeterRegistry from the NeonBeeConfig to the MetricsEndpoint. Only when the MetricsEndpoint is loaded, the default NeonBeePrometheusMeterRegistry is added to the CompositeMeterRegistry.

Cool! I'm happy that we had a talk.

[kristian]

@halber I will now look into your comments. sorry for the delay. let us close this one soon :)

[kristian]

lgtm! 👍