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

Missing RabbitMQ metrics if bean is defined as a ConnectionFactory #25138

Closed
Huo12345 opened this issue Feb 9, 2021 · 8 comments
Closed

Missing RabbitMQ metrics if bean is defined as a ConnectionFactory #25138

Huo12345 opened this issue Feb 9, 2021 · 8 comments
Assignees
Labels
type: bug A general bug
Milestone

Comments

@Huo12345
Copy link

Huo12345 commented Feb 9, 2021

Expected behaviour

When deploying an application with a RabbitMQ connection and prometheus metrics enabled to cloud foundy, the prometheus endpoint should expose metrics for RabbitMQ as documented in Spring Boot Reference Documentation similar to when run locally.

Observed behaviour

When deployed to cloud foundry, no RabbitMQ metrics are exposed on the prometheus endpoint.

Steps to reproduce

  1. Download sample src attached demo-rabbitmq-metrics-issue.zip
  2. Start RabbitMQ service (locally installed service or from docker-compose file in the source)
  3. Run the application with the profile local and check prometheus endpoint for entries with rabbit -> Should have multiple
  4. Deploy the app to cloud foundry with a RabbitMQ service attached and check the prometheus endpoint there for entries with rabbit -> Should have none.
    4.1 If there is no cloud foundry instance available for testing, you can run the app with profile ab to see the metrics and if to not see them. More details in section Probable Cause.

Probable Cause

When investigating this issue I found that RabbitMetricsAutoConfiguration class has the condition @ConditionalOnBean({ AbstractConnectionFactory.class, MeterRegistry.class }) attached. From code snippets in the documentation I assume that cloud foundry exposes the connection as ConnectionFactory, an interface implemented by AbstractConnectionFactory. To test this theory I manually configured a CachingConnectionFactory and exposed it in a bean of type ConnectionFactory and AbstractConnectionFactory (see class RabbitConfig in attached code, profiles ab for AbstractConnectionFactory and if for ConnectionFactory). When exposing it as ConnectionFactory no metrics were available, but when using AbstractConnectionFactory the metrics showed up in the prometheus endpoint. Is it possible to change the conditional on RabbitMetricsAutoConfiguration to @ConditionalOnBean({ ConnectionFactory.class, MeterRegistry.class }) to fix this issue?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 9, 2021
@snicoll snicoll changed the title Bug: Missing RabbitMQ metrics if connection is configured by cloud foundry Missing RabbitMQ metrics if connection is configured by cloud foundry Feb 9, 2021
@scottfrederick
Copy link
Contributor

@Huo12345 I don't see anything in your application that configures the RabbitMQ connection on Cloud Foundry, so it appears that the ConnectionFactory is being created and contributed to the application context by Java buildpack auto-reconfiguration. This approach is not compatible with Spring Boot auto-configuration in all cases. This is the main reason why the library that enables buildpack auto-reconfiguration has been deprecated.

A better approach is to include the Java CFEnv library in your application as shown in the CFEnv documentation and disable buildpack auto-reconfiguration when pushing the app to CF. There is no additional code to write, just adding the CFEnv dependency is sufficient. This will result in Boot auto-configuration creating the ConnectionFactory in a way that is compatible with the metrics auto-configuration.

Can you try this approach and see if it addresses your issue?

@scottfrederick scottfrederick added the status: waiting-for-feedback We need additional information before we can continue label Feb 10, 2021
@Huo12345
Copy link
Author

@scottfrederick I can confirm that using the Java CFEnv library fixes the issue mentioned above. So my issue is fixed, thanks.
Despite that, I question the use of AbstractConnectionFactory as a condition for the RabbitMetricsAutoConfiguration since I've seen some instances in documentation (i.e. Spring AMQP Documentation) where the Java configuration exposed a bean of type ConnectionFactory, leading to the same behaviour as described above. Especially because I see no reason not to use ConnectionFactory as the condition.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 11, 2021
@wilkinsona
Copy link
Member

Arguably this is a bug in the CloudFoundry code that defines the bean as a ConnectionFactory as it's recommended that beans are defined with as much type information as possible. /cc @garyrussell as, IMO, the Spring AMQP documentation should be updated here too.

The metrics auto-configuration requires an AbstractConnectionFactory as it calls AbstractConnectionFactory.getRabbitConnectionFactory(). While we could define the rabbitConnectionFactoryMetricsPostProcessor when there's a ConnectionFactory bean, it would then sometimes do nothing in the (perhaps unlikely) event that the org.springframework.amqp.rabbit.connection.ConnectionFactory isn't an AbstractConnectionFactory.

@garyrussell
Copy link
Contributor

IMO, the Spring AMQP documentation should be updated here too.

@wilkinsona Which documentation? And, to what? Any boot examples generally rely on the auto-configured beans and don't define any.

Or, do you mean change all the examples that do define a connection factory @Bean to ACF to encourage using narrower types? We can do that.

The metrics BPP checks the bean type anyway, so making it conditional on the interface wouldn't break anything.

@wilkinsona
Copy link
Member

Sorry for not being clear, Gary. I was referring to the documentation that @Huo12345 linked to above. rabbitTemplate and myQueue both declare a return type that matches what they return but connectionFactory and amqpAdmin do not. I would change them to declare that they return CachingConnectionFactory and RabbitAdmin respectively.

The metrics BPP checks the bean type anyway, so making it conditional on the interface wouldn't break anything.

Yeah, indeed, it won't break anything. It may end up being defined unnecessarily but I think that's unlikely and it certainly wouldn't be the end of the world if it was.

@garyrussell
Copy link
Contributor

Sorry; I missed that comment.

wouldn't be the end of the world

Yes, and as you said, it is [extremely] unlikely it wouldn't be an ACF, except perhaps in tests where it might be a mock CF.

@scottfrederick
Copy link
Contributor

There is an issue to address the usage of Spring Cloud Connectors in the Java buildpack, and the Java CFEnv documentation covers this use case in the interim. I'll close this issue.

@scottfrederick scottfrederick added for: external-project For an external project and not something we can fix and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Feb 11, 2021
@wilkinsona wilkinsona changed the title Missing RabbitMQ metrics if connection is configured by cloud foundry Missing RabbitMQ metrics if bean is defined as a ConnectionFactory Feb 11, 2021
@wilkinsona wilkinsona removed the for: external-project For an external project and not something we can fix label Feb 11, 2021
@wilkinsona wilkinsona added this to the 2.3.x milestone Feb 11, 2021
@wilkinsona wilkinsona added the type: bug A general bug label Feb 11, 2021
@wilkinsona wilkinsona reopened this Feb 11, 2021
@wilkinsona
Copy link
Member

We've decided to tweak the condition so the post-processor will be in place if the bean is defined as a ConnectionFactory rather than as an AbstractConnectionFactory or one of its sub-classes.

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

No branches or pull requests

5 participants