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 #27660

Closed
wants to merge 2 commits into from
Closed

Conversation

onobc
Copy link
Contributor

@onobc onobc commented Aug 15, 2021

@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

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 15, 2021
@onobc
Copy link
Contributor Author

onobc commented Aug 20, 2021

@wilkinsona I rebased and moved from jvm->system

@wilkinsona
Copy link
Member

@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 one DiskSpaceMetrics instance per path and binds it to the registry.

@onobc
Copy link
Contributor Author

onobc commented Aug 23, 2021

@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 one DiskSpaceMetrics 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 micrometer-metrics/micrometer#2747 (comment) .

I will get to it in the next 1-2 days. That timeline work?

@wilkinsona
Copy link
Member

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.

@onobc onobc requested a review from wilkinsona August 25, 2021 03:07
@snicoll snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 6, 2021
@snicoll snicoll added this to the 2.6.x milestone Sep 6, 2021
@onobc onobc requested a review from wilkinsona September 6, 2021 13:31
@snicoll snicoll changed the title Add configuration for DiskSpaceMetrics path Provide a configuration property for setting the path used by auto-configured disk space metrics Sep 16, 2021
@snicoll snicoll self-assigned this Sep 16, 2021
@snicoll snicoll closed this in e231f65 Sep 16, 2021
@snicoll snicoll modified the milestones: 2.6.x, 2.6.0-M3 Sep 16, 2021
@snicoll
Copy link
Member

snicoll commented Sep 16, 2021

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.

@onobc
Copy link
Contributor Author

onobc commented Sep 16, 2021

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:

    public static <T> List<T> asList(T... a) {
        return new ArrayList<>(a);
    }

⚠️ However, looking closer one will notice that is not a java.util..ArrayList<T> it 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 :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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