@wilkinsona I was playing around w/ the options listed in the ticket and had this simple approach coded up so went ahead and submitted this proposal. I know I did not wait for feedback on the ticket and am more than happy to close this out if If you end up leaning another direction.
This goes w/ the simple approach of not sharing properties w/ the DiskSpaceHealthIndicator
and only configures a single path for now (in the MetricsProperties
).
Fixed gh-27306
Comment From: onobc
@wilkinsona I rebased and moved from jvm->system
Comment From: wilkinsona
@bono007 Are you interested in reworking this to support multiple paths? In light of https://github.com/spring-projects/spring-boot/issues/27306#issuecomment-903907394, that can be done without considering the disk space health indicator. I think we'd probably end up with a MetricBinder
implementation that consumes the paths. When called, it would create one DiskSpaceMetrics
instance per path and binds it to the registry.
Comment From: onobc
@bono007 Are you interested in reworking this to support multiple paths? In light of #27306 (comment), that can be done without considering the disk space health indicator. I think we'd probably end up with a
MetricBinder
implementation that consumes the paths. When called, it would create oneDiskSpaceMetrics
instance per path and binds it to the registry.
You know I am 😺
I like the MeterBinder -> N DiskSpaceMetrics approach as well. This also allows us to close https://github.com/micrometer-metrics/micrometer/pull/2747#issuecomment-903915522 .
I will get to it in the next 1-2 days. That timeline work?
Comment From: wilkinsona
You know I am 😺
😀 I didn’t want to assume you would be
I will get to it in the next 1-2 days. That timeline work?
Absolutely. It’d be good to get the change in before RC1.
Comment From: snicoll
Thanks Chris, that's excellent. FYI, you might be interested by the polish commit, in particular:
- The
List
configuration property must be mutable if the binder needs to append to the list depending on the configuration style - The annotation processor can't detect a default value expressed this way so I've added a manual hint.
Comment From: onobc
Thanks Chris, that's excellent. FYI, you might be interested by the polish commit, in particular:
You're welcome Stéphane. And thank you for taking the time to point me to the polish commit and outline what changed - much appreciated.
- The
List
configuration property must be mutable if the binder needs to append to the list depending on the configuration style
Yep, I know about this one and was almost certain that Arrays.asList
produced a mutable list as the code looks like this:
``java
public static <T> List<T> asList(T... a) {
return new ArrayList<>(a);
}
````
⚠️ However, looking closer one will notice that is not a
java.util..ArrayListit returns but rather a
java.util.Arrays.ArrayList` private imutable impl. Wow, thanks for the clever naming JDK8.
- The annotation processor can't detect a default value expressed this way so I've added a manual hint.
Good catch. I think this crossed my mind at one point as something to follow up on as I was unsure if it would find that complex default (obviously I forgot to circle back :) )