Map NewRelicProperties Connect and Read Timeouts To NewRelicConfig

[neiljpowell]

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

See related PR conversation: micrometer-metrics/micrometer#1961

[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:

spring-boot/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/metrics/export/datadog/DatadogMetricsExportAutoConfiguration.java

Lines 67 to 73 in 9be7fa8

	@Bean
	@ConditionalOnMissingBean
	public DatadogMeterRegistry datadogMeterRegistry(DatadogConfig datadogConfig, Clock clock) {
	return DatadogMeterRegistry.builder(datadogConfig).clock(clock).httpClient(
	new HttpUrlConnectionSender(this.properties.getConnectTimeout(), this.properties.getReadTimeout()))
	.build();
	}

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.

[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();
}

[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.

[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.

[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 snicoll@3266cd2

@shakuzen what do you think?

[shakuzen]

@snicoll Looks good to me. Thanks.

[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. #20871. I will definitely keep that in mind.

[snicoll]

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