[Metrics-Jersey2] Changed 'getDefinitionMethod ' into 'getHandlingMet…

[antonio-petricca]

Hi, I use OpenAPI Generator Plugin to create my web resources interface/abstract-class, then I inherit those abstracts and override methods in order to provide their implemention.

On these overrides I put @Timed or any other metrics annotation.

In such situation getDefinitionMethod searches for annotations into the abstract class, which it cannot find because declared into the descendant implementation.

For that reason I changed 'getDefinitionMethod' into 'getHandlingMethod'.

[gitpod-io]

[joschi]

@antonio-petricca Thank you for your contribution!

Tests

[joschi]

@antonio-petricca Thanks for your contribution!

The issue I see with this is that it breaks the other way around: Having annotations on the interface or base/abstract classes.

If you could take another look at it and not break the existing functionality, I'm happy to merge it. 😄

See the following test cases:

diff --git metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/InheritedMetricsJerseyTest.java metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/InheritedMetricsJerseyTest.java
new file mode 100644
index 000000000..a71b56422
--- /dev/null
+++ metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/InheritedMetricsJerseyTest.java
@@ -0,0 +1,48 @@
+package com.codahale.metrics.jersey2;
+
+import com.codahale.metrics.MetricRegistry;
+import com.codahale.metrics.Timer;
+import com.codahale.metrics.jersey2.resources.InstrumentedAbstractResource;
+import com.codahale.metrics.jersey2.resources.InstrumentedExtendingResource;
+import com.codahale.metrics.jersey2.resources.InstrumentedInterfaceResource;
+import com.codahale.metrics.jersey2.resources.InstrumentedResource;
+import org.glassfish.jersey.server.ResourceConfig;
+import org.glassfish.jersey.test.JerseyTest;
+import org.junit.Test;
+
+import javax.ws.rs.core.Application;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+import static com.codahale.metrics.MetricRegistry.name;
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class InheritedMetricsJerseyTest extends JerseyTest {
+    static {
+        Logger.getLogger("org.glassfish.jersey").setLevel(Level.OFF);
+    }
+
+    private MetricRegistry registry;
+
+    @Override
+    protected Application configure() {
+        this.registry = new MetricRegistry();
+
+        ResourceConfig config = new ResourceConfig();
+        config = config.register(new MetricsFeature(this.registry));
+        config = config.register(InstrumentedExtendingResource.class);
+
+        return config;
+    }
+
+    @Test
+    public void timedMethodsFromInterfaceAreTimed() {
+        assertThat(target("concrete/interface").request().get(String.class)).isEqualTo("abstract");
+        assertThat(target("concrete/abstract").request().get(String.class)).isEqualTo("concrete");
+        assertThat(target("concrete/concrete").request().get(String.class)).isEqualTo("yay");
+
+        assertThat(registry.timer(name(InstrumentedExtendingResource.class, "fromConcreteClass")).getCount()).isEqualTo(1);
+        assertThat(registry.timer(name(InstrumentedExtendingResource.class, "fromAbstractClass")).getCount()).isEqualTo(1);
+        assertThat(registry.timer(name(InstrumentedAbstractResource.class, "interface")).getCount()).isEqualTo(1);
+    }
+}
diff --git metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedAbstractResource.java metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedAbstractResource.java
new file mode 100644
index 000000000..0b203215d
--- /dev/null
+++ metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedAbstractResource.java
@@ -0,0 +1,24 @@
+package com.codahale.metrics.jersey2.resources;
+
+import com.codahale.metrics.annotation.Timed;
+
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+
+@Path("/abstract")
+@Produces(MediaType.TEXT_PLAIN)
+public abstract class InstrumentedAbstractResource implements InstrumentedInterfaceResource {
+    @GET
+    @Timed
+    @Path("abstract")
+    public String fromAbstractClass() {
+        return "yay";
+    }
+
+    @Override
+    public String fromInterface() {
+        return "abstract";
+    }
+}
diff --git metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedExtendingResource.java metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedExtendingResource.java
new file mode 100644
index 000000000..2d47ea3f9
--- /dev/null
+++ metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedExtendingResource.java
@@ -0,0 +1,24 @@
+package com.codahale.metrics.jersey2.resources;
+
+import com.codahale.metrics.annotation.Timed;
+
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+
+@Path("/concrete")
+@Produces(MediaType.TEXT_PLAIN)
+public class InstrumentedExtendingResource extends InstrumentedAbstractResource {
+    @GET
+    @Timed
+    @Path("concrete")
+    public String fromConcreteClass() {
+        return "yay";
+    }
+
+    @Override
+    public String fromAbstractClass() {
+        return "concrete";
+    }
+}
diff --git metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedInterfaceResource.java metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedInterfaceResource.java
new file mode 100644
index 000000000..952356adc
--- /dev/null
+++ metrics-jersey2/src/test/java/com/codahale/metrics/jersey2/resources/InstrumentedInterfaceResource.java
@@ -0,0 +1,19 @@
+package com.codahale.metrics.jersey2.resources;
+
+import com.codahale.metrics.annotation.Timed;
+
+import javax.ws.rs.GET;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.core.MediaType;
+
+@Path("/interface")
+@Produces(MediaType.TEXT_PLAIN)
+public interface InstrumentedInterfaceResource {
+    @GET
+    @Timed
+    @Path("interface")
+    default String fromInterface() {
+        return "yay";
+    }
+}

[antonio-petricca]

You are right Guy!

I think we should find a way to allow the developers to choose between the two options: interfaces or classes.

Do you agree with me?

[antonio-petricca]

In my code I used my "hack" as follow:

@Slf4j
public class JerseySpiConfigurator implements AutoDiscoverable {

    @Override
    public void configure(FeatureContext context) {
        log.info("Registering metrics...");

        context.register(
            new InstrumentedResourceMethodApplicationListenerHack(
                MetricsEngine
                    .getInstance()
                    .getMetricRegistry()
            )
        );
    }
}

Would be acceptable for you to have a second version (mine) of InstrumentedResourceMethodApplicationListener class which a user could choose between during initializazion?

Or, do you think may be better to provide a kind of configuration settings which tell metrics to use one of the above ones?

[antonio-petricca]

I have found a possibile solution. I am implementing it...

[antonio-petricca]

Asap I will try it onto my real project and I will give you a feedback.

As you can see I changed the unit tests.

[antonio-petricca]

I inform you that it works on my real project (with OpenAPI).

Please tell me something about.

Regards,
Antonio

PS: if you want the same implementation should be ported the Jersey 3 and Jersey 3.1.

[antonio-petricca]

Hi @joschi , any news? :)

[github-actions]

This pull request is stale because it has been open 180 days with no activity. Remove the "stale" label or comment or this will be closed in 14 days.

[antonio-petricca]

Ho can I remove the stale label?

[github-actions]

This pull request is stale because it has been open 180 days with no activity. Remove the "stale" label or comment or this will be closed in 14 days.