Add connect/readTimout property mapping so overridden values are propagated.

See related PR conversation: https://github.com/micrometer-metrics/micrometer/pull/1961

Comment From: wilkinsona

Thanks for the PR. Reading through the Micrometer discussion and reminding myself of the reason for the property deprecation, I'm not sure that we should map them in Spring Boot. In addition to the use of deprecated API, it would make the NewRelic auto-configuration inconsistent with the various other auto-configurations where we configure an HttpSender with the timeouts:

https://github.com/spring-projects/spring-boot/blob/9be7fa8e1f55a0d8b43a59d54f3af3812cc2d631/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/export/datadog/DatadogMetricsExportAutoConfiguration.java#L67-L73

Ideally, we do something similar for NewRelic but the indirection of NewRelicClientProvider makes this harder. We could mimic the logic in NewRelicMeterRegistry and create the provider based on the config's clientProviderType and passing in an HttpSender but that's jumping through quite a few hoops.

I'm not sure what our best option is here and with 2.3.0.RELEASE due tomorrow, it's unlikely that we'll be able to figure this one out in time. Flagging for team attention nevertheless.

Comment From: neiljpowell

@wilkinsona Hi. Thanks for the quick response. @jkschneider proposed a fix in the auto configuration similar to example you provided in PR 1961 (above). I'm happy to update the PR as such, if you're comfortable with it. CC: @shakuzen

jkschneider proposed fix:

@Bean
@ConditionalOnMissingBean
public NewRelicMeterRegistry newRelicMeterRegistry(NewRelicConfig newRelicConfig, 
  Clock clock, ObjectProvider<NewRelicClientProvider> newRelicClientProvider) {
    Builder builder = NewRelicMeterRegistry.builder(newRelicConfig).clock(clock);
    newRelicClientProvider.ifUnique(builder::clientProvider);

        // this is the fix, using the deprecated properties when no explicit client provider is given?
    if (!newRelicClientProvider.iterator().hasNext() && ClientProviderType.INSIGHTS_API.equals(newRelicConfig.clientProviderType())) {
        builder.clientProvider(new NewRelicInsightsApiClientProvider(newRelicConfig,
                new HttpUrlConnectionSender(newRelicConfig.connectTimeout(), newRelicConfig.readTimeout())));
    }
    return builder.build();
}

Comment From: snicoll

So, we can't do in that direction proposed by that PR. Micrometer has deprecated those methods and the plan is to remove them in Micrometer 2.0 so we shouldn't start using them.

After chatting with @shakuzen we essentially have two options:

  1. We replicate the logic to configure the NewRelicClientProvider based on the type. If the type is set to the Api provider, we configure the HttpSender with timeout values
  2. Micrometer offers an httpSender that we configure regardless of the provider as we do in other places. This would require a change in micrometer and we'd also require that exact version.

I'd like to experiment a bit on 1. and it feels to me it's the more reasonable option.

Comment From: neiljpowell

@snicoll Hi. I've updated the PR based on @jkschneider suggestion but using the NewRelicProperties connect/readTimeouts to avoid use of the deprecated config.

Comment From: snicoll

Thanks @neiljpowell but I am not keen to pursue this suggestion. We should apply configuration on things that we auto-configure and leave user-configured bean untouched.

I've added something that looks relatively minimal in https://github.com/snicoll/spring-boot/commit/3266cd29b1acf8894679b8591b0fe9e5986b4926

@shakuzen what do you think?

Comment From: shakuzen

@snicoll Looks good to me. Thanks.

Comment From: neiljpowell

@snicoll Thank you for helping get the issue resolved. Interestingly, you landed on a simpler version of my original PR for adopting the changes. https://github.com/spring-projects/spring-boot/pull/20871. I will definitely keep that in mind.

Comment From: snicoll

Thanks for the feedback. I am going to decline this and will fix the issue on our side as part of #21578.