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

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

Open
wants to merge 3 commits into
base: release/4.2.x
Choose a base branch
from

Conversation

markjeffreymiller
Copy link

@markjeffreymiller markjeffreymiller commented Aug 23, 2023

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 markjeffreymiller requested review from a team as code owners August 23, 2023 20:36
@markjeffreymiller
Copy link
Author

markjeffreymiller commented Aug 23, 2023

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

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).
@markjeffreymiller markjeffreymiller force-pushed the check-the-implementation-method-for-annotations-before-checking-interface branch from d160b34 to b4c6e66 Compare August 23, 2023 20:54
@markjeffreymiller
Copy link
Author

This PR is similar in goal to #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
Copy link
Author

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)

Copy link

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 github-actions bot added the Stale label Feb 29, 2024
@markjeffreymiller
Copy link
Author

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

@github-actions github-actions bot removed the Stale label Mar 1, 2024
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.

None yet

1 participant