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

Provide a configuration property for setting the path used by auto-configured disk space metrics #27306

Closed
wilkinsona opened this issue Jul 13, 2021 · 4 comments
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@wilkinsona
Copy link
Member

The path for disk space metrics is hardcoded as . but we allow the path for the disk space health indicator to be auto-configured. I think the path for the metrics should be equally configurable. I wonder if it might be possible to use the same property for both as I suspect it's unlikely that you'd want the two to be different.

When looking at this, we should keep the request to support multiple paths for the disk space health check in mind. I suspect that the requirement to have a health check for multiple paths would apply equally to metrics.

@wilkinsona wilkinsona added type: enhancement A general enhancement status: pending-design-work Needs design work before any code can be developed for: team-meeting An issue we'd like to discuss as a team to make progress labels Jul 13, 2021
@wilkinsona wilkinsona removed the for: team-meeting An issue we'd like to discuss as a team to make progress label Jul 21, 2021
@wilkinsona wilkinsona added this to the 2.6.x milestone Jul 21, 2021
@onobc
Copy link
Contributor

onobc commented Aug 13, 2021

@wilkinsona

QUESTION: Somewhat unrelated, but I have been curious about this before. Are DiskSpaceMetrics really JVM metrics, or more aligned w/ SystemMetrics? The DiskSpaceHealthIndicator seems to align more w/ the latter as it lives in a system sub-package. Either way, it is probably a question better suited to Micrometer as it owns DiskSpaceMetrics and is what put it in the jvm package to begin with.

ASSUMPTION: I am assuming a breaking change is ok as this is going into 2.6.

THOUGHTS

So this is interesting as the thought of sharing the same property would be the first bridge of a metric and a health indicator properties that I am aware of. This is the typical layout of the properties if they are not shared:

  • DiskSpaceHealthIndicatorProperties -> "management.health.diskspace.path"
  • MetricsProperties (typical metrics property home) -> "management.metrics.diskspace.path"

I also took a read through the request to support multiple paths for the disk space health check and that would also complicate a sharing of properties as it requires additional settings for threshold. One possible unshared properties layout might look like:

management.metrics.diskspace.paths: a,b,c

management.health.diskspace.paths:
  - path: a
    threshold: .8
  - path: b 
    threshold: .7
  - path: c
    threshold: .5

I am thinking that it may be better to keep these separated. I would think the number of paths configured would be pretty small and therefore having to configure them twice is probably not that much of a pain. Do we have any metrics or idea on how heavily these 2 features are used?

@wilkinsona
Copy link
Member Author

Somewhat unrelated, but I have been curious about this before. Are DiskSpaceMetrics really JVM metrics, or more aligned w/ SystemMetrics? The DiskSpaceHealthIndicator seems to align more w/ the latter as it lives in a system sub-package. Either way, it is probably a question better suited to Micrometer as it owns DiskSpaceMetrics and is what put it in the jvm package to begin with.

No, I don't think they are. IMO, they're system metrics similar to FileDescriptorMetrics. I've opened #27688 to move them in Boot to SystemMetricsAutoConfiguration and micrometer-metrics/micrometer#2750 to move them in Micrometer.

@wilkinsona
Copy link
Member Author

We discussed this today and came to the conclusion that we'd like to get rid of the disk space health indicator. With hindsight, the available disk space should really have been exposed as a metric with the threshold configured in the monitoring system to alert when the disk space gets too low. In the fullness of time, #21311 will also make it possible to include diskspace in a service-level objective that's mapping to a health indicator.

Given the above, we think the best course of action here is to provide a metrics-specific property for the disk space paths and for it to support multiple paths. We'll leave the disk space health side of things unchanged with a view to it being removed in 3.0.

@snicoll
Copy link
Member

snicoll commented Sep 6, 2021

Closing in favor of PR #27660

@snicoll snicoll closed this as completed Sep 6, 2021
@snicoll snicoll added status: superseded An issue that has been superseded by another and removed status: pending-design-work Needs design work before any code can be developed labels Sep 6, 2021
@snicoll snicoll removed this from the 2.6.x milestone Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants