Add auto-configuration for Micrometer DiskSpaceMetrics #26001
Conversation
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public DiskSpaceMetrics diskSpaceMetrics() { | ||
| return new DiskSpaceMetrics(new File(System.getProperty("user.dir"))); |
There was a problem hiding this comment.
Would we want to make this configurable? That could easily move it out of the "JVM" category if it was just some random path.
There was a problem hiding this comment.
Given how easy it is to expose it, I am not sure I would make it configurable (at least for a first version). However the disk space health indicator uses the current path (i.e. new File(".")) so I'd rather use that.
...org/springframework/boot/actuate/autoconfigure/metrics/JvmMetricsAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
spring-projects-issues
added
the
status: waiting-for-triage
...org/springframework/boot/actuate/autoconfigure/metrics/JvmMetricsAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
...org/springframework/boot/actuate/autoconfigure/metrics/JvmMetricsAutoConfigurationTests.java
Outdated
Show resolved
Hide resolved
snicoll
added
type: enhancement
| } | ||
|
|
||
| private ContextConsumer<AssertableApplicationContext> validateBaseJvmMetricsBeansArePresentWithCustomBean( | ||
| String customBean) { |
There was a problem hiding this comment.
I wouldn't overuse ContextConsumer like this. The essence of the assertion is to show that a single bean of a given type is present, and the one with the custom name we've added there as well (hence it's used). By moving this to a method that takes "a bean name", the link between the two is less obvious so I don't think this abstraction improves readability.
There was a problem hiding this comment.
Fair enough. I will update sometime today.
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public DiskSpaceMetrics diskSpaceMetrics() { | ||
| return new DiskSpaceMetrics(new File(System.getProperty("user.dir"))); |
There was a problem hiding this comment.
Given how easy it is to expose it, I am not sure I would make it configurable (at least for a first version). However the disk space health indicator uses the current path (i.e. new File(".")) so I'd rather use that.
- Reverted overuse of ContextConsumer - use "." for the directory to report metrics on
|
Thanks again @bono007. |
My pleasure @snicoll |
@philwebb this one is a bit interesting as to where the "home" for the auto-configuration should go for "DiskSpaceMetrics". It is in the Micrometer JVM package but feels a bit more like "System" metrics.
2 reasons I landed it in the JvmMetricsAutoConfiguration:
user.dirwhich is the current working directory the JVM was launched fromFixes gh-25996