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

Make endpoint web extensions conditional on endpoint being exposed over HTTP #28389

Closed
1 of 4 tasks
mbhave opened this issue Oct 20, 2021 · 8 comments
Closed
1 of 4 tasks
Assignees
Labels
status: first-timers-only An issue that can only be worked on by brand new contributors status: superseded An issue that has been superseded by another type: task A general task

Comments

@mbhave
Copy link
Contributor

mbhave commented Oct 20, 2021

Hi, this is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

If you have contributed before, consider leaving this one for someone new, and looking through our general ideal-for-contribution issues. Thanks!

Problem

Spring Boot provides a number of built-in endpoints, called actuators, to help you monitor and and manage your application. These endpoints are available over JMX and HTTP. However, depending on the technology, the behavior of some of these endpoints might need to change. For example, when the health endpoint is used over HTTP and the endpoint returns a status of down, this gets mapped to the 503 status code. For this purpose, these endpoints have technology specific extensions which are annotated with @EndpointWebExtension or @EndpointJmxExtension. The beans for these extensions should be auto-configured only if the endpoint is exposed over that technology. Otherwise, it's an unnecessary additional bean in the application context. Currently, when spring.jmx.enabled=true, even if the endpoint is not exposed over HTTP, the bean for the web extensions will be created.

Solution

#28131 added support for an additional attribute on @ConditionalOnAvailableEndpoint. We can update auto-configurations for web extensions to make use of it.

Tests for the corresponding auto-configurations will also need to be added to make sure that creation of the bean backs off correctly.

Steps to Fix

  • Claim this issue with a comment below and ask any clarifying questions you need
  • Set up a repository locally following the Contributing Guidelines
  • Try to fix the issue following the steps above
  • Commit your changes and start a pull request.
@mbhave mbhave added this to the 2.x milestone Oct 20, 2021
@mbhave mbhave added the type: task A general task label Oct 20, 2021
@philwebb philwebb modified the milestones: 2.x, 2.6.x Oct 20, 2021
@mbhave mbhave added the status: first-timers-only An issue that can only be worked on by brand new contributors label Oct 20, 2021
@davidh44
Copy link
Contributor

Hi, I'd like to claim this issue

@wilkinsona
Copy link
Member

Thanks very much, @davidh44. It's all yours. Please let us know if you have any questions.

@shikha-varun
Copy link

@davidh44 are you working on this issue ?

@davidh44
Copy link
Contributor

@shikha-varun yup spending time this weekend on it

@davidh44
Copy link
Contributor

@wilkinsona @mbhave I dug into the documentation + codebase and last commit (b7521e2), and have a few questions to clarify my understanding:

  • Am I correct in adding "exposure = EndpointExposure.WEB" to @ConditionalOnAvailableEndpoint in each of the built-in endpoints' auto configuration files?
  • Any reason for listing those 4 specific endpoint types out of the 25 built-in ones? I'm assuming all 25 need to be updated and have added tests?
  • Any hints on how to write the tests? I see an existing "runWhenNotExposedShouldNotHaveEndpointBean()". Should I be building off/expanding this to include the scenario when JMX is enabled?
  • Anything else I am missing/need to know?

Thanks

@wilkinsona
Copy link
Member

Am I correct in adding "exposure = EndpointExposure.WEB" to @ConditionalOnAvailableEndpoint in each of the built-in endpoints' auto configuration files?

Not quite. @ConditionalOnAvailableEndpoint(exposure = EndpointExposure.WEB) should be added to each @Bean method that defines an endpoint web extension.

Any reason for listing those 4 specific endpoint types out of the 25 built-in ones? I'm assuming all 25 need to be updated and have added tests?

The changes only apply to the @Bean method of beans annotated with @EndpointWebExtension. #28131 took care of this for the health endpoint's web extensions and we now need to do the same for the four other extensions listed in the description.

Any hints on how to write the tests? I see an existing "runWhenNotExposedShouldNotHaveEndpointBean()". Should I be building off/expanding this to include the scenario when JMX is enabled?

For each of the four auto-configurations, I'd add a new test that enables JMX (spring.jmx.enabled=true) and exposes the endpoint over JMX (management.endpoints.jmx.exposure.include=caches, for example). It should then assert that the context has the endpoint bean but does not have the extension. For the caches endpoint, that would look like this:

@Test
void runWhenOnlyExposedOverJmxShouldHaveEndpointBeanWithoutWebExtension() {
    this.contextRunner.withBean(CacheManager.class, () -> mock(CacheManager.class))
            .withInitializer(new ConditionEvaluationReportLoggingListener(LogLevel.DEBUG))
            .withPropertyValues("spring.jmx.enabled=true", "management.endpoints.jmx.exposure.include=caches")
            .run((context) -> assertThat(context).hasSingleBean(CachesEndpoint.class)
                    .doesNotHaveBean(CachesEndpointWebExtension.class));
}

Anything else I am missing/need to know?

No, I don't think so, but feel free to let us know if you have any further questions.

@davidh44
Copy link
Contributor

@wilkinsona Thanks, makes sense! I made some changes and started a pull request. Let me know how it looks, thanks.

@wilkinsona
Copy link
Member

Great stuff. Thanks, @davidh44. I'll close this issue in favor of your PR.

@wilkinsona wilkinsona removed this from the 2.6.x milestone Oct 28, 2021
@wilkinsona wilkinsona added the status: superseded An issue that has been superseded by another label Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: first-timers-only An issue that can only be worked on by brand new contributors status: superseded An issue that has been superseded by another type: task A general task
Projects
None yet
Development

No branches or pull requests

5 participants