search

vert-x3 / issues

Apache License 2.0
37 stars 7 forks source link

Metrics regression #575

Closed vietj closed 3 years ago

vietj commented 3 years ago

Currently vertx-dropwizard-metrics build fails because custom options cannot be loaded from the options as they are replaced sometimes with a new instance.

This build shows the issue : https://github.com/vert-x3/vertx-dropwizard-metrics/runs/1908022188?check_suite_focus=true , e.g

testEventBusMetricsWithHandler(io.vertx.ext.dropwizard.MetricsTest)  Time elapsed: 0.072 sec  <<< FAILURE!
1154
java.lang.AssertionError: null
1155
    at org.junit.Assert.fail(Assert.java:87)
1156
    at org.junit.Assert.assertTrue(Assert.java:42)
1157
    at org.junit.Assert.assertNotNull(Assert.java:713)
1158
    at org.junit.Assert.assertNotNull(Assert.java:723)
1159
    at io.vertx.test.core.AsyncTestBase.assertNotNull(AsyncTestBase.java:351)
1160
    at io.vertx.ext.dropwizard.MetricsTest.testEventBusMetricsWithHandler(MetricsTest.java:837)
1161

It can be reproduced by simply running this test locally too.

I think we are missing a corresponding test in vertx-core.

tsegismont commented 3 years ago

@vietj this is not a regression from my patch (actually the PR is not merged yet)

The problem comes from:

https://github.com/eclipse-vertx/vert.x/blob/d9f1b16272c1d79828ca7d970352f3c3eb461a11/src/main/java/io/vertx/core/spi/VertxMetricsFactory.java#L44

It doesn't work because the toJson method in DropwizardMetricsOptions does not take several fields into account:

https://github.com/vert-x3/vertx-dropwizard-metrics/blob/35c18fbbe6479519f8a2507587849ec79c524951/src/main/generated/io/vertx/ext/dropwizard/DropwizardMetricsOptionsConverter.java#L101-L116

I'm not sure why actually because when parsing JSON these fields are taken into account.

vietj commented 3 years ago

ok I'll ahve a look myself then

On Tue, Feb 16, 2021 at 10:24 PM Thomas Segismont notifications@github.com wrote:

@vietj https://github.com/vietj this is not a regression from my patch (actually the PR https://github.com/eclipse-vertx/vert.x/pull/3810 is not merged yet)

The problem comes from:

https://github.com/eclipse-vertx/vert.x/blob/d9f1b16272c1d79828ca7d970352f3c3eb461a11/src/main/java/io/vertx/core/spi/VertxMetricsFactory.java#L44

It doesn't work because the toJson method in DropwizardMetricsOptions does not take several fields into account:

https://github.com/vert-x3/vertx-dropwizard-metrics/blob/35c18fbbe6479519f8a2507587849ec79c524951/src/main/generated/io/vertx/ext/dropwizard/DropwizardMetricsOptionsConverter.java#L101-L116

I'm not sure why actually because when parsing JSON these fields are taken into account.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/issues/issues/575#issuecomment-780126731, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCXNCBJA7XR4472XQK3S7LPATANCNFSM4XWYMV7Q .

vietj commented 3 years ago

thanks @tsegismont you are totally right: https://github.com/vert-x3/vertx-dropwizard-metrics/issues/104