Skip to content

Templated requests with Jersey may cause an explosion of URI tag values #12447

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

Closed
jkschneider opened this issue Mar 12, 2018 · 15 comments
Closed
Assignees
Labels
type: bug A general bug
Milestone

Comments

@jkschneider
Copy link
Contributor

See micrometer-metrics/micrometer#486 for a reproducible example.

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

This is an age-old problem that I (overly optimistically) hoped would be taken care of by the move to Micrometer.

The meter's being registered by WebMvcMetricsFilter. Given the class's name and its focus on Spring MVC, that doesn't really make sense for Jersey. I think we should update the filter so that, if possible, it just ignores requests that aren't handled by Spring MVC. Perhaps we can no-op the filter if getHandler(HttpServletRequest) returns null. If we want to provide metrics for Jersey, that should be done in a separate filter.

@mikksoone
Copy link

Thanks, the Bean provided in #5875 (comment) solves the issue, but yeah does not look so nice at all.

@jkschneider
Copy link
Contributor Author

jkschneider commented Mar 12, 2018

@wilkinsona Note that we have complete Jersey 2 support in Micrometer with the micrometer-jersey2 module, including a parameterized URI tag. Ultimately, we didn't implement this with a servlet filter, but with a RequestEventListener.

We just need to clean up shunting the WebMvcMetricsFilter if Jersey and autoconfiguring the Jersey support when relevant.

@jkschneider
Copy link
Contributor Author

@mikksoone If your app is wholly Jersey 2 and not a mixture of Jersey and WebMVC, I think a sufficient workaround for now is to disable the webmvc filter with management.metrics.web.server.auto-time-requests=false, and then configure the Jersey integration with something like this.

@mikksoone
Copy link

@jkschneider disabling management.metrics.web.server.auto-time-requests=false is indeed disabling it, but configuring the Jersey according to the micrometer-spring-legacy did not work out of the box (no http.server.requests anymore). I'm using Spring Boot 2.0, mentioning it just in case.

@mweirauch
Copy link
Contributor

The referenced PR auto-configures the Jersey2 instrumentation.

What @mikksoone was experiencing, that it was not setup for Spring-Boot 2. In addition to that, by attracting spring-boot-starter-hateos, spring-webmvc is silently pulled in which in turn triggers the WebMvc MetricsFilter auto-configuration, so that the untemplated http.server.requests metrics were present.

For the instrumentation to kick in, it needs to be present on the classpath. (micrometer-jersey2). Perhaps it's possible for start.spring.io to automagically pull in that dependency when Jersey and Actuator are selected?

Likely another issue:
What I observed when plugging in spring-boot-starter-hateos, the actuator endpoints are not reachable anymore. (2.0.1.BUILD-SNAPSHOT) I see them being registered by with WebMvcEndpointHandlerMapping. When in a non-mixed environment with Jersey only I can't see any output of how they are registered. But they are reachable.

I am not familiar with the boot internals regarding such a mixed-web-env setup and also not about spring-hateos and its Jersey support.

@wilkinsona
Copy link
Member

What I observed when plugging in spring-boot-starter-hateos, the actuator endpoints are not reachable anymore. (2.0.1.BUILD-SNAPSHOT) I see them being registered by with WebMvcEndpointHandlerMapping.

Can you please open a separate issue for this and, ideally, provide a small sample that reproduces the behaviour you have described?

When in a non-mixed environment with Jersey only I can't see any output of how they are registered. But they are reachable.

Unlike Spring MVC and WebFlux, Jersey doesn't log any of its mappings. In 2.0.1 snapshots we've started logging a summary of the web-exposed endpoints irrespective of the web framework that's being used (see #12442).

@mweirauch
Copy link
Contributor

@wilkinsona I digged a little further regarding the mixed setup: When webmvc and Jersey both serve root ("/") - as happened here - the actuator endpoints are 404. Does this still qualify as a bug? (Just don't wan't to open new issues for known stuff.) As soon as I move Jerseys application-path, things start working again. Same behaviour with Spring-Boot 1.5. So no behavioral change for this unlucky constellation when spring-webmvc sneaks in.

@wilkinsona
Copy link
Member

Does this still qualify as a bug?

No, I don't think it does. When you use Jersey and MVC side-by-side our expectation is that they'll be configured to co-exist, either by moving them to separate paths or by using a filter for Jersey and configuring it to allow the request to continue when it 404s.

@mweirauch
Copy link
Contributor

Thanks for the answer. Is there a slight chance the associated PR can make it into 2.0.1? Would help those updating from SB 1.5 with micrometer-spring-legacy and micrometer-jersey2 setup as well.

@bclozel bclozel added this to the Backlog milestone Apr 5, 2018
@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 5, 2018
@wilkinsona wilkinsona added the for: team-attention An issue we'd like other members of the team to review label Apr 25, 2018
@wilkinsona wilkinsona added type: bug A general bug and removed for: team-attention An issue we'd like other members of the team to review type: enhancement A general enhancement labels May 4, 2018
@wilkinsona wilkinsona modified the milestones: Backlog, 2.0.x May 4, 2018
@wilkinsona
Copy link
Member

wilkinsona commented May 4, 2018

We spent some time discussing this today and came to a few conclusions:

The current metrics filter contains a mix of general and MVC-specific logic. We'd like it to focus only on the general logic and then delegate to implementations of a strategy interface for getting information that's specific to a particular web stack (#13064). This change will have to wait for 2.1 and means that we won't accept #12482 in its current form and won't merge it into 2.0.x.

We still need to do something in 2.0.x and we'll use this issue to do that. Our current plan is to remove the fallback to using the path info for the URI.

@snicoll snicoll changed the title URI tag of http.server.requests meter does NOT contain templated URI for jersey URI tag of http.server.requests meter does not contain templated URI for jersey Jun 4, 2018
@mweirauch
Copy link
Contributor

I fail to see where the OPs issue is being fixed. No offense, but this issue has been downgraded from "missing auto-configured jersey2 instrumentation" to "tone down WebMVC MetricsFilter to fix metrics explosion by having it chunt unknown requests into common tag values". The current fix is a good one, If I may say so. Don't get me wrong.

Meanwhile, there's two PRs addressing this issue. Knowingly one needs to take care of the WebMVC MetricsFilter to not interfere. (Either by turning it off or hooking the Jersey-Filter in front of it. At least the latter is what I do testwise in 1.5.)

Again, no offense. I understand you have to deal with a plethora of issues and need to find a good path between getting stuff fixed and postponed or rejected.

I assume #13064 is where the real work shall be done? Planned for which milestone?

@wilkinsona
Copy link
Member

I assume #13064 is where the real work shall be done?

Correct, along with something similar to #12482 but reworked for proposed new approach.

Planned for which milestone?

It's assigned to the backlog which, as described here, is for issues that we hope to address in 2.1.

@wilkinsona
Copy link
Member

I fail to see where the OPs issue is being fixed

I should also point out that the OP is the lead of the Micrometer project and has given a 👍 reaction to our proposed course of action.

@wilkinsona wilkinsona changed the title URI tag of http.server.requests meter does not contain templated URI for jersey Templated requests with Jersey may cause an explosion of URI tag values Jun 4, 2018
@mweirauch
Copy link
Contributor

mweirauch commented Jun 4, 2018

I should also point out that the OP is the lead of the Micrometer project and has given a +1 reaction to our proposed course of action.

I was referring to the "OP content" of referenced issue and the comments here and over which clarified the situation of what is missing (the jersey2 instrumentation setup) and what is causing what was seen (webmvc metrics filter producing metrics for jersey paths).
And yes, it's not unlikey I had contact with Jon when collaborating on the micrometer project :)

All good! Thanks for your replies!

isopov added a commit to isopov/spring-boot that referenced this issue Jun 8, 2018
mweirauch added a commit to mweirauch/spring-boot that referenced this issue Sep 2, 2018
mweirauch added a commit to mweirauch/spring-boot that referenced this issue Sep 6, 2018
bclozel pushed a commit that referenced this issue Jan 8, 2019
This commit aligns the Spring WebFlux instrumentation on Spring MVC
since gh-12447.
From now on, if the best matching path pattern is not found,
the recorded uri tag will be "UNKNOWN".

Note that for WebFlux.fn, the pattern information is properly
recorded as of SPR-17395.

Closes gh-15609
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

6 participants