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

Possibility to use io.vertx.core.VertxBuilder inside io.vertx.core.impl.launcher.VertxLifecycleHooks #5388

Conversation

bolbat
Copy link

@bolbat bolbat commented Nov 13, 2024

Vertx 4.5.10 introduced new "Deprecations and breaking changes", especially: Deprecate setting a MeterRegistry on MicrometerMetricsOptions
https://github.com/vert-x3/wiki/wiki/4.5.10-Deprecations-and-breaking-changes
The new way to configure "MicrometerMetricsFactory" is to use io.vertx.core.VertxBuilder.withMetrics(...) method
But there is no possibility to do this from "io.vertx.core.Launcher" - because only "io.vertx.core.VertxOptions" are accessible for configuration purposes, which is not enough now and code warn about usage of deprecated "io.vertx.micrometer.MicrometerMetricsOptions.setMicrometerRegistry(MeterRegistry)"

Vertx 4.5.11 introduced new method for customizing VertxBuilder inside "io.vertx.core.impl.launcher.VertxLifecycleHooks"
#5288
But this method allowing to customize internal "io.vertx.core.impl.VertxBuilder" which is not helping to fix deprecations

Currently "io.vertx.core.VertxBuilder" have one implementation, inlined in Vertx class, which have private method "internalBuilder()" for is creating internal VertxBuilder with provided configuration and initializing it

This PR doing the next:

  1. Moving "io.vertx.core.VertxBuilder" implementation from Vertx class to io.vertx.core.impl.VertxBuilder as "VertxBuilderBridge" class (maybe name and place should be adjusted)
  2. Method internalBuilder() made public
  3. Moved internal builder init() invocation from internalBuilder() method to build() and buildClustered() methods
  4. Used "VertxBuilderBridge" class as "io.vertx.core.VertxBuilder" in Vertx class instead of inlined implementation
  5. Used "VertxBuilderBridge" class as example in "io.vertx.core.impl.launcher.VertxLifecycleHooks.createVertxBuilder(JsonObject)" default implementation

This is enough to fix new deprecations from inside Launcher API to configure all required things that now require public VertxBuilder by overriding "io.vertx.core.impl.launcher.VertxLifecycleHooks.createVertxBuilder(JsonObject)" and using it in the same way as in default implementation

io.vertx.core.VertxBuilder and possibility to create
io.vertx.core.impl.VertxBuilder from it

related code cleanup and removed code duplication
io.vertx.core.VertxBuilder and possibility to create
io.vertx.core.impl.VertxBuilder from it

related code cleanup and removed code duplication
@vietj vietj added this to the 4.5.12 milestone Nov 14, 2024
@bolbat bolbat changed the title Additional vertx builder override in vertx lifecycle hooks + related changes Possibility to use io.vertx.core.VertxBuilder inside io.vertx.core.impl.launcher.VertxLifecycleHooks Nov 14, 2024
@tsegismont
Copy link
Contributor

The new way to configure "MicrometerMetricsFactory" is to use io.vertx.core.VertxBuilder.withMetrics(...) method
But there is no possibility to do this from "io.vertx.core.Launcher" - because only "io.vertx.core.VertxOptions" are accessible for configuration purposes, which is not enough now and code warn about usage of deprecated "io.vertx.micrometer.MicrometerMetricsOptions.setMicrometerRegistry(MeterRegistry)"

Can you elaborate about why this is not possible? Maybe with a small snippet?

@bolbat
Copy link
Author

bolbat commented Nov 19, 2024

Can you elaborate about why this is not possible? Maybe with a small snippet?

Of course, details in comments in code on the screenshots

How Launcher looks after 4.5.10 release
Screenshot 2024-11-19 at 11 49 23

How Launcher looks after 4.5.11 release
Screenshot 2024-11-19 at 11 59 56

With changes provided by this pull request we will be able at least to use public VertexBuilder implementation for applying this configuration without deprecation warnings and then building from it internal VertxBuilder and then use it in VertxLifecycleHooks in new 'createVertxBuilder' method

Like I did in default implementation in this method as example

default VertxBuilder createVertxBuilder(JsonObject config) {
	  final VertxBuilderBridge vertxBuilder = new VertxBuilder.VertxBuilderBridge();
	  if (config != null)
		  vertxBuilder.with(new VertxOptions(config));

	  return vertxBuilder.internalBuilder();
  }

@tsegismont
Copy link
Contributor

It's possible to avoid the deprecated method call doing this:

  @Override
  public VertxBuilder createVertxBuilder(JsonObject config) {
    PrometheusMeterRegistry registry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT);

    CollectorRegistry prometheusClientRegistry = new CollectorRegistry();
    registry = new PrometheusMeterRegistry(PrometheusConfig.DEFAULT, prometheusClientRegistry, Clock.SYSTEM);

    VertxBuilder vertxBuilder = super.createVertxBuilder(config);

    VertxOptions options = vertxBuilder.options();

    options.setMetricsOptions(new MicrometerMetricsOptions()
      .setPrometheusOptions(new VertxPrometheusOptions().setEnabled(true)
        .setStartEmbeddedServer(true)
        .setEmbeddedServerOptions(new HttpServerOptions().setPort(8888))
        .setEmbeddedServerEndpoint("/metrics/vertx"))
      .setEnabled(true));

    vertxBuilder.metrics(new MicrometerMetricsFactory(registry).metrics(options));

    return vertxBuilder;
  }

Besides, it's code that's similar to what you will have when upgrading to Vert.x 5

@tsegismont
Copy link
Contributor

Can you try this and tell us if that suits your needs?

@bolbat
Copy link
Author

bolbat commented Nov 20, 2024

Can you try this and tell us if that suits your needs?

Will test this tomorrow in real application

Looks OK, no more deprecation warnings, and seems that it should work
Screenshot 2024-11-20 at 16 05 58

@bolbat
Copy link
Author

bolbat commented Nov 21, 2024

@tsegismont tested, proposed solution works
Closing PR as not actual

@bolbat bolbat closed this Nov 21, 2024
@tsegismont
Copy link
Contributor

Thank you for letting us know!

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.

3 participants