Expose Spring Integration graph using an actuator #12331

TYsewyn · 2018-03-03T20:13:59Z

Based on the idea of @michael-simons (read this post), this PR will add the functionality of exposing the Spring Integration graph via JMX, Jersey, Spring MVC & WebFlux.

NB: Currently says @since 2.1.0 in the Javadoc.

…e with the injected application context

michael-simons · 2018-03-04T17:00:41Z

...mework/boot/actuate/autoconfigure/integration/IntegrationGraphEndpointAutoConfiguration.java

+ */
+@Configuration
+@ConditionalOnClass(IntegrationGraphServer.class)
+public class IntegrationGraphEndpointAutoConfiguration {


You have to add this to spring.factories as well, see /spring-boot-project/spring-boot-actuator-autoconfigure/src/main/resources/spring.factories.

This mechanism facilitates the fact that the autoconfiguration classes can be looked up instead are discovered via a package scan.

Good catch! For some reason it didn't get committed & pushed, but I do remember making the change 🤔
Anyway, will do that right away! Thanks!

wilkinsona · 2018-03-05T10:09:34Z

@garyrussell @artembilan what do you think please?

garyrussell · 2018-03-05T14:36:59Z

Fine with me; SI currently only supports MVC and WebFlux. Thanks Tim!

My only question is whether we should auto configure a IntegrationGraphServer by default when actuators are enabled.

This seems to be dependent on the presence of a bean; there is little overhead if it's never referenced.

artembilan

Right, we need to auto-configure an IntegrationGraphServer bean. And I think there is just enough to add one conditional into the IntegrationAutoConfiguration.IntegrationConfiguration.

@wilkinsona , I'd be glad to see your comments regarding code style.

Thanks

artembilan · 2018-03-05T14:51:31Z

...project/spring-boot-actuator-autoconfigure/src/main/asciidoc/endpoints/integrationgraph.adoc

+=== Response Structure
+
+The response contains all Spring Integration components used within the application, as well as the links between them.
+More info about the structure can be found in the https://docs.spring.io/spring-integration/reference/htmlsingle/#integration-graph[subchapter] for the integration graph in the reference documentation of Spring Integration.


I wouldn't use htmlsingle here as a cross-link. Consider to switch to this one: https://docs.spring.io/spring-integration/reference/html/system-management-chapter.html#integration-graph.

Also I don't think that info is formal word. The information doesn't pull any questions from me 😄

philwebb · 2018-03-06T05:08:53Z

...mework/boot/actuate/autoconfigure/integration/IntegrationGraphEndpointAutoConfiguration.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright 2012-2018 the original author or authors.


artembilan · 2018-03-05T14:54:01Z

...mework/boot/actuate/autoconfigure/integration/IntegrationGraphEndpointAutoConfiguration.java

+import org.springframework.integration.support.management.graph.IntegrationGraphServer;
+
+/**
+ * {@link EnableAutoConfiguration Auto-configuration} for the {@link IntegrationGraphEndpoint}.


I don't think that it's good idea to use imports only for JavaDocs.
Not sure how it is for Spring Boot, but in SI we disallow such a code via particular Checkstyle rule.

We don't have a similar restriction, but now I'm wondering if we should. It's fine for now.

artembilan · 2018-03-05T14:54:57Z

...e/src/main/java/org/springframework/boot/actuate/autoconfigure/integration/package-info.java

@@ -0,0 +1,20 @@
+/*
+ * Copyright 2012-2018 the original author or authors.


Is it really OK to have Copyright in the package-info.java files?

Yes please, that's consistent with the others that we have.

artembilan · 2018-03-05T14:55:57Z

...ate/autoconfigure/endpoint/web/documentation/IntegrationGraphEndpointDocumentationTests.java

+/**
+ * Tests for generating documentation describing the {@link IntegrationGraphEndpoint}.
+ *
+ * @author Tim Ysewyn


I think @since 2.1.0 is applied to test classes as well.

Actually, we don't @since tests.

philwebb · 2018-03-06T05:08:53Z

...mework/boot/actuate/autoconfigure/integration/IntegrationGraphEndpointAutoConfiguration.java

@@ -0,0 +1,49 @@
+/*
+ * Copyright 2012-2018 the original author or authors.


TYsewyn · 2018-03-06T18:51:56Z

@garyrussell @artembilan I was thinking the same. Should I just put following under IntegrationAutoConfiguration.IntegrationConfiguration?

@ConditionalOnMissingBean
@Bean
public IntegrationGraphServer integrationGraphServer() {
	return new IntegrationGraphServer();
}

And is this PR the right one to do this? I would assume that you would like to backport this to 1.x if possible?

artembilan · 2018-03-06T18:57:14Z

IMO that's the right place to do that.
And I don't think that we are going to backport it. There is no bug in the subject and there is always a workaround to do the same task using existing tools and hooks.

…there isn’t one yet

TYsewyn · 2018-03-10T19:30:42Z

@artembilan @garyrussell last addition has been pushed.
If everything is fine for you I think this PR can be marked for 2.1.0?
Please let me know if you need me to change some things!

snicoll · 2018-03-11T09:54:41Z

@TYsewyn we'll review and assign a milestone to this one in due course. Thanks!

artembilan

LGTM

See spring-projectsgh-12331

Closes spring-projectsgh-12331

snicoll · 2018-04-17T13:44:15Z

I took an extensive look at this one and polished in my own fork. I didn't like the fact an IntegrationGraphServer is created as part of the main auto-configuration. That's a management concern so it shouldn't be there at all.

I've tried to make it conditional to the fact that the endpoint is not enabled but there is no easy way to do that as @ConditionalOnEnabledEndpoint only works on specific types. Perhaps we should revisit this a bit.

Also I didn't get why the endpoint is disabled by default. Is that really what we want?

artembilan · 2018-04-17T14:33:16Z

I had the same thought originally, Stephane. Right, the IntegrationGraphServer is a management concern, but from the other hand it exists in the core module and it is a simple as possible passive component. So, in the end I decided that it won't hurt to keep one more bean from the main auto-configuration.

If that is still a concern, let's consider to instantiate it from some actuator auto-configuration then!
The problem that it is an independent component and can be used even if a mentioned endpoint is disabled.

+1 do not make the endpoint disabled by default. Missed that part somehow during review...
Any explanations on the matter, @TYsewyn ?

Thanks

snicoll · 2018-04-17T15:15:01Z

Thanks for the feedback.

and it is a simple as possible passive component

I am not sure I agree with that. There is an event listener that will build the graph once the context has refreshed. I see no reason to have that running if you don't use the actuator. If you need this feature, you can define the bean (as you'd probably do now).

The problem that it is an independent component and can be used even if a mentioned endpoint is disabled.

That's fine but IMO there is a difference between providing this (that only the endpoint would be using) and providing this when the endpoint is required. My preference is for the latter but unfortunately, that's not easy to implement.

+1 do not make the endpoint disabled by default.

I've already changed that in my polish commit

artembilan · 2018-04-17T15:50:21Z

Stephane,

I think we're good to go with your polishing: you have addressed all the concerns! 👍

TYsewyn · 2018-04-18T20:33:11Z

I disabled the endpoint because there wasn't an IntegrationGraphServer bean present in the auto-configuration, so it didn't make any sense to bootstrap that endpoint by default.
We added one later and I forgot to switch it on by default when adding the bean.

TYsewyn · 2018-04-18T20:41:18Z

spring-boot-project/spring-boot-docs/src/main/asciidoc/production-ready-features.adoc

@@ -105,6 +105,10 @@ exchanges).
 |Displays arbitrary application info.
 |Yes

+|`integrationgraph`
+|Exposes the Spring Integration graph.
+|No


@snicoll Can you update this in your polishing commit to indicate that it will be enabled by default? Thanks!

Yup, thanks for the ping.

snicoll · 2018-04-19T07:28:42Z

you have addressed all the concerns

I did not I think. I wish that the IntegrationGraphServer wasn't created at all if the endpoint is disabled or when Spring Integration is not enabled.

@artembilan how can I know that SI is enabled on a project? Is there a typical bean I could look for?

artembilan · 2018-04-19T14:42:45Z

@snicoll ,

I think you can create IntegrationGraphServer conditionally when an endpoint is enabled.
On the other hand our main IntegrationAutoConfiguration relies on the @ConditionalOnClass(EnableIntegration.class). Maybe that would be enough here as well?
If the story is still about an excluded IntegrationAutoConfiguration, then we can consider a condition against the IntegrationContextUtils.INTEGRATION_HEADER_CHANNEL_REGISTRY_BEAN_NAME bean which is a type of DefaultHeaderChannelRegistry. That is one of those which is registered by that @EnableIntegration.

Hope that helps.

snicoll · 2018-04-19T17:40:23Z

Thanks for the feedback @artembilan

I think you can create IntegrationGraphServer conditionally when an endpoint is enabled.

Actually I can't at the moment, so we'll have to change the condition if you want to make that happen.

snicoll · 2018-04-23T09:28:46Z

I've created #12945 to improve the condition. If this is merged, I can prevent IntegrationGraphServer to be created when then endpoint is disabled.

snicoll · 2018-04-25T15:12:50Z

@TYsewyn that's now merged in master. Thanks a lot!

See gh-12331

…raph * pr/12331: Polish "Add actuator endpoint for exposing the Spring Integration graph" Add actuator endpoint for exposing the Spring Integration graph

TYsewyn · 2018-04-26T19:43:00Z

Thanks everyone for the valuable feedback!

TYsewyn added 9 commits February 25, 2018 15:52

Added actuator endpoint for exposing the Spring Integration graph

134a833

Updated the ASF license header

63d7e7d

Merge branch 'master' into feature/actuator-integration-graph

9c12443

Fix checkstyle

e4894a5

Fixed conditionals for the auto-configuration

9c73e37

Inject an instance of IntegrationGraphServer instead of creating on…

9048a21

…e with the injected application context

Added IntegrationGraphEndpointWebIntegrationTests

5bbaa86

Added documentation

b99e990

Merge branch 'master' into feature/actuator-integration-graph

f1d75a5

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 3, 2018

michael-simons reviewed Mar 4, 2018

View reviewed changes

Updated spring.factories

ccadda0

artembilan suggested changes Mar 5, 2018

View reviewed changes

philwebb reviewed Mar 6, 2018

View reviewed changes

Updated documentation based on input from the PR

9cbd4ec

Added auto-configuration of an IntegrationGraphServer bean in case …

292bb20

…there isn’t one yet

artembilan approved these changes Mar 13, 2018

View reviewed changes

snicoll pushed a commit to snicoll/spring-boot that referenced this pull request Apr 17, 2018

Add actuator endpoint for exposing the Spring Integration graph

1ddc511

See spring-projectsgh-12331

snicoll added a commit to snicoll/spring-boot that referenced this pull request Apr 17, 2018

Polish "Add actuator endpoint for exposing the Spring Integration graph"

c2e903f

Closes spring-projectsgh-12331

snicoll self-assigned this Apr 17, 2018

snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 17, 2018

snicoll added this to the Backlog milestone Apr 17, 2018

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Apr 17, 2018

TYsewyn commented Apr 18, 2018

View reviewed changes

snicoll removed the status: waiting-for-feedback We need additional information before we can continue label Apr 23, 2018

snicoll modified the milestones: Backlog, 2.1.0.M1 Apr 25, 2018

snicoll pushed a commit that referenced this pull request Apr 25, 2018

Add actuator endpoint for exposing the Spring Integration graph

8c67ef1

See gh-12331

snicoll closed this in 03cf4fb Apr 25, 2018

		@@ -0,0 +1,49 @@
		/*
		* Copyright 2012-2018 the original author or authors.

		@@ -0,0 +1,20 @@
		/*
		* Copyright 2012-2018 the original author or authors.

Expose Spring Integration graph using an actuator #12331

Expose Spring Integration graph using an actuator #12331

Uh oh!

Conversation

TYsewyn commented Mar 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilkinsona commented Mar 5, 2018

Uh oh!

garyrussell commented Mar 5, 2018

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TYsewyn commented Mar 6, 2018

Uh oh!

artembilan commented Mar 6, 2018

Uh oh!

TYsewyn commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snicoll commented Mar 11, 2018

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

snicoll commented Apr 17, 2018

Uh oh!

artembilan commented Apr 17, 2018

Uh oh!

snicoll commented Apr 17, 2018

Uh oh!

artembilan commented Apr 17, 2018

Uh oh!

TYsewyn commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll commented Apr 19, 2018

Uh oh!

artembilan commented Apr 19, 2018

Uh oh!

snicoll commented Apr 19, 2018

Uh oh!

snicoll commented Apr 23, 2018

Uh oh!

snicoll commented Apr 25, 2018

Uh oh!

TYsewyn commented Apr 26, 2018

Uh oh!

Uh oh!

TYsewyn commented Mar 10, 2018 •

edited

Loading