Skip to content

Auto-configure Jersey2 server instrumentation #12482

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

Conversation

mweirauch
Copy link
Contributor

This PR auto-configures the Micrometer Jersey2 server instrumentation. It's adopted from the micrometer-spring-legacy auto-configuration.

Relates to gh-12447 and micrometer-metrics/micrometer#486

@@ -76,7 +77,8 @@
RabbitMetricsAutoConfiguration.class, CacheMetricsAutoConfiguration.class,
DataSourcePoolMetricsAutoConfiguration.class,
RestTemplateMetricsAutoConfiguration.class,
WebFluxMetricsAutoConfiguration.class, WebMvcMetricsAutoConfiguration.class);
WebFluxMetricsAutoConfiguration.class, WebMvcMetricsAutoConfiguration.class,
JerseyServerMetricsAutoConfiguration.class);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just for completeness sake in case somebody is going to use the MetricsRun tooling in other tests. It's not used in the Jersey auto-configuration tests.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 14, 2018
@MarkusKull
Copy link

Did a quick test by copying this configuration into a jersey application with prometheus: Some uri-tags are now indeed templated. But raw uris are additionally recorded, probably because both MVC- and Jersey-RequestFilters apply to a http request. As quick workaround the jersey-meters were renamed and MVC-meters were suppressed by a MeterFilter.
A better fix could simply disable the MVC-Filter when encountering Jersey. Or MVC should omit recording when seeing a RequestAttribute set by Jersey-Filter.

@mweirauch
Copy link
Contributor Author

Yap. WebMVC instrumentation kicks in. One could disable WebMvcMetricsAutoConfiguration in that case. Alternatively configure Jersey to work in servlet-filter mode and set it's filter order to -2147483648. That at least work in Spring-Boot 1.5. (Haven't tested Jersey pass-through of requests it doesn't handle, though.)

@philwebb philwebb added for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 4, 2018
@philwebb philwebb added this to the Backlog milestone May 4, 2018
@philwebb
Copy link
Member

philwebb commented May 4, 2018

We might rework this depending on how the implementation of #13064 goes.

@snicoll
Copy link
Member

snicoll commented May 14, 2018

There is also another implementation for the same feature in #12905 that we should review as part of merging this.

@mweirauch
Copy link
Contributor Author

After some time has passed, what's your stance on this PR? #13064 hasn't seen any progress as of yet.

We are slowly moving to Spring Boot 2 services and I remembered this being still open. It's not hard to come up with a custom/proprietary setup for the time being, but it would be cool to not have to care about Jersey instrumentation in the future. (Even if it might get reworked with #13064.)

@philwebb
Copy link
Member

@mweirauch We're still hoping to get to this one if we can.

@mweirauch mweirauch force-pushed the autoconfigure-jersey2-server-instrumentation branch from 7875732 to c9558d4 Compare August 28, 2018 21:36
@mweirauch
Copy link
Contributor Author

Just in case, I went ahead and updated the PR against master/2.1.x.

Contains parts of #12905 and credited @michael-simons where appropriate. (Hope this is ok dear "Namensvetter" ;))

@mweirauch mweirauch force-pushed the autoconfigure-jersey2-server-instrumentation branch from c9558d4 to 1cdcf6c Compare September 2, 2018 11:24
@mweirauch mweirauch force-pushed the autoconfigure-jersey2-server-instrumentation branch from 1cdcf6c to 49afeda Compare September 6, 2018 20:33
@wilkinsona
Copy link
Member

I think we should merge this one rather than #12905, with a single squashed commit amended to list @michael-simons as a co-author.

wilkinsona pushed a commit to wilkinsona/spring-boot that referenced this pull request Oct 5, 2018
See spring-projectsgh-12482

Co-authored-by: Michael J. Simons <michael@simons.ac>
@wilkinsona
Copy link
Member

There are some differences in Micrometer's Jersey tag support and what it and Boot offer for Web MVC. I've opened micrometer-metrics/micrometer#900 to try and align things. I don't think it needs to block merging this.

wilkinsona pushed a commit to wilkinsona/spring-boot that referenced this pull request Oct 5, 2018
See spring-projectsgh-12482

Co-authored-by: Michael J. Simons <michael@simons.ac>
@wilkinsona
Copy link
Member

In the absence of #13064, I think what's proposed here plus a little polishing is sufficient. In the polishing, I've added a filter that applies management.metrics.web.server.max-uri-tags to Jersey metrics. The Spring MVC metrics filter is conditional on the presence of DispatcherServlet. I think applications that depend on both Jersey and MVC will be in the minority. Those that do can customise things to meet their needs, for example by disabling WebMvcMetricsAutoConfiguration as proposed above. We can iterate on this based on the feedback we get, and then hopefully improve things significantly in a 2.x release that includes #13064.

@wilkinsona wilkinsona closed this in 72e2313 Oct 5, 2018
wilkinsona added a commit that referenced this pull request Oct 5, 2018
* gh-12482:
  Polish "Auto-configure Micrometer's Jersey 2 server instrumentation"
  Auto-configure Micrometer's Jersey 2 server instrumentation
@wilkinsona wilkinsona removed this from the 2.1.x milestone Oct 5, 2018
@wilkinsona wilkinsona added this to the 2.1.0.RC1 milestone Oct 5, 2018
@wilkinsona
Copy link
Member

@mweirauch, thank you for making your first contribution to Spring Boot. @michael-simons, thanks for your contribution here too. You are a co-author of dd126fa. The proposed changes are now in master. I polished things a little in 72e2313. Other than some repackaging, the main changes were to a meter filter and to reuse the management.metrics.web.server properties rather than adding Jersey-specific properties.

@mweirauch mweirauch deleted the autoconfigure-jersey2-server-instrumentation branch October 8, 2018 10:07
@mweirauch
Copy link
Contributor Author

Thanks @wilkinsona for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: merge-with-amendments Needs some changes when we merge type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants