Update the PrometheusRSocketClientProperties prefix #77
Conversation
This commit updates the prefix of the PrometheusRSocketClientProperties config props to be consistent w/ the metrics config props naming scheme in Spring Boot 3.x. Specifically, change from `management.metrics.export.prometheus.rsocket` to `management.prometheus.metrics.export.rsocket`.
| import java.time.Duration; | ||
|
|
||
| @ConfigurationProperties("management.metrics.export.prometheus.rsocket") | ||
| @ConfigurationProperties("management.prometheus.metrics.export.rsocket") |
There was a problem hiding this comment.
Another option for this prefix is micrometer.prometheus.rsocket (the controller config props is currently prefixed w/ micrometer.prometheus-controller).
Either way, the current prefix should be updated away from the Spring Boot 2.x naming scheme.
There was a problem hiding this comment.
I don't feel strongly one way or the other. As you say, we do have a micrometer-specific config prop prefix elsewhere, so part of me wants to align with that. On the other hand, I can understand how using the same naming scheme that Spring Boot uses would be familiar and perhaps expected from Spring Boot users and this config props is in a Spring Boot starter.
If we go with management. I would suggest management.prometheus-rsocket.metrics.export rather than using the same prefix (management.prometheus.metrics.export) as Spring Boot uses for its Prometheus integration in Actuator. It feels like we shouldn't be adding properties to something provided by an external project. Thoughts?
There was a problem hiding this comment.
Thanks @shakuzen ,
For me, I think the main reason of going for management.* would be consistency and convenience of not having to modify existing code that uses those prefixes. I don't think that is the best reasoning though. It is truly a different project and as such I think the best choice would be to break this coupling now and go w/ the micrometer.prometheus.rsocket (or some variant). That would be my vote.
...icrometer/prometheus/rsocket/autoconfigure/PrometheusRSocketClientAutoConfigurationTest.java
Outdated
Show resolved
Hide resolved
|
Based on the discussion on this PR and the feedback we received from the Boot team, I updated the PR to use the |
This PR updates the prefix of the
PrometheusRSocketClientPropertiesconfig props to use the same/similar prefix that Boot does.Specifically, change from
management.metrics.export.prometheus.rsockettomicrometer.prometheus.rsocket.