Add readTimeout and connectTimeout to NewRelicConfig from PushRegistryConfig

[neiljpowell]

These are in use and should be available/not deprecated for NewRelicMeterRegistry/API Client provider.

Having them in the NewRelicConfig also allows for mapping from NewRelicProperties in Spring Boot. Mapping their values is not currently possible since they aren't mapped in the PushRegistryPropertiesConfigAdapter.

[neiljpowell]

@shakuzen Please consider this for 1.4.x to better support Spring Boot adoption.

[shakuzen]

The reason these were deprecated is because different HttpSender implementations have different meanings for these configurations (connect timeout, read timeout), so it was thought best to deprecate them and let users configure their HttpSender appropriately themselves. However, I do find it inconsistent that we deprecated them but then use the configuration to construct the default HttpSender. Let me think how we want to proceed with these. Un-deprecating them and adding some clarification may be one way out in general - not just for the New Relic registry.

[neiljpowell]

Ok. Ideally, for the NewRelicConfig they would more clearly named as apiConnectTimeout and apiReadTimeout. The Agent configuration is external, even though it also supports proxy settings.

[neiljpowell]

@shakuzen This PR / handling of the read/connectTImeout will be important for the Boot 2.3 adoption, as noted: spring-projects/spring-boot#20839

[neiljpowell]

@shakuzen Based on @snicoll commit to support use of the NewRelicClientProvider, noted in spring-projects/spring-boot#20908, we'll need address proper mapping of the read/connectTimout. They are no longer being set explicitly from the NewRelicProperties. I'm happy to help with the resolution, if different than this PR.

[snicoll]

Good catch @neiljpowell. Given that Micrometer is now in control, I think that some clarification on those properties are in oder. If we find a way to let Spring Boot configures them, I am happy to do that as well.

[neiljpowell]

@shakuzen I'm happy to revert the deprecating of the read/connectTimeout properties instead of this New Relic specific fix, per your comment above.

[jkschneider]

Just want to add some color to the history of these properties...

The 1.0.0 release of Micrometer included connect and read timeout for push-based registries, influenced heavily as it was by Spectator.

Requests started coming in for other forms of HTTP client customization: adding support for proxies, for SSL verification skipping, etc. Proxies seemed reasonable, but if we had included direct property support for SSL verification skipping, I feared that Micrometer would be flagged by vulnerability scanning automation for allowing this. It was starting to feel like we were going to have to add all the options of a conventional HTTP client.

@neiljpowell I think you can see how the same combinatorial explosion of possibilities started happening with NewRelicInsightsApiClientProvider (based on the deprecated constructor including proxy host and port).

The HttpSender interface solved all of these problems at once: it placed things like SSL verification skipping in the hands of the user so Micrometer couldn't be considered "vulnerable", allowed for any arbitrary payload manipulation, proxying, etc.

Then requests started coming in from folks wanting to still get metrics to a SaaS over HTTP, but have the app push them through something like Kafka first! I regret the HttpSender name as a result, but didn't have the imagination to see this use case coming. Really it's more general like a MetricsSender, but the name doesn't influence its utility much.

For 1.1.0 and 1.2.0 we added HttpUrlConnectionSender, a simple implementation whose configuration options didn't need to grow that worked for the vast majority of cases without requiring any third-party HTTP client libraries on the classpath.

@shakuzen The only reason we mapped deprecated connect and read timeout config properties to the HttpUrlConnectionSender constructor was for backwards compatibility. It isn't a signal of these properties' general utility I don't think.

[jkschneider]

I think a more appropriate fix is to address it in Spring Boot where the deprecated property still exists on NewRelicProperties (via its inheritance of PushRegistryProperties). We can conditionally construct the NewRelicMeterRegistry in NewRelicMetricsExportAutoConfiguration like this:

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

[jkschneider]

I believe this should be addressed in Spring Boot's autoconfiguration and not introduce any more HTTP client configuration properties, instead deferring client customization to HttpSender.

[neiljpowell]

@jkschneider Thanks for the background and additional context. Your comments make sense, especially given the interesting integration approaches being used by some. However, in the case of New Relic, HTTP is the primary integration approach for their APIs. Cleanly mapped read and connect timeout defaults/overrides would be expected by users, especially since they have been in Boot's NewRelicProperties/PushRegistryProperties (not deprecated). This PR, plus a small change in Boot's NewRelicPropertiesConfigAdapter for the mapping is all that is needed. Per @snicoll, clean integration via properties mapping is also the preference for Spring Boot vs. the proposed snippet.

The introduction of the NewRelicClientProvider interface and constructor on the NewRelicInsightsApiClientProvider that accepts HttpSender, easily enables the additional integration use cases you mentioned. It's reasonable that this would be the way to get at more "advanced" HttpSender options like proxy config.

Otherwise, if you / @shakuzen really don't want to default the Micrometer New Relic (or other) integration preference, you should consider deprecating Boot's PushRegistryProperties read/connectTimeout properties for consistency.

[shakuzen]

I want to confirm my understanding that nothing is broken right now - Spring Boot users can still configure the timeouts via configuration properties - it just means deprecated methods will be used under the hood. That's something we want to fix but the urgency on fixing it is lower and gives us time to make sure we're making the right change.

I think part of @jkschneider's point was that we don't necessarily need a corresponding Config method for there to be a Spring Boot property. Non-Boot Micrometer users are already instantiating various things themselves: the Config, the Registry - so instantiating an HttpSender how one wants is reasonable. Spring Boot, on the other hand, auto-configures such things typically, so having properties for it that aren't in Micrometer seems reasonable to me. It provides the HttpSender for other registries that take an HttpSender in the corresponding auto-configuration already.

The tricky part here (for the NewRelicMeterRegistry since 1.4) is that the HttpSender should only be provided in a certain combination of configuration. Spring Boot knowing about this specific combination seems like a code smell that could easily break. Perhaps there is a way to work around this without bringing back the timeout methods in a Config class. I'm not sure whether this is more confusing, but one thought I had that is we could remove the fail fast logic here:

micrometer/implementations/micrometer-registry-new-relic/src/main/java/io/micrometer/newrelic/NewRelicMeterRegistry.java

Lines 169 to 172 in 3e1b71d

	if (httpClient != null) {
	if (clientProvider != null) {
	throw new IllegalStateException("Please remove httpClient() configuration as it has been deprecated in favour of clientProvider().");
	}

Unless we're constructing a NewRelicInsightsApiClientProvider when building we ignore the HttpSender configured on the builder . We'd have to un-deprecate the Builder's httpClient method and update the JavaDoc to mention when the HttpSender is used or not. This would allow Spring Boot to simplify its logic and always provide a default HttpSender based on configuration properties, and whether that HttpSender is used or not is part of Micrometer's logic.

[neiljpowell]

@shakuzen The only thing I think that is broken is that timeout overrides provided in NewRelicProperties won't be honored, due to missing mapping from properties to config in the NewRelicPropertiesConfigAdapter/PushRegistryPropertiesConfigAdapter. It won't be obvious to a Boot user that they also need to provide an HttpSender to use override values. A clean and simple approach would be to add the mapping in the Boot NewRelicPropertiesConfigAdapter to the existing deprecated timeouts. That would cleanly map overriden values to the config, as a user would expect, until such time a final approach is determined.

[neiljpowell]

@shakuzen @jkschneider I propose adding the minimal mapping code to the existing deprecated timeouts. This provides expected behavior for now, as mentioned. Thoughts? See: spring-projects/spring-boot#21440

[snicoll]

Given the conversation on the Spring Boot PR and what we've decided to do spring-projects/spring-boot#21578 I don't think we should "undeprecate" those two properties in Micrometer.

[jkschneider]

@snicoll Agreed, I think you've arrived a good solution.