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

Enhanced configurability for micrometer utilization in AbstractMessageListenerContainer #1459

Closed
jo-george opened this issue May 17, 2022 · 6 comments · Fixed by #1460
Closed

Comments

@jo-george
Copy link

Expected Behavior

It should be possible to configure the default behavior and disable utilization of micrometer in context of the AMQP integration, particularly the AbstractMessageListenerContainer, before it even attempts to establish the micrometer support via creating a MicrometerHolder. A convenient way would be the possibility to configure the attribute AbstractMessageListenerContainer.micrometerEnabled and set it to false during the initialization, if needed.

If the case, that multiple instances of MeterRegistrys are available in the application context, is actually relevant and not just a theoretical case, it should be possible to specify the particular MeterRegistry (instantiable) subclass via configuration.

Current Behavior

Currently, the utilization of micrometer by the AbstractMessageListenerContainer is enabled by default when micrometer (or rather io.micrometer.core.instrument.MeterRegistry) is found on the classpath. So, when micrometer is used by any part of the application, it is also enabled by default for the *MessageListenerContainers, even if not wanted or not needed.

Additionally, during instantiation of the MicrometerHolder there is an assumption that only a single instance of MeterRegistry can be present in the application context. Technically, it might be possible to have more than one MeterRegistry and in that case it is not possible to specify, which particular MeterRegistry should be used and the message "No micrometer registry present" would also be misleading in that case.

Context

In our application, we're collecting some business related metrics using an InfluxMeterRegistry which should not automatically be used in context of our RabbitMQ integration via spring-amqp. The most convenient way, at least for us, to avoid this utilization would be to make AbstractMessageListenerContainer.micrometerEnabled configurable, so we could just configure it to be false.

Setting the AbstractMessageListenerContainer.micrometerEnabled attribute to false at the right moment during bean initialization via customized post-processing seems not to be possible, at least in our case, where the SimpleMessageListenerContainer is created and also initialized (including afterPropertiesSet()) as part of the call invokeInitMethods(beanName, wrappedBean, mbd); inside AbstractAutowireCapableBeanFactory.initializeBean(...) while the post-processings are applied right before and right after this whole step. Actually, a post-processing between creation and initialization, probably as a part of the ListenerContainerFactoryBean would be needed to programmatically set the micrometerEnabled flag which seems to be currently not in place for the instantiation of SimpleMessageListenerContainers.

@garyrussell
Copy link
Contributor

Regarding the second issue (multiple registries), we recently fixed a similar problem in spring-kafka

spring-projects/spring-kafka@dc47e72

@garyrussell
Copy link
Contributor

The AMLC already has a setter for micrometerEnabled.

/**
* Set to false to disable micrometer listener timers.
* @param micrometerEnabled false to disable.
* @since 2.2
*/
public void setMicrometerEnabled(boolean micrometerEnabled) {
this.micrometerEnabled = micrometerEnabled;
}

If you are using @RabbitListener, you can add a container customizer to the container factory:

factory.setContainerCustomizer(container -> container.setMicrometerEnabled(false));

@jo-george
Copy link
Author

Thank you very much for the quick response and the solution for the situation with multiple registries.

Regarding your advice with using @RabbitListener, I have one question: are the container customizers applied before or after afterPropertiesSet() of the AMLC is being called? To prevent the instantiation of the MicrometerHolder it would have to happen before. I've just seen it's the last step in AbstractRabbitListenerContainerFactory.createListenerContainer(...)

I guess we need to refactor our integration because currently we're using the ListenerContainerFactoryBean to create the message listener containers. Or would it be an option to also add container customizers to the LCFB and apply them between creation and initialization of the containers?

@jo-george
Copy link
Author

...or adding the property micrometerEnabled also to the ListenerContainerFactoryBean and set it during the initial configuration of the container in createInstance(), just like the other properties of the container, like acknowledgeMode, deBatchingEnabled, autoStartup etc. might be even simpler.

@garyrussell
Copy link
Contributor

before or after

The RabbitListenerEndpointRegistry calls afterPropertiesSet() after the factory has created the container (the factory calls the customizer before returning).

MessageListenerContainer listenerContainer = factory.createListenerContainer(endpoint);
listenerContainer.afterPropertiesSet();

Yes, the LCFB should expose the property and/or setter for a customizer; I will add that.

@garyrussell
Copy link
Contributor

We should also move the micrometer initialization to start().

artembilan pushed a commit that referenced this issue May 18, 2022
Resolves #1459

* Add micrometer properties and container customizers to LCFB.

* Use `ObjectProvider` to locate registry.
artembilan pushed a commit that referenced this issue May 18, 2022
Resolves #1459

* Add micrometer properties and container customizers to LCFB.

* Use `ObjectProvider` to locate registry.
garyrussell added a commit that referenced this issue May 18, 2022
garyrussell added a commit that referenced this issue May 18, 2022
garyrussell added a commit that referenced this issue May 18, 2022
garyrussell added a commit that referenced this issue May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants