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
[Metrics-Jersey2] Changed 'getDefinitionMethod ' into 'getHandlingMet… #2793
base: release/4.2.x
Are you sure you want to change the base?
[Metrics-Jersey2] Changed 'getDefinitionMethod ' into 'getHandlingMet… #2793
Conversation
…hod' to get annotation on derived resources classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antonio-petricca Thank you for your contribution!
…hod' to get annotation on derived resources classes.
ce9bcac
to
7b4567f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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";
+ }
+}
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? |
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 Or, do you think may be better to provide a kind of configuration settings which tell metrics to use one of the above ones? |
…nto release/4.2.x
I have found a possibile solution. I am implementing it... |
…et working): reverted to official version.
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. |
I inform you that it works on my real project (with OpenAPI). Please tell me something about. Regards, PS: if you want the same implementation should be ported the Jersey 3 and Jersey 3.1. |
Hi @joschi , any news? :) |
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. |
Ho can I remove the |
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'.