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

Ignore trailing slash by default when recording web metrics #18207

Closed
rswart opened this issue Sep 2, 2019 · 7 comments
Closed

Ignore trailing slash by default when recording web metrics #18207

rswart opened this issue Sep 2, 2019 · 7 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@rswart
Copy link

rswart commented Sep 2, 2019

The WebMvcMetricsFilter uses the HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE to determine which handler the metric should be labeled with.

Since useTrailingSlashMatch is true by default most mvc controllers configured with a request mapping like:

@GetMapping(path = "/foo", produces = {MediaType.APPLICATION_JSON_VALUE})

will match both /foo and /foo/ and as a result BEST_MATCHING_HANDLER_ATTRIBUTE for this request mapping can have 2 values.

This leads to split metrics for the same request mapping: one for /foo and one for /foo/.

For now I have worked around this with a MeterRegistryCustomizer that strips the trailing slash, but I wonder if this does not deserve a more structural solution, e.g inside the metrics filter itself.

@philwebb philwebb transferred this issue from spring-projects/spring-boot Sep 9, 2019
@philwebb
Copy link
Member

philwebb commented Sep 9, 2019

@rswart Thanks for the report. I've transferred this issue to the Spring Framework team to see if they can deal with this when setting the BEST_MATCHING_HANDLER_ATTRIBUTE value. I think that might be the best solution since it will fix any other applications that rely on the attribute.

@philwebb philwebb changed the title WebMvcMetricsFilter split metrics when useTrailingSlashMatch=true BEST_MATCHING_HANDLER_ATTRIBUTE with trailing slash match may have different values Sep 9, 2019
@rstoyanchev
Copy link
Contributor

will match both /foo and /foo/ and as a result BEST_MATCHING_HANDLER_ATTRIBUTE for this request mapping can have 2 values

I'm probably missing something. The attribute reflects the matched handler. Shouldn't that be the same in both cases?

@philwebb
Copy link
Member

philwebb commented Sep 9, 2019

@rswart Did you mean HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE? I think that's the attribute that we use to add the uri tag. See WebMvcTags.

@rswart
Copy link
Author

rswart commented Sep 9, 2019

Yes, apologies. I didn't look into it deeply enough.

So apparently 2 patterns are created for the same handler when useTrailingSlashMatch is true leading to 2 separate metrics in micrometer.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 11, 2019

The BEST_MATCHING_HANDLER_ATTRIBUTE correctly reflects the pattern used to match the handler. There is no promise that it will present a unique value that is insensitive to trailing slashes.

I am quite certain that what is a "fix" for some will cause regression for others. A valid counter-argument would be that it's better to present the complete information, and the trailing slash can then be easily filtered out or ignored. I think it would make more sense for a metrics filter to decide whether to strip trailing slashes or not, possibly making that configurable.

@rstoyanchev
Copy link
Contributor

I am closing this from a Spring Framework perspective, but feel free to re-open and transfer back to Spring Boot if that makes sense.

@philwebb philwebb reopened this Sep 11, 2019
@philwebb philwebb transferred this issue from spring-projects/spring-framework Sep 11, 2019
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 11, 2019
@philwebb philwebb changed the title BEST_MATCHING_HANDLER_ATTRIBUTE with trailing slash match may have different values Duplicate metrics can be recorded by WebMvcTags since it does not rationalize trailing slashes Sep 11, 2019
@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Sep 27, 2019
@philwebb philwebb added type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Oct 23, 2019
@philwebb philwebb added this to the 2.2.x milestone Oct 23, 2019
@bclozel
Copy link
Member

bclozel commented Oct 23, 2019

The team decided to strip trailing slashes in our metrics filter, but still offer a configuration property in case trailing slashes are meaningful for some applications.

@mbhave mbhave self-assigned this Jan 7, 2020
@mbhave mbhave closed this as completed in e60194c Jan 13, 2020
@mbhave mbhave modified the milestones: 2.2.x, 2.2.3 Jan 13, 2020
@mbhave mbhave changed the title Duplicate metrics can be recorded by WebMvcTags since it does not rationalize trailing slashes Ignore trailing slash by default when recording web metrics Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants