-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
base: release/4.2.x
Are you sure you want to change the base?
Conversation
Effects of this changeI created a simple JAXRS endpoint, where the JAXRS annotations are defined on the interface:
and the implementation looks like this:
I then generated some traffic to this endpoint and processed the resulting metrics output like so:
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: |
d160b34
to
b4c6e66
Compare
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). |
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) |
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. |
Not stale; issue still persists. I haven't had time to rebase my branch to fix the checks, but still plan to do so. |
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. |
Not stale; issue still persists. I haven't had time to rebase my branch to fix the checks, but still plan to do so. |
d4c7681
to
4c07176
Compare
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).
4c07176
to
41e3dc6
Compare
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.