Skip to content

Fix actuator endpoint tables in docs #14712

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.

Sign up for GitHub

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
wants to merge 1 commit into from
Closed

Fix actuator endpoint tables in docs #14712

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ...-boot-project/spring-boot-docs/src/main/asciidoc/production-ready-features.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,10 @@ endpoints:
|Yes
|No

|`caches`
|Yes
|No

|`conditions`
|Yes
|No
Expand Down Expand Up @@ -262,7 +266,7 @@ endpoints:

|`integrationgraph`
|Yes
|Yes
|No
Copy link
Member

@snicoll snicoll Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes you think this is not enabled by default? There is a graph only if you're actually using Spring Integration. If you have no graph, there's not much to show so the endpoint is conditional on that.

So the endpoint is enabled by default but only shows up if you're using SI. Is that confusing?

Copy link
Member

@wilkinsona wilkinsona Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table is for exposure rather than enablement. The endpoint is enabled by default, but I don't believe it is exposed by default. In other words, I agree with @dreis2211 that the table is currently incorrect.

Copy link
Contributor

@mbhave mbhave Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bit different from exposure or enablement. I think adding the integration graph as No in the exposure table, might lead to the misconception that it will be available if management.endpoints.web.exposure is set to *. But it is conditionally available on SI and setting that flag won't make it available.

Copy link
Contributor Author

@dreis2211 dreis2211 Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flyway endpoint is very similar here. It requires Flyway of course, but also states a No. I don't care too much in the end and do what the team prefers, but setting integrationgraph to No seems more consistent at the moment.

Setting it to No also backs the code handling default exposure. E.g. WebEndpointAutoConfiguration

	@Bean
	public ExposeExcludePropertyEndpointFilter<ExposableWebEndpoint> webExposeExcludePropertyEndpointFilter() {
		WebEndpointProperties.Exposure exposure = this.properties.getExposure();
		return new ExposeExcludePropertyEndpointFilter<>(ExposableWebEndpoint.class,
				exposure.getInclude(), exposure.getExclude(), "info", "health");
	}

Or ManagementWebSecurityConfigurerAdapter

	@Override
	protected void configure(HttpSecurity http) throws Exception {
		http.authorizeRequests()
				.requestMatchers(
						EndpointRequest.to(HealthEndpoint.class, InfoEndpoint.class))
				.permitAll().anyRequest().authenticated().and().formLogin().and()
				.httpBasic();
	}

Both only expose the health and the info endpoint.

Having that said, I think we could do the following:

  1. Accept the PR as it is to be more consistent
  2. Clarify both the enablement and exposure tables documentation with an additional hint, that some endpoints can be conditional on something. Flyway having Flyway available, integrationgraph having SI on the classpath, etc

Copy link
Contributor

@mbhave mbhave Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dreis2211 Ignore my comment. I misread the change. Thanks for the clarification.

Copy link
Member

@snicoll snicoll Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This table is for exposure rather than enablement.

I misread that, sorry.


|`jolokia`
|N/A
Expand Down