Add support for multiple paths to DiskSpaceMetrics #2747
Conversation
| public DiskSpaceMetrics(List<File> paths) { | ||
| this(paths, emptyList()); | ||
| } | ||
|
|
||
| public DiskSpaceMetrics(File path, Iterable<Tag> tags) { |
There was a problem hiding this comment.
I was hoping to not add 2 new constructors by editing the existing ones and changing their single path param to a varargs path param. That approach came up short w/ this particular <init> as varargs has to be the last in param list.
I am open to suggestions. I think a builder would be overkill. What are thoughts on deprecation of the single path <init> variants?
There was a problem hiding this comment.
If a user wants disk space metrics for multiple paths, wouldn't binding multiple DiskSpaceMetrics make sense? What is the benefit of the proposed changes over that approach?
Not that it necessarily matters, but I would think a single path is sufficient for most users.
shakuzen
added
the
waiting for feedback
The main reason of going w/ this approach is to keep the auto-config simple by not having to dynamically register N beans as the paths will be config property driven. IOW, doing this @Bean
@ConditionalOnMissingBean
public DiskSpaceMetrics diskSpaceMetrics() {
return new DiskSpaceMetrics(props.getListOfPaths());
}is much simpler than creating an ApplicationContextInitializer (or other means) and registering N of the metrics dynamically via something like props.getListOfPaths().forEach(path -> {
context.registerBean("diskSpaceMetrics" + i, DiskSpaceMetrics.class,
() -> new DiskSpaceMetrics(new File(path)));
} |
onobc
force-pushed
the
diskspace-metrics-multiple-paths
branch
from
d420976 to
2662ce7
Compare
|
@shakuzen I rebased to pickup the move to the |
shakuzen
removed
the
waiting for feedback
| private final Iterable<Tag> tags; | ||
| private final File path; | ||
| private final String absolutePath; | ||
| private final List<File> paths; |
There was a problem hiding this comment.
We could be less specific than List since we don't care about order. How about Collection?
|
|
||
| public DiskSpaceMetrics(File path) { | ||
| this(path, emptyList()); | ||
| } | ||
|
|
||
| public DiskSpaceMetrics(List<File> paths) { |
There was a problem hiding this comment.
While mostly intuitive, a JavaDoc would be nice. I know we're missing one on the existing constructors - I can follow-up with adding that separately. In the case of this constructor, the paths should be to different partitions, since there's no point getting the same information about the same partition for different paths on that partition.
Thanks @shakuzen. Yeh, we are going to handle the multi-paths on the Spring Boot side in a MeterBinder impl that will bind the N DiskSpaceMetrics (your original thought about the impl). |
|
@shakuzen if you think this could be valuable for others then I don't mind making the suggested changes and re-opening. Just let me know. |
|
For the sake of discoverability, the involved Spring Boot PR that will create |
We haven't had a request for it until this. That could partially be due to DiskSpaceMetrics not being auto-configured in Spring Boot until recently. If other users are interested, we can re-visit this. I'd rather see requests from users for this than me assume it's something many users want. |
What
This code proposal will allow consumers to add disk space metrics to multiple paths.
Why
SpringBoot auto-configures a
DiskSpaceMetricsand also has aDiskSpaceHealthContributor. There is a desire to keep their configurability in parity.The health contributor has a request to support multiple paths. The first attempted proposal was closed quite some time ago due to concern of breaking actuator output compatibility (root disk threshold values vs. child path thresholds etc.). I intend to get a 2nd attempt going now that a breaking change should be ok in a minor version bump (2.6).
This change to
DiskSpaceMetricswill be followed with an update to JvmMetricsAutoConfiguration to allow configuration of multiple paths.