Skip to content

Conversation

artembilan
Copy link
Member

Fixes #3376

The MeterRegistry may request meters on application shutdown.
The gauges for channels, handlers and message sources don't make sense
at the moment since all those beans are going to be destroyed.

  • Remove gauges for channel, handler and message source numbers from the
    IntegrationManagementConfigurer.destroy()

Cherry-pick to 5.3.x & 5.2.x

@wilkinsona
Copy link
Member

Is there an existing relationship between beans that will ensure that IntegrationManagementConfigurer is disposed before the Micrometer MeterRegistry? I couldn't spot anything but I'm not too familiar with the code so may well have missed something.

@artembilan
Copy link
Member Author

@wilkinsona ,

thank you for feedback!

So, let's see together what we have so far!

Since Micrometer is an optional dependency we can't make a hard class reference into that MeterRegistry everywhere we need metrics to expose. So, we have introduced a facade in front of it as a MetricsCaptor.
The further logic is like this then in the IntegrationManagementConfigurer:

public void afterSingletonsInstantiated() {
	Assert.state(this.applicationContext != null, "'applicationContext' must not be null");
	Assert.state(MANAGEMENT_CONFIGURER_NAME.equals(this.beanName), getClass().getSimpleName()
			+ " bean name must be " + MANAGEMENT_CONFIGURER_NAME);
	if (ClassUtils.isPresent("io.micrometer.core.instrument.MeterRegistry",
			this.applicationContext.getClassLoader())) {
		this.metricsCaptor = MicrometerMetricsCaptor.loadCaptor(this.applicationContext);
	}

And already that MicrometerMetricsCaptor.loadCaptor() does this:

MeterRegistry registry = applicationContext.getBean(MeterRegistry.class);

So, I think according your requirement there is no such a dependency injection relationship between IntegrationManagementConfigurer and MeterRegistry.

Maybe we should think about applying @Conditional on the inner Micrometer-specific @Configuration instead and then have an optional injection into the IntegrationManagementConfigurer bean definitiont. See an IntegrationManagementConfiguration.

Thanks again

@artembilan
Copy link
Member Author

@wilkinsona ,

any further feedback, please?

We probably need to see how this can make it into the upcoming release this Wednesday.

Thanks

@wilkinsona
Copy link
Member

wilkinsona commented Sep 15, 2020

I wonder if you could use a BeanFactoryPostProcessor or ImportBeanDefinitionRegistrar to register the MetricsCaptor implementation when Micrometer is on the classpath. It would be configured to depend on the MeterRegistry and could then be injected into IntegrationManagementConfigurer using an ObjectProvider so that it can be optional. This injection should then result in IntegrationManagementConfigurer being destroyed before the MeterRegistry.

Fixes spring-projects#3376

The `MeterRegistry` may request meters on application shutdown.
The gauges for channels, handlers and message sources don't make sense
at the moment since all those beans are going to be destroyed.

* Remove gauges for channel, handler and message source numbers from the
`IntegrationManagementConfigurer.destroy()`

**Cherry-pick to 5.3.x & 5.2.x**
a  `MicrometerMetricsCaptorConfiguration` when `MeterRegistry`
is on class path.
* Make `MicrometerMetricsCaptorConfiguration.integrationMicrometerMetricsCaptor()`
 bean dependant on the `ObjectProvider<MeterRegistry>`
* Make `IntegrationManagementConfiguration.managementConfigurer()`
dependant on the `ObjectProvider<MetricsCaptor>`.
This way the `IntegrationManagementConfigurer` is destroyed before
`MeterRegistry` when application context is closed
* Deprecate `MicrometerMetricsCaptor.loadCaptor()` in favor of
`@Import(MicrometerImportSelector.class)`
…PTOR_NAME`

bean when `MeterRegistry` is on class path and no `MICROMETER_CAPTOR_NAME` bean yet.
* Make `IntegrationManagementConfiguration.managementConfigurer()`
dependant on the `ObjectProvider<MetricsCaptor>`.
This way the `IntegrationManagementConfigurer` is destroyed before
`MeterRegistry` when application context is closed
* Deprecate `MicrometerMetricsCaptor.loadCaptor()` in favor of
`@Import(MicrometerMetricsCaptorRegistrar.class)`
* Fix test to make a `MeterRegistry` bean as `static` since
`@EnableIntegrationManagement` depends on this bean definition now
@artembilan
Copy link
Member Author

OK. I have added a MicrometerMetricsCaptorRegistrar implements ImportBeanDefinitionRegistrar to satisfy dependency tree.

Please, take a look how is this now, @wilkinsona .

Thank you!

@wilkinsona
Copy link
Member

With the caveat that I haven't tried to run the code, those changes look good to me now.

@garyrussell garyrussell merged commit e154067 into spring-projects:master Sep 16, 2020
artembilan added a commit that referenced this pull request Sep 16, 2020
* GH-3376: Remove gauges on application ctx close

Fixes #3376

The `MeterRegistry` may request meters on application shutdown.
The gauges for channels, handlers and message sources don't make sense
at the moment since all those beans are going to be destroyed.

* Remove gauges for channel, handler and message source numbers from the
`IntegrationManagementConfigurer.destroy()`

**Cherry-pick to 5.3.x & 5.2.x**

* * Add `MicrometerImportSelector` to conditionally load
a  `MicrometerMetricsCaptorConfiguration` when `MeterRegistry`
is on class path.
* Make `MicrometerMetricsCaptorConfiguration.integrationMicrometerMetricsCaptor()`
 bean dependant on the `ObjectProvider<MeterRegistry>`
* Make `IntegrationManagementConfiguration.managementConfigurer()`
dependant on the `ObjectProvider<MetricsCaptor>`.
This way the `IntegrationManagementConfigurer` is destroyed before
`MeterRegistry` when application context is closed
* Deprecate `MicrometerMetricsCaptor.loadCaptor()` in favor of
`@Import(MicrometerImportSelector.class)`

* * Add `MicrometerMetricsCaptorRegistrar` to register a `MICROMETER_CAPTOR_NAME`
bean when `MeterRegistry` is on class path and no `MICROMETER_CAPTOR_NAME` bean yet.
* Make `IntegrationManagementConfiguration.managementConfigurer()`
dependant on the `ObjectProvider<MetricsCaptor>`.
This way the `IntegrationManagementConfigurer` is destroyed before
`MeterRegistry` when application context is closed
* Deprecate `MicrometerMetricsCaptor.loadCaptor()` in favor of
`@Import(MicrometerMetricsCaptorRegistrar.class)`
* Fix test to make a `MeterRegistry` bean as `static` since
`@EnableIntegrationManagement` depends on this bean definition now

# Conflicts:
#	spring-integration-core/src/main/java/org/springframework/integration/config/EnableIntegrationManagement.java
#	spring-integration-core/src/main/java/org/springframework/integration/config/IntegrationManagementConfiguration.java
#	spring-integration-core/src/main/java/org/springframework/integration/config/IntegrationManagementConfigurer.java

* Fix some deprecation warnings
artembilan added a commit that referenced this pull request Sep 16, 2020
* GH-3376: Remove gauges on application ctx close

Fixes #3376

The `MeterRegistry` may request meters on application shutdown.
The gauges for channels, handlers and message sources don't make sense
at the moment since all those beans are going to be destroyed.

* Remove gauges for channel, handler and message source numbers from the
`IntegrationManagementConfigurer.destroy()`

**Cherry-pick to 5.3.x & 5.2.x**

* * Add `MicrometerImportSelector` to conditionally load
a  `MicrometerMetricsCaptorConfiguration` when `MeterRegistry`
is on class path.
* Make `MicrometerMetricsCaptorConfiguration.integrationMicrometerMetricsCaptor()`
 bean dependant on the `ObjectProvider<MeterRegistry>`
* Make `IntegrationManagementConfiguration.managementConfigurer()`
dependant on the `ObjectProvider<MetricsCaptor>`.
This way the `IntegrationManagementConfigurer` is destroyed before
`MeterRegistry` when application context is closed
* Deprecate `MicrometerMetricsCaptor.loadCaptor()` in favor of
`@Import(MicrometerImportSelector.class)`

* * Add `MicrometerMetricsCaptorRegistrar` to register a `MICROMETER_CAPTOR_NAME`
bean when `MeterRegistry` is on class path and no `MICROMETER_CAPTOR_NAME` bean yet.
* Make `IntegrationManagementConfiguration.managementConfigurer()`
dependant on the `ObjectProvider<MetricsCaptor>`.
This way the `IntegrationManagementConfigurer` is destroyed before
`MeterRegistry` when application context is closed
* Deprecate `MicrometerMetricsCaptor.loadCaptor()` in favor of
`@Import(MicrometerMetricsCaptorRegistrar.class)`
* Fix test to make a `MeterRegistry` bean as `static` since
`@EnableIntegrationManagement` depends on this bean definition now

# Conflicts:
#	spring-integration-core/src/main/java/org/springframework/integration/config/EnableIntegrationManagement.java
#	spring-integration-core/src/main/java/org/springframework/integration/config/IntegrationManagementConfiguration.java
#	spring-integration-core/src/main/java/org/springframework/integration/config/IntegrationManagementConfigurer.java

* Fix some deprecation warnings

# Conflicts:
#	spring-integration-core/src/main/java/org/springframework/integration/config/IntegrationManagementConfigurer.java
@artembilan
Copy link
Member Author

Back-ported to 5.3.x as d0cab67 after fixing some conflicts.

Back-ported to 5.2.x as a2a8764 with more conflict resolutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gauges for the number of MessageChannels, MessageHandlers, and MessageSources may cause a BeanCreationNotAllowedException when the context is closed
3 participants