Skip to content

Provide more finely grained metrics for Spring Data REST repositories #14872

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
high-stakes opened this issue Oct 17, 2018 · 11 comments
Closed
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@high-stakes
Copy link

high-stakes commented Oct 17, 2018

No description provided.

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

wilkinsona commented Oct 17, 2018

Thanks for the report. It's an interesting problem. I'm not sure that there's much we can do about it in Spring Boot alone as it would require detailed understanding of Spring Data REST's URI structure and knowing the likely cardinality of each parameter in the path. For example, you would want separate stats for /api/person and api/house, but you would not want separate stats for /api/house/1, /api/house/2, …, /api/house/12345.

We currently get the matching path pattern (/api/{repository}/ in this case) from a request attribute that's set by Spring MVC. Perhaps Spring Data REST could set an attribute of its own that's more detailed. Things like /api/house/, /api/person, api/house/{id}, and api/person/{id}.

What do you think @olivergierke and @mp911de?

@odrotbohm
Copy link
Member

The usage of $basePath/{repository} stems from the fact that this so far has been the most convenient way to register a controller that will eventually end up "multiple" mappings registered. Effectively it's just one and we determine the repository to be used at invocation time when resolving the handler mapping etc.

We also use the {repository} placeholder to resolve the repository in HandlerMethodArgumentResolver implementations based on the @RequestMapping annotation on the invoked method. I'd assume this get's more complicated if we switched to avoiding these annotations and rather add explicit mappings separately.

Another reason that moving off that model might not be as easy is that users can currently use the {repository} placeholder to override resources globally. I assume we'd break that functionality moving to explicit mappings.

I'll consult @rstoyanchev but I'm afraid this – if possible at all – will not be a quick fix. I wonder if in the meantime there's a hook in the metrics APIs that some component registered by the auto-configuration for Spring Data REST could use to create different buckets. I.e. something similar to what @high-stakes has in place but that can be more focussed using Spring Data REST internal metadata.

@wilkinsona
Copy link
Member

Thanks for taking a look, @olivergierke.

I wasn't proposing a changing to any of Spring Data REST's mappings. I was wondering if it would be possible if Spring Data REST could, during request handling, set a request attribute with a path pattern in it after it's resolved the repository. That pattern could contains a little more information than Spring MVC's as it could replace the {repository} placeholder with the concrete value for the repository that's been resolved. It could also, perhaps, add more components to the pattern for the various resources that each repository provides, just leaving in {id} and the like for high cardinality parts of the pattern.

@wilkinsona
Copy link
Member

To put that another way, I am proposing that this:

I wonder if in the meantime there's a hook in the metrics APIs that some component registered by the auto-configuration for Spring Data REST could use to create different buckets. I.e. something similar to what @high-stakes has in place but that can be more focussed using Spring Data REST internal metadata.

Could be done by Spring Data REST and the result placed in a request attribute.

@rstoyanchev
Copy link
Contributor

The usage of $basePath/{repository} stems from the fact that this so far has been the most convenient way to register a controller that will eventually end up "multiple" mappings registered.

Not sure if this helps or not but as of 5.1, it possible to assign a base path to a controller, through external configuration (via WebMvcConfigurer), see SPR-16336 and docs.

Also since 4.2 there has been a public API to register and unregister controller methods programmatically, see SPR-11541 and docs.

odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Oct 18, 2018
…tribute.

We now expose a partially resolved PathPattern to contain the explicit repository base path so that the subpaths can be grouped around the individual repositories exposed.

Related ticket: spring-projects/spring-boot#14872
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Oct 18, 2018
…tribute.

We now expose a partially resolved PathPattern to contain the explicit repository base path so that the subpaths can be grouped around the individual repositories exposed.

Related ticket: spring-projects/spring-boot#14872
odrotbohm added a commit to spring-projects/spring-data-rest that referenced this issue Oct 18, 2018
…tribute.

We now expose a partially resolved PathPattern to contain the explicit repository base path so that the subpaths can be grouped around the individual repositories exposed.

Related ticket: spring-projects/spring-boot#14872
@odrotbohm
Copy link
Member

I've filed, fixed and back-ported DATAREST-1294 into Lovelace and Kay maintenance branches. We now expose a EFFECTIVE_REPOSITORY_LOOKUP_PATH request attribute to contain a partially resolved PathPattern that has the repository base resolved, i.e. one that will effectively produce different patterns per exposed repository.

As per @rstoyanchev's suggestion I also filed DATAREST-1295 to investigate if we can drop our own base path handling in favor of Spring MVCs.

@wilkinsona wilkinsona changed the title Actuator metrics data quality issue with REST Data Provide more finely grained metrics for Spring Data REST repositories Oct 18, 2018
@wilkinsona wilkinsona added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 18, 2018
@wilkinsona wilkinsona added this to the 2.0.x milestone Oct 18, 2018
@wilkinsona
Copy link
Member

Great stuff. Thank you, @olivergierke. We'll need to wait a little while to implement this as there's no release of the Spring Data release train planned before Boot 2.1.0.RELEASE.

@wilkinsona wilkinsona added the status: on-hold We can't start working on this issue yet label Oct 18, 2018
@odrotbohm
Copy link
Member

If you'd be willing to upgrade to a bugfix release even after RC1 we can easily ship one in time.

@wilkinsona
Copy link
Member

Yeah, we'd be happy to do that. Bug fixes are fine post-RC. Would you be able to ship a Kay release train too? That would allow us to do this in 2.0.x at the same time. I think a change in 2.0.x is warranted as it's low-risk and it's a bit arguable whether the current behaviour could actually be considered as a bug due to the metrics being rather useless.

@odrotbohm
Copy link
Member

I'll schedule a Kay and Lovelace service release for Friday next week then. /cc @mp911de

@wilkinsona wilkinsona removed the status: on-hold We can't start working on this issue yet label Oct 18, 2018
@wilkinsona
Copy link
Member

I'll defer to @olivergierke on that one. Any change to support it would be in Spring Data REST so
DATAREST-1294 is probably the best place to continue the discussion.

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

5 participants