metrics icon indicating copy to clipboard operation
metrics copied to clipboard

FEAT: Jersey: check impl method for annotations before interface method

Open markjeffreymiller opened this issue 1 year ago • 7 comments

Metrics annotations (@Timed, @Metered, etc) don’t allow annotating a resource on the implementing method, only on the defining (interface) method (or more accurately, the method corresponding to the @Path definition, more or less).

This patch enhances dropwizard-metrics to extract metrics annotations more intelligently, preferring the annotation on the implementation (and falling back to the interface / definitionMethod if the implementation annotation is absent).

This is a change in existing behavior, but I believe there are very few cases in which the metrics annotations spans an interface and implementation class, and almost zero such cases where the developer would prefer that the interface annotations take precedence.

markjeffreymiller avatar Aug 23 '23 20:08 markjeffreymiller

Effects of this change

I created a simple JAXRS endpoint, where the JAXRS annotations are defined on the interface:

@Path("/v1/purchase")
public interface AsynchronousV1PurchaseResource {

    @GET
    @Consumes({ "application/json", "application/x-protobuf" })
    @Produces({ "application/json", "application/x-protobuf" })
    @Path("/{purchaseId}")
    @Timed
    @ExceptionMetered
    @ResponseMetered
    void getPurchaseInfo(
            @Suspended AsyncResponse response,
            @PathParam("purchaseId") @NotNull Long purchaseId
    );
}

and the implementation looks like this:

    @Override
    @Metered(name = "getPurchaseInfo-Metered", absolute = true)
    @Timed(name = "getPurchaseInfo-Timed", absolute = true)
    @ExceptionMetered(name = "getPurchaseInfo-ExceptionMetered", absolute = true)
    @ResponseMetered(name = "getPurchaseInfo-ResponseMetered", absolute = true)
    public void getPurchaseInfo(AsyncResponse response, Long purchaseId) {
        // … if `purchaseId` ends in 3, 4, or 5, return an http 3XX, 4XX, or 5XX; otherwise HTTP 200
    }

I then generated some traffic to this endpoint and processed the resulting metrics output like so:

cat rawMetrics | perl -pe 's/"(value|.*_rate|p\d+|count|max|mean|min|stddev)" : .*/"$1" : "SOME_VALUE"/g; > metrics.txt

metrics.old.txt is the result of running with the existing version of dropwizard-jersey2

metrics.new.txt is the result of running with the attached patch

This screenshot highlights some of the changes in the metrics output:

Screenshot 2023-08-23 at 1 48 11 PM

markjeffreymiller avatar Aug 23 '23 20:08 markjeffreymiller

This PR is similar in goal to https://github.com/dropwizard/metrics/pull/2793, but I think it's substantially simpler and probably easier to review (note that the same patch is repeated 3 times to cover metrics-jersey 2, 3, and 3.1).

markjeffreymiller avatar Aug 23 '23 21:08 markjeffreymiller

Finally, if possible I'd like to backport this patch to the 3.x release branches. Is there any sort of automated process for making that happen, or would I have to file the usual PR(s)? (I would wait to do so until after this PR is reviewed and merged of course)

markjeffreymiller avatar Aug 23 '23 21:08 markjeffreymiller

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.

github-actions[bot] avatar Feb 29 '24 00:02 github-actions[bot]

Not stale; issue still persists. I haven't had time to rebase my branch to fix the checks, but still plan to do so.

markjeffreymiller avatar Feb 29 '24 18:02 markjeffreymiller