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

Management endpoints are created even though they are disabled #2921

Closed
aheusingfeld opened this issue May 6, 2015 · 13 comments
Closed

Management endpoints are created even though they are disabled #2921

aheusingfeld opened this issue May 6, 2015 · 13 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@aheusingfeld
Copy link
Contributor

Though Spring Boot provides the configuration property to disable certain management endpoints, all of them are instantiated no matter whether they are enabled or disabled via configuration. Especially looking at the TraceEndpoint which holds an instance of TraceRepository which again references objects representing the last 100 HTTP requests, we seem to be looking at memory hogs here.

A very naive idea to avoid this would be something like this (from org.springframework.boot.actuate.autoconfigure.EndpointAutoConfiguration)

    @Bean
    @ConditionalOnMissingBean
    @ConditionalOnExpression("${endpoints.trace.enabled}")
    public TraceEndpoint traceEndpoint(@Value("${trace.repository.capacity}") int capacity) {
        this.traceRepository.setCapacity(capacity);
        return new TraceEndpoint(this.traceRepository);
    }

but I'm not sure about the side effects this might introduce. And this doesn't prevent the TraceRepository and the WebRequestTraceFilter from being instantiated.

@snicoll
Copy link
Member

snicoll commented May 7, 2015

I guess that option meant "exposure" as REST or MBean endpoints but it'd probably more consistent to use that property to create the service at all. Other things should nicely fit in (with the exposure part being conditional of the presence of the service itself).

There is a endpoints.enabled property that allows to change the settings of all endpoints in one convenient property. If we want to pursue this, we'd probably have to build a custom condition that checks both of these properties.

@aheusingfeld
Copy link
Contributor Author

it'd probably more consistent to use that property to create the service at all.

I'd agree. The one thing that really needs to be reviewed is that the endpoints registered via EndpointWebMvcAutoConfiguration do honor the setting of the endpoint.NAME.enabled flag e.g. via @ConditionalOnProperty(prefix = "endpoints.health", name = "enabled", matchIfMissing = true) but the EndpointAutoConfiguration doesn't. Is that on purpose?

But again the TraceRepository itself doesn't honor any of the parameters. And unfortunately its capacity isn't controllable via application.properties either. I changed that by adding the following to a @Configuration class:

    /**
     * Make the capacity of the {@link TraceRepository} configurable. This determines how many requests are kept under /management/trace
     * @param capacity the capacity of the trace repository
     * @return the configured TraceRepository
     */
    @Bean
    public TraceRepository traceRepository(@Value("${management.trace.capacity:0}") int capacity) {
        InMemoryTraceRepository inMemoryTraceRepository = new InMemoryTraceRepository();
        inMemoryTraceRepository.setCapacity(capacity);
        return inMemoryTraceRepository;
    }

@snicoll
Copy link
Member

snicoll commented May 8, 2015

I find this weird as well. @dsyer, @philwebb do you know the reason?

As for the capacity thing, please open a separate issue, it's worth adding that as well.

@snicoll snicoll changed the title Enhancement: Management endpoints use memory though they are disabled Management endpoints are creted even though they are disabled May 8, 2015
@wilkinsona
Copy link
Member

The mismatch in the handling of enabled was introduced by me as part of the fix for #2767. The bug was MVC specific and I neglected to consider the JMX/general side of things.

@shakuzen
Copy link
Member

👍 For what it's worth, I also would like to see the endpoints not created at all if they are disabled as an enhancement.

@snicoll snicoll changed the title Management endpoints are creted even though they are disabled Management endpoints are created even though they are disabled May 28, 2015
@dsyer
Copy link
Member

dsyer commented May 28, 2015

I think it's useful (and generally harmless) to have the JMX end points even if the MVC ones are disabled.

@aheusingfeld
Copy link
Contributor Author

I think it's useful (and generally harmless) ...

My point is if JMX is disabled/ not used at all and the endpoint is disabled (requests are traced differently in this application), I don't need to have all these Traces shuffled around.

@dsyer
Copy link
Member

dsyer commented May 28, 2015

I guess if you feel strongly about a particular endpoint, e.g. you don't want traces, you just need to exclude from your @EnableAutoConfiguration (TraceWebFilterAutoConfiguration in this case).

@aheusingfeld
Copy link
Contributor Author

Yes, @EnableAutoConfiguration(exclude = {TraceWebFilterAutoConfiguration.class, TraceRepositoryAutoConfiguration.class}) to be precise. That's what I did as a workaround. I then created the beans in a @configuration class and added the capability to configure the capacity like this:

    /**
     * Make the capacity of the {@link TraceRepository} configurable. This determines how many requests are kept under /management/trace
     * @param capacity the capacity of the trace repository
     * @return the configured TraceRepository
     */
    @ConditionalOnProperty(value = "endpoints.trace.enabled", havingValue = "true", matchIfMissing = true)
    @Bean
    public TraceRepository traceRepository(@Value("${management.trace.capacity:10}") int capacity) {
        Assert.isTrue(capacity > 0, "management.trace.capacity must be > 0");
        Assert.isTrue(capacity < 500, "management.trace.capacity must be < 500");
        InMemoryTraceRepository inMemoryTraceRepository = new InMemoryTraceRepository();
        inMemoryTraceRepository.setCapacity(capacity);
        return inMemoryTraceRepository;
    }

    @ConditionalOnProperty(value = "endpoints.trace.enabled", havingValue = "true", matchIfMissing = true)
    @Bean
    public WebRequestTraceFilter webRequestLoggingFilter(BeanFactory factory, TraceRepository traceRepository, @Value("${management.dump_requests:false}") boolean dumpRequests) {
        WebRequestTraceFilter filter = new WebRequestTraceFilter(traceRepository);
        filter.setDumpRequests(dumpRequests);
        if (factory.containsBean("errorAttributes") && factory.isTypeMatch("errorAttributes", ErrorAttributes.class)) {
            filter.setErrorAttributes(factory.getBean(ErrorAttributes.class));
        }
        return filter;
    }

I have to admit though that this doesn't consider whether JMX is enabled or not at the moment.

@snicoll
Copy link
Member

snicoll commented May 28, 2015

Alex, please let's not mix the disable/enable debate with the ability to customize an endpoint. As I already asked previously, please open a separate thread for that one as it's clearly unrelated (unless I misunderstood something?).

Looking at your original description again, I feel that the issue you're having has nothing to do with that. If you want to control the TraceRepository you should customize that and not the endpoint. In particular, I find it very weird that you change the structure of the repository within the endpoint configuration.

Indeed we're not exposing everything as properties as we would like to avoid the PDD effect (Properties Driven Development).

What about the following for your use case?

@Configuration
public class SomeConfigOfYours {

    @Bean
    public TraceRepository traceRepository(@Value("${trace.repository.capacity}") int capacity) {
        InMemoryTraceRepository repo = new InMemoryTraceRepository();
                repo.setCapacity(capacity);
                return repo;
    }

}

Only one TraceRepository would be created (yours). What am I missing (again, from your original request)?

@aheusingfeld
Copy link
Contributor Author

I don't think you're missing something, it might just be that we talk past each other. The above snippet is from a CustomTraceConfiguration class which configures the TraceRepository and the WebRequestTraceFilter but I don't see where "change the structure of the repository within the endpoint configuration"?

The main point was that we had the requirement to introduce a way to disable the collection of the Traces which is now possible via the property endpoints.trace.enabled because of the

@ConditionalOnProperty(value = "endpoints.trace.enabled", havingValue = "true", matchIfMissing = true)

So, there 2 issues that I worked around in the above mentioned solution

  1. TraceWebFilterAutoConfiguration doesn't check endpoints.trace.enabled, but it should
  2. TraceRepositoryAutoConfiguration doesn't currently offer to disable the collection of Traces, but IMHO it should -> I'll create a feature request to separate this

Does that make sense?

@PostalBear
Copy link
Contributor

PostalBear commented Dec 10, 2016

Does it make sense to at least add @ConditionalOnProperty to WebRequestTraceFilter bean declaration as described by aheusingfeld so one could completely exclude this filter from filter chain ?

My use case is that I'm currently developing high load app and don't want to trace each and every call with built-in functionality (I use different mechanisms for that). I also do not want that my app creates objects which are not used. However to disable it completely I have to inherit from this filter and override shouldNotFilter method like this, which I don't like.

protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
  return true;
 }

I would be glad to have a way to do it via configuration, as it was already discussed in this thread.
I'm also willing to fix it and create pull request if it would save your time :)

@philwebb
Copy link
Member

@PostalBear You could just exclude the configuration:

@SpringBootApplication(exclude=TraceWebFilterAutoConfiguration.class)

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Jan 11, 2017
@philwebb philwebb added priority: normal type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review labels Jan 11, 2017
@wilkinsona wilkinsona added this to the 2.0.0.M4 milestone Jul 29, 2017
@wilkinsona wilkinsona self-assigned this Aug 11, 2017
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

7 participants