Spring-boot: Provide more finely grained metrics for Spring Data REST repositories

[high-stakes]

Issue:
When using spring boot actuator with data-rest and exposing webmvc metrics, the request statistics times are summed up for all REST Data repositories because their endpoint is represented as /api/{repository}/.

Expected:

Each spring data rest repository endpoint is represented by separate metrics as the summed up data for all repositories is meaningless when identifying performance issues. For example /api/person stats need to be separated from /api/house repository metrics.

Environment:
Spring boot 5.0.7

[wilkinsona]

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?

[high-stakes]

My ugly workaround was this for the time being, maybe a custom tagsprovider in data rest would do the trick:

    /**
     * We are modifying the tags exposed by the webmvc metrics
     */
    @Bean
    public WebMvcTagsProvider webMvcTagsProvider() {
        return new DefaultWebMvcTagsProvider() {
            @Override
            public Iterable<Tag> getTags(HttpServletRequest request, HttpServletResponse response, Object handler, Throwable exception) {
                return StreamSupport.stream(super.getTags(request, response, handler, exception).spliterator(), false)
                        .map(it -> it.getKey().equals("uri") ? mapUriTag(it, request) : it).collect(Collectors.toList());
            }
        };
    }

    /**
     * Modify a URI tag provided by spring metrics
     * By default spring resolves REST data repositories as {repository} in the uri
     * We substitute this and other templated URI parts with the real URI so metrics are exposed on a repository basis instead.
     * We blacklist some templated parts from substitution like "id" as they belong to the same endpoint
     */
    private static Tag mapUriTag(Tag tag, HttpServletRequest request) {
        List<String> replaceBlackList = Arrays.asList("{id}");
        StringTokenizer templatePathTokenizer = new StringTokenizer(tag.getValue(),"/");
        StringTokenizer realPathTokenizer = new StringTokenizer(request.getServletPath(),"/");
        StringBuilder finalPath = new StringBuilder();

        while(templatePathTokenizer.hasMoreTokens()) {
            String templatePathToken = templatePathTokenizer.nextToken();
            String realPathToken = realPathTokenizer.hasMoreTokens() ? realPathTokenizer.nextToken() : null;
            finalPath.append("/");
            if (templatePathToken.startsWith("{") && templatePathToken.endsWith("}")
                    && !replaceBlackList.contains(templatePathToken) && realPathToken != null) {
                finalPath.append(realPathToken);
            }
            else {
                finalPath.append(templatePathToken);
            }
        }
        return new ImmutableTag("uri",finalPath.toString());
    } 

[odrotbohm]

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]

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]

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]

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]

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]

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.

[odrotbohm]

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

[wilkinsona]

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]

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

[high-stakes]

Great job!
I am wondering if replacing the {search} and similar patterns (except {projection},{id}) would also make sense or not.
Con: you can have a lot of metrics for one repository endpoint
Pro: the metrics have the same granularity as different kinds of queries. My findByFirstName query can be fast but findByLastName extremely slow as the field is not indexed, etc. Since these are more of RPC style calls i think it makes sense to distinguish

What do you think?

[wilkinsona]

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.