From 2659601d50473b5268f9d65c7a0b444fbd1c9f77 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 22 Nov 2024 11:12:09 -0500 Subject: [PATCH 1/6] Cache parsed URIs throughout Dialogue Parsing String to URI can be expensive in terms of CPU and allocations for high throughput services. This generalizes the caching previously added to DnsSupport in https://github.com/palantir/dialogue/pull/2398 to also cover ApacheHttpClientBlockingChannel requests and HttpsProxyDefaultRoutePlanner proxy parsing to leverage a shared parsed URI cache. --- .../hc5/ApacheHttpClientBlockingChannel.java | 7 +- .../hc5/HttpsProxyDefaultRoutePlanner.java | 20 ++- .../clients/DialogueDnsResolutionWorker.java | 2 +- .../dialogue/clients/DnsPollingSpec.java | 3 +- .../palantir/dialogue/clients/DnsSupport.java | 67 +-------- dialogue-core/build.gradle | 17 +-- .../java/com/palantir/dialogue/core/Uris.java | 133 ++++++++++++++++++ .../com/palantir/dialogue/core/UrisTest.java | 128 +++++++++++++++++ 8 files changed, 297 insertions(+), 80 deletions(-) create mode 100644 dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java create mode 100644 dialogue-core/src/test/java/com/palantir/dialogue/core/UrisTest.java diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java index 095a7ea1f..44be392da 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java @@ -29,6 +29,7 @@ import com.palantir.dialogue.ResponseAttachments; import com.palantir.dialogue.blocking.BlockingChannel; import com.palantir.dialogue.core.BaseUrl; +import com.palantir.dialogue.core.Uris; import com.palantir.logsafe.Arg; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; @@ -45,6 +46,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.InetAddress; +import java.net.URI; import java.net.URL; import java.util.Arrays; import java.util.Collections; @@ -100,8 +102,9 @@ final class ApacheHttpClientBlockingChannel implements BlockingChannel { public Response execute(Endpoint endpoint, Request request) throws IOException { // Create base request given the URL URL target = baseUrl.render(endpoint, request); + URI targetUri = Uris.tryParse(target.toString()).uriOrThrow(); ClassicRequestBuilder builder = - ClassicRequestBuilder.create(endpoint.httpMethod().name()).setUri(target.toString()); + ClassicRequestBuilder.create(endpoint.httpMethod().name()).setUri(targetUri); // Fill headers request.headerParams().forEach(builder::addHeader); @@ -315,7 +318,7 @@ public int code() { public ListMultimap headers() { if (headers == null) { ListMultimap tmpHeaders = MultimapBuilder.treeKeys(String.CASE_INSENSITIVE_ORDER) - .arrayListValues() + .arrayListValues(1) .build(); Iterator
headerIterator = response.headerIterator(); while (headerIterator.hasNext()) { diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java index d39d345ae..08daccd31 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/HttpsProxyDefaultRoutePlanner.java @@ -17,11 +17,12 @@ package com.palantir.dialogue.hc5; import com.palantir.conjure.java.client.config.HttpsProxies; +import com.palantir.dialogue.core.Uris; +import com.palantir.dialogue.core.Uris.MaybeUri; import java.net.InetSocketAddress; import java.net.Proxy; import java.net.ProxySelector; import java.net.URI; -import java.net.URISyntaxException; import java.util.List; import javax.annotation.CheckForNull; import org.apache.hc.client5.http.impl.routing.DefaultRoutePlanner; @@ -49,12 +50,8 @@ final class HttpsProxyDefaultRoutePlanner extends DefaultRoutePlanner { @Override @CheckForNull public HttpHost determineProxy(final HttpHost target, final HttpContext _context) throws HttpException { - final URI targetUri; - try { - targetUri = new URI(target.toURI()); - } catch (final URISyntaxException ex) { - throw new HttpException("Cannot convert host to URI: " + target, ex); - } + final URI targetUri = parseTargetUri(target); + ProxySelector proxySelectorInstance = this.proxySelector; if (proxySelectorInstance == null) { proxySelectorInstance = ProxySelector.getDefault(); @@ -79,6 +76,15 @@ public HttpHost determineProxy(final HttpHost target, final HttpContext _context return result; } + private static URI parseTargetUri(HttpHost target) throws HttpException { + MaybeUri maybeUri = Uris.tryParse(target.toString()); + if (maybeUri.isSuccessful()) { + return maybeUri.uriOrThrow(); + } else { + throw new HttpException("Cannot convert host to URI: " + target, maybeUri.exception()); + } + } + private Proxy chooseProxy(final List proxies) { Proxy result = null; // check the list for one we can use diff --git a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java index b599d8f31..8214c621e 100644 --- a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java +++ b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java @@ -25,8 +25,8 @@ import com.google.common.collect.SetMultimap; import com.google.common.collect.Sets; import com.google.errorprone.annotations.concurrent.GuardedBy; -import com.palantir.dialogue.clients.DnsSupport.MaybeUri; import com.palantir.dialogue.core.DialogueDnsResolver; +import com.palantir.dialogue.core.Uris.MaybeUri; import com.palantir.logsafe.Safe; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.Unsafe; diff --git a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsPollingSpec.java b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsPollingSpec.java index f6140d7bb..a65d7ff5b 100644 --- a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsPollingSpec.java +++ b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsPollingSpec.java @@ -83,7 +83,8 @@ public String kind() { @Override public Stream extractUris(Optional input) { - return input.stream().flatMap(item -> item.uris().stream()); + return input.map(serviceConfiguration -> serviceConfiguration.uris().stream()) + .orElseGet(Stream::empty); } @Override diff --git a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java index 271d0abe6..984ae0a7e 100644 --- a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java +++ b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java @@ -18,8 +18,6 @@ import com.codahale.metrics.Counter; import com.codahale.metrics.Timer; -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Suppliers; import com.google.common.collect.Collections2; @@ -32,19 +30,18 @@ import com.palantir.dialogue.core.DialogueDnsResolver; import com.palantir.dialogue.core.DialogueExecutors; import com.palantir.dialogue.core.TargetUri; -import com.palantir.logsafe.Preconditions; +import com.palantir.dialogue.core.Uris; +import com.palantir.dialogue.core.Uris.MaybeUri; import com.palantir.logsafe.Safe; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.Unsafe; import com.palantir.logsafe.UnsafeArg; -import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.logger.SafeLogger; import com.palantir.logsafe.logger.SafeLoggerFactory; import com.palantir.refreshable.Disposable; import com.palantir.refreshable.Refreshable; import com.palantir.refreshable.SettableRefreshable; import com.palantir.tritium.metrics.MetricRegistries; -import com.palantir.tritium.metrics.caffeine.CacheStats; import com.palantir.tritium.metrics.registry.SharedTaggedMetricRegistries; import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; import java.lang.ref.Cleaner; @@ -90,22 +87,9 @@ final class DnsSupport { .build(), SCHEDULER_NAME))); - /** - * Shared cache of string to parsed URI. This avoids excessive allocation overhead when parsing repeated targets. - */ - private static final LoadingCache uriCache = CacheStats.of( - SharedTaggedMetricRegistries.getSingleton(), "dialogue-uri") - .register(stats -> Caffeine.newBuilder() - .maximumWeight(100_000) - .weigher((key, _value) -> key.length()) - .expireAfterAccess(Duration.ofMinutes(1)) - .softValues() - .recordStats(stats) - .build(DnsSupport::tryParseUri)); - @VisibleForTesting static void invalidateCaches() { - uriCache.invalidateAll(); + Uris.clearCache(); } /** Identical to the overload, but using the {@link #sharedScheduler}. */ @@ -151,14 +135,8 @@ static Refreshable> pollForChanges( return dnsResolutionResult; } - /** - * This prefix may reconfigure several aspects of the client to work better in a world where requests are routed - * through a service mesh like istio/envoy. - */ - private static final String MESH_PREFIX = "mesh-"; - static boolean isMeshMode(String uri) { - return uri.startsWith(MESH_PREFIX); + return Uris.isMeshMode(uri); } @SuppressWarnings("checkstyle:CyclomaticComplexity") @@ -222,20 +200,14 @@ private static boolean usesProxy(ProxySelector proxySelector, URI uri) { @Unsafe static MaybeUri tryParseUri(@Unsafe String uriString) { - try { - return MaybeUri.success(new URI(uriString)); - } catch (Exception e) { - log.debug("Failed to parse URI", e); - return MaybeUri.failure( - new SafeIllegalArgumentException("Failed to parse URI", e, UnsafeArg.of("uri", uriString))); - } + return Uris.tryParse(uriString); } @Unsafe @Nullable private static URI tryParseUri(TaggedMetricRegistry metrics, @Safe String serviceName, @Unsafe String uri) { try { - MaybeUri maybeUri = uriCache.get(uri); + MaybeUri maybeUri = Uris.tryParse(uri); URI result = maybeUri.uriOrThrow(); if (result.getHost() == null) { log.error( @@ -287,31 +259,4 @@ public void run() { } } } - - @Unsafe - record MaybeUri(@Nullable URI uri, @Nullable SafeIllegalArgumentException exception) { - static @Unsafe DnsSupport.MaybeUri success(URI uri) { - return new MaybeUri(uri, null); - } - - static @Unsafe DnsSupport.MaybeUri failure(SafeIllegalArgumentException exception) { - return new MaybeUri(null, exception); - } - - boolean isSuccessful() { - return uri() != null; - } - - boolean isMeshMode() { - return uri() != null && DnsSupport.isMeshMode(uri().getScheme()); - } - - @Unsafe - URI uriOrThrow() { - if (exception() != null) { - throw exception(); - } - return Preconditions.checkNotNull(uri(), "uri"); - } - } } diff --git a/dialogue-core/build.gradle b/dialogue-core/build.gradle index ad386a123..6a98230a9 100644 --- a/dialogue-core/build.gradle +++ b/dialogue-core/build.gradle @@ -7,20 +7,21 @@ dependencies { api 'com.palantir.conjure.java.runtime:client-config' implementation project(':dialogue-futures') implementation 'com.github.ben-manes.caffeine:caffeine' + implementation 'com.google.code.findbugs:jsr305' + implementation 'com.google.errorprone:error_prone_annotations' implementation 'com.google.guava:guava' + implementation 'com.palantir.conjure.java.api:errors' + implementation 'com.palantir.conjure.java.api:service-config' + implementation 'com.palantir.refreshable:refreshable' implementation 'com.palantir.safe-logging:logger' implementation 'com.palantir.safe-logging:preconditions' implementation 'com.palantir.safe-logging:safe-logging' - implementation 'com.palantir.tracing:tracing' - implementation 'io.dropwizard.metrics:metrics-core' implementation 'com.palantir.safethreadlocalrandom:safe-thread-local-random' - implementation 'com.palantir.tritium:tritium-metrics' - implementation 'com.google.code.findbugs:jsr305' - implementation 'com.google.errorprone:error_prone_annotations' - implementation 'com.palantir.conjure.java.api:errors' - implementation 'com.palantir.conjure.java.api:service-config' + implementation 'com.palantir.tracing:tracing' implementation 'com.palantir.tracing:tracing-api' - implementation 'com.palantir.refreshable:refreshable' + implementation 'com.palantir.tritium:tritium-caffeine' + implementation 'com.palantir.tritium:tritium-metrics' + implementation 'io.dropwizard.metrics:metrics-core' testImplementation 'com.palantir.tracing:tracing-test-utils' testImplementation 'com.palantir.safe-logging:preconditions-assertj' diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java new file mode 100644 index 000000000..0da050321 --- /dev/null +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java @@ -0,0 +1,133 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue.core; + +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; +import com.palantir.dialogue.DialogueImmutablesStyle; +import com.palantir.logsafe.Preconditions; +import com.palantir.logsafe.Unsafe; +import com.palantir.logsafe.UnsafeArg; +import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; +import com.palantir.logsafe.logger.SafeLogger; +import com.palantir.logsafe.logger.SafeLoggerFactory; +import com.palantir.tritium.metrics.caffeine.CacheStats; +import com.palantir.tritium.metrics.registry.SharedTaggedMetricRegistries; +import java.net.URI; +import java.time.Duration; +import javax.annotation.Nullable; +import org.immutables.value.Value; + +public final class Uris { + private static final SafeLogger log = SafeLoggerFactory.get(Uris.class); + + /** + * This prefix may reconfigure several aspects of the client to work better in a world where requests are routed + * through a service mesh like istio/envoy. + */ + private static final String MESH_PREFIX = "mesh-"; + + /** + * Shared cache of string to parsed URI. This avoids excessive allocation overhead when parsing repeated targets. + */ + private static final LoadingCache uriCache = CacheStats.of( + SharedTaggedMetricRegistries.getSingleton(), "dialogue-uri") + .register(stats -> Caffeine.newBuilder() + .maximumWeight(100_000) + .weigher((key, _value) -> key.length()) + .expireAfterAccess(Duration.ofMinutes(1)) + .softValues() + .recordStats(stats) + .build(Uris::parse)); + + @Unsafe + public static MaybeUri tryParse(@Unsafe String uriString) { + return uriCache.get(uriString); + } + + @Unsafe + private static MaybeUri parse(String uriString) { + try { + return MaybeUri.success(new URI(uriString)); + } catch (Exception e) { + log.debug("Failed to parse URI", e); + return MaybeUri.failure( + new SafeIllegalArgumentException("Failed to parse URI", e, UnsafeArg.of("uri", uriString))); + } + } + + public static void clearCache() { + uriCache.invalidateAll(); + } + + /** + * Returns true if the specified URI string is a mesh-mode formatted URI, configured to route through a + * service mesh like istio/envoy. + */ + public static boolean isMeshMode(String uri) { + return uri.startsWith(MESH_PREFIX); + } + + @Unsafe + @Value.Immutable(builder = false) + @DialogueImmutablesStyle + public interface MaybeUri { + @Value.Parameter + @Nullable + URI uri(); + + @Value.Parameter + @Nullable + SafeIllegalArgumentException exception(); + + @Value.Derived + default boolean isSuccessful() { + return uri() != null; + } + + @Value.Derived + default boolean isMeshMode() { + URI uri = uri(); + return uri != null && Uris.isMeshMode(uri.getScheme()); + } + + @Unsafe + @Value.Auxiliary + default URI uriOrThrow() { + SafeIllegalArgumentException exception = exception(); + if (exception != null) { + throw exception; + } + return Preconditions.checkNotNull(uri(), "uri"); + } + + @Value.Check + default void check() { + Preconditions.checkState(uri() != null ^ exception() != null, "Only one of uri or exception can be null"); + } + + static @Unsafe MaybeUri success(URI uri) { + return ImmutableMaybeUri.of(uri, null); + } + + static @Unsafe MaybeUri failure(SafeIllegalArgumentException exception) { + return ImmutableMaybeUri.of(null, exception); + } + } + + private Uris() {} +} diff --git a/dialogue-core/src/test/java/com/palantir/dialogue/core/UrisTest.java b/dialogue-core/src/test/java/com/palantir/dialogue/core/UrisTest.java new file mode 100644 index 000000000..59f27863b --- /dev/null +++ b/dialogue-core/src/test/java/com/palantir/dialogue/core/UrisTest.java @@ -0,0 +1,128 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.dialogue.core; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; + +class UrisTest { + + @BeforeEach + void before() { + Uris.clearCache(); + } + + @AfterEach + void after() { + Uris.clearCache(); + } + + @ParameterizedTest + @ValueSource( + strings = { + "http://www.palantir.com/", + "https://www.palantir.com", + "https://github.com/palantir", + "https://www.example.com/foo/bar/baz", + }) + void parsesValidUris(String input) { + assertThat(Uris.tryParse(input)).isNotNull().satisfies(parsed -> { + assertThat(parsed.isSuccessful()).isTrue(); + assertThat(parsed.isMeshMode()).isFalse(); + assertThat(parsed.exception()).isNull(); + assertThat(parsed.uri()).isNotNull(); + assertThat(parsed.uriOrThrow()).isNotNull(); + }); + } + + @ParameterizedTest + @ValueSource( + strings = { + "mesh-http://www.palantir.com/", + "mesh-https://www.palantir.com", + "mesh-https://github.com/palantir", + "mesh-https://www.example.com/foo/bar/baz", + }) + void parsesMeshUris(String input) { + assertThat(Uris.tryParse(input)).isNotNull().satisfies(parsed -> { + assertThat(parsed.isSuccessful()).isTrue(); + assertThat(parsed.isMeshMode()).isTrue(); + assertThat(parsed.exception()).isNull(); + assertThat(parsed.uri()).isNotNull(); + assertThat(parsed.uriOrThrow()).isNotNull(); + }); + } + + @ParameterizedTest + @ValueSource(strings = {" x ", " ", "foobar://"}) + void parsesInvalidUris(String input) { + assertThat(Uris.tryParse(input)).isNotNull().satisfies(parsed -> { + assertThat(parsed.isSuccessful()).isFalse(); + assertThat(parsed.isMeshMode()).isFalse(); + assertThat(parsed.exception()).isNotNull(); + assertThat(parsed.uri()).isNull(); + assertThatThrownBy(parsed::uriOrThrow) + .isInstanceOf(SafeIllegalArgumentException.class) + .hasMessageContaining("Failed to parse URI"); + }); + } + + @ParameterizedTest + @CsvSource({ + "github.com,https://github.com", + "github.com,https://github.com:443", + "github.com,https://github.com:8080", + "github.com,https://github.com/palantir/dialogue/", + "github.com,https://github.com:443/palantir/dialogue/", + "github.com,https://github.com:8080/palantir/dialogue/", + "github.com,mesh-https://github.com/palantir/dialogue/", + "github.com,mesh-https://github.com:443/palantir/dialogue/", + "github.com,mesh-https://github.com:8080/palantir/dialogue/", + }) + void tryGetHost(String expectedHostname, String input) { + assertThat(Uris.tryParse(input)).satisfies(parsed -> { + assertThat(parsed.isSuccessful()).isTrue(); + assertThat(parsed.uri()).isNotNull(); + assertThat(parsed.uri().getHost()).isEqualTo(expectedHostname); + assertThat(parsed.exception()).isNull(); + assertThat(parsed.isMeshMode()).isEqualTo(input.startsWith("mesh-")); + }); + } + + @ParameterizedTest + @CsvSource({ + "false,https://github.com", + "false,https://github.com:443", + "false,https://github.com:8080", + "false,https://github.com/palantir/dialogue/", + "false,https://github.com:443/palantir/dialogue/", + "false,https://github.com:8080/palantir/dialogue/", + "true,mesh-https://github.com/palantir/dialogue/", + "true,mesh-https://github.com:443/palantir/dialogue/", + "true,mesh-https://github.com:8080/palantir/dialogue/", + }) + void isMeshMode(boolean expected, String input) { + assertThat(Uris.isMeshMode(input)).isEqualTo(expected).isEqualTo(Uris.isMeshMode(input)); + } +} From 222238ea49e9f42d291b4ce289a939f344a7d75f Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 22 Nov 2024 11:21:02 -0500 Subject: [PATCH 2/6] Cleanup DnsSupport --- .../clients/DialogueDnsResolutionWorker.java | 3 +- .../palantir/dialogue/clients/DnsSupport.java | 21 +---- .../DefaultDialogueDnsResolverTest.java | 6 +- .../dialogue/clients/DnsSupportTest.java | 86 ------------------- 4 files changed, 7 insertions(+), 109 deletions(-) delete mode 100644 dialogue-clients/src/test/java/com/palantir/dialogue/clients/DnsSupportTest.java diff --git a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java index 8214c621e..0ec2df87e 100644 --- a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java +++ b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DialogueDnsResolutionWorker.java @@ -26,6 +26,7 @@ import com.google.common.collect.Sets; import com.google.errorprone.annotations.concurrent.GuardedBy; import com.palantir.dialogue.core.DialogueDnsResolver; +import com.palantir.dialogue.core.Uris; import com.palantir.dialogue.core.Uris.MaybeUri; import com.palantir.logsafe.Safe; import com.palantir.logsafe.SafeArg; @@ -103,7 +104,7 @@ private synchronized void doUpdate(@Nullable INPUT updatedInputState) { long start = System.nanoTime(); ImmutableSet allHosts = spec.extractUris(inputState) .filter(Objects::nonNull) - .map(DnsSupport::tryParseUri) + .map(Uris::tryParse) .filter(MaybeUri::isSuccessful) .filter(Predicate.not(MaybeUri::isMeshMode)) .map(MaybeUri::uri) diff --git a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java index 984ae0a7e..b72136c35 100644 --- a/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java +++ b/dialogue-clients/src/main/java/com/palantir/dialogue/clients/DnsSupport.java @@ -18,7 +18,6 @@ import com.codahale.metrics.Counter; import com.codahale.metrics.Timer; -import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Suppliers; import com.google.common.collect.Collections2; import com.google.common.collect.ImmutableList; @@ -31,7 +30,6 @@ import com.palantir.dialogue.core.DialogueExecutors; import com.palantir.dialogue.core.TargetUri; import com.palantir.dialogue.core.Uris; -import com.palantir.dialogue.core.Uris.MaybeUri; import com.palantir.logsafe.Safe; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.Unsafe; @@ -87,11 +85,6 @@ final class DnsSupport { .build(), SCHEDULER_NAME))); - @VisibleForTesting - static void invalidateCaches() { - Uris.clearCache(); - } - /** Identical to the overload, but using the {@link #sharedScheduler}. */ static Refreshable> pollForChanges( boolean dnsNodeDiscovery, @@ -135,10 +128,6 @@ static Refreshable> pollForChanges( return dnsResolutionResult; } - static boolean isMeshMode(String uri) { - return Uris.isMeshMode(uri); - } - @SuppressWarnings("checkstyle:CyclomaticComplexity") static ImmutableList getTargetUris( @Safe String serviceNameForLogging, @@ -159,7 +148,7 @@ static ImmutableList getTargetUris( // are considered equivalent. // When a proxy is used, pre-resolved IP addresses have no impact. In many cases the // proxy handles DNS resolution. - if (resolvedHosts.isEmpty() || DnsSupport.isMeshMode(uri) || usesProxy(proxySelector, parsed)) { + if (resolvedHosts.isEmpty() || Uris.isMeshMode(uri) || usesProxy(proxySelector, parsed)) { targetUris.add(TargetUri.of(uri)); } else { String host = parsed.getHost(); @@ -198,17 +187,11 @@ private static boolean usesProxy(ProxySelector proxySelector, URI uri) { } } - @Unsafe - static MaybeUri tryParseUri(@Unsafe String uriString) { - return Uris.tryParse(uriString); - } - @Unsafe @Nullable private static URI tryParseUri(TaggedMetricRegistry metrics, @Safe String serviceName, @Unsafe String uri) { try { - MaybeUri maybeUri = Uris.tryParse(uri); - URI result = maybeUri.uriOrThrow(); + URI result = Uris.tryParse(uri).uriOrThrow(); if (result.getHost() == null) { log.error( "Failed to correctly parse URI {} for service {} due to null host component. " diff --git a/dialogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java b/dialogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java index 4f9a9e443..25c9329ca 100644 --- a/dialogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java +++ b/dialogue-clients/src/test/java/com/palantir/dialogue/clients/DefaultDialogueDnsResolverTest.java @@ -86,7 +86,7 @@ void unknown_host() { assertThat(result).isEmpty(); ClientDnsMetrics metrics = ClientDnsMetrics.of(registry); - assertThat(metrics.failure("EAI_NONAME").getCount()).isEqualTo(1); + assertThat(metrics.failure("EAI_NONAME").getCount()).isOne(); } @Test @@ -99,12 +99,12 @@ void unknown_host_from_cache() { ImmutableSet result = resolver.resolve(badHost); assertThat(result).isEmpty(); - assertThat(metrics.failure("EAI_NONAME").getCount()).isEqualTo(1); + assertThat(metrics.failure("EAI_NONAME").getCount()).isOne(); // should resolve from cache ImmutableSet result2 = resolver.resolve(badHost); assertThat(result2).isEmpty(); - assertThat(metrics.failure("CACHED").getCount()).isEqualTo(1); + assertThat(metrics.failure("CACHED").getCount()).isOne(); } private static ImmutableSet resolve(String hostname) { diff --git a/dialogue-clients/src/test/java/com/palantir/dialogue/clients/DnsSupportTest.java b/dialogue-clients/src/test/java/com/palantir/dialogue/clients/DnsSupportTest.java deleted file mode 100644 index a5bf89b54..000000000 --- a/dialogue-clients/src/test/java/com/palantir/dialogue/clients/DnsSupportTest.java +++ /dev/null @@ -1,86 +0,0 @@ -/* - * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.palantir.dialogue.clients; - -import static org.assertj.core.api.Assertions.assertThat; - -import com.palantir.dialogue.core.TargetUri; -import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry; -import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; -import java.net.URI; -import java.util.List; -import java.util.Optional; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.CsvSource; - -class DnsSupportTest { - - private final TaggedMetricRegistry taggedMetrics = new DefaultTaggedMetricRegistry(); - - @BeforeEach - void before() { - DnsSupport.invalidateCaches(); - } - - @ParameterizedTest - @CsvSource({ - "github.com,https://github.com", - "github.com,https://github.com:443", - "github.com,https://github.com:8080", - "github.com,https://github.com/palantir/dialogue/", - "github.com,https://github.com:443/palantir/dialogue/", - "github.com,https://github.com:8080/palantir/dialogue/", - "github.com,mesh-https://github.com/palantir/dialogue/", - "github.com,mesh-https://github.com:443/palantir/dialogue/", - "github.com,mesh-https://github.com:8080/palantir/dialogue/", - }) - void tryGetHost(String expectedHostname, String input) { - assertThat(DnsSupport.tryParseUri(input)).satisfies(parsed -> { - assertThat(parsed.isSuccessful()).isTrue(); - assertThat(parsed.uri()).isNotNull(); - assertThat(parsed.uri().getHost()).isEqualTo(expectedHostname); - assertThat(parsed.exception()).isNull(); - assertThat(parsed.isMeshMode()).isEqualTo(input.startsWith("mesh-")); - }); - - assertThat(DnsSupport.getTargetUris( - "test", - List.of(input), - DnsSupport.proxySelector(Optional.empty()), - Optional.empty(), - taggedMetrics)) - .hasSize(1) - .containsExactly(TargetUri.of(URI.create(input).toString())); - } - - @ParameterizedTest - @CsvSource({ - "false,https://github.com", - "false,https://github.com:443", - "false,https://github.com:8080", - "false,https://github.com/palantir/dialogue/", - "false,https://github.com:443/palantir/dialogue/", - "false,https://github.com:8080/palantir/dialogue/", - "true,mesh-https://github.com/palantir/dialogue/", - "true,mesh-https://github.com:443/palantir/dialogue/", - "true,mesh-https://github.com:8080/palantir/dialogue/", - }) - void isMeshMode(boolean expected, String input) { - assertThat(DnsSupport.isMeshMode(input)).isEqualTo(expected).isEqualTo(DnsSupport.isMeshMode(input)); - } -} From 239b100e1de70f6e076aaaef4cef8172bb953731 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 22 Nov 2024 11:27:54 -0500 Subject: [PATCH 3/6] Do not cache stacktrace elements --- .../main/java/com/palantir/dialogue/core/Uris.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java index 0da050321..b3baad47e 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java @@ -19,6 +19,7 @@ import com.github.benmanes.caffeine.cache.Caffeine; import com.github.benmanes.caffeine.cache.LoadingCache; import com.palantir.dialogue.DialogueImmutablesStyle; +import com.palantir.logsafe.Arg; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.Unsafe; import com.palantir.logsafe.UnsafeArg; @@ -41,6 +42,9 @@ public final class Uris { */ private static final String MESH_PREFIX = "mesh-"; + private static final Arg[] EMPTY_ARGS = new Arg[0]; + private static final StackTraceElement[] EMPTY_STACKTRACE = new StackTraceElement[0]; + /** * Shared cache of string to parsed URI. This avoids excessive allocation overhead when parsing repeated targets. */ @@ -110,7 +114,10 @@ default boolean isMeshMode() { default URI uriOrThrow() { SafeIllegalArgumentException exception = exception(); if (exception != null) { - throw exception; + throw new SafeIllegalArgumentException( + exception.getLogMessage(), + exception, + exception.getArgs().toArray(EMPTY_ARGS)); } return Preconditions.checkNotNull(uri(), "uri"); } @@ -125,6 +132,8 @@ default void check() { } static @Unsafe MaybeUri failure(SafeIllegalArgumentException exception) { + // do not cache stacktrace elements + exception.setStackTrace(EMPTY_STACKTRACE); return ImmutableMaybeUri.of(null, exception); } } From 947a7ac2005ee5811c037875f827eed8764ea8e2 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Fri, 22 Nov 2024 16:28:36 +0000 Subject: [PATCH 4/6] Add generated changelog entries --- changelog/@unreleased/pr-2432.v2.yml | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 changelog/@unreleased/pr-2432.v2.yml diff --git a/changelog/@unreleased/pr-2432.v2.yml b/changelog/@unreleased/pr-2432.v2.yml new file mode 100644 index 000000000..47023e11a --- /dev/null +++ b/changelog/@unreleased/pr-2432.v2.yml @@ -0,0 +1,9 @@ +type: improvement +improvement: + description: Parsing String to URI can be expensive in terms of CPU and allocations + for high throughput services. This generalizes the caching previously added to + DnsSupport in https://github.com/palantir/dialogue/pull/2398 to also cover ApacheHttpClientBlockingChannel + requests and HttpsProxyDefaultRoutePlanner proxy parsing to leverage a shared + parsed URI cache. + links: + - https://github.com/palantir/dialogue/pull/2432 From 28a5e14007d9467984e9f70133975a03f4a82404 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Sat, 23 Nov 2024 04:23:55 -0500 Subject: [PATCH 5/6] constant message --- .../src/main/java/com/palantir/dialogue/core/Uris.java | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java b/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java index b3baad47e..cf8e3ff4b 100644 --- a/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java +++ b/dialogue-core/src/main/java/com/palantir/dialogue/core/Uris.java @@ -42,9 +42,6 @@ public final class Uris { */ private static final String MESH_PREFIX = "mesh-"; - private static final Arg[] EMPTY_ARGS = new Arg[0]; - private static final StackTraceElement[] EMPTY_STACKTRACE = new StackTraceElement[0]; - /** * Shared cache of string to parsed URI. This avoids excessive allocation overhead when parsing repeated targets. */ @@ -115,9 +112,7 @@ default URI uriOrThrow() { SafeIllegalArgumentException exception = exception(); if (exception != null) { throw new SafeIllegalArgumentException( - exception.getLogMessage(), - exception, - exception.getArgs().toArray(EMPTY_ARGS)); + "Failed to parse URI", exception, exception.getArgs().toArray(new Arg[0])); } return Preconditions.checkNotNull(uri(), "uri"); } @@ -133,7 +128,7 @@ default void check() { static @Unsafe MaybeUri failure(SafeIllegalArgumentException exception) { // do not cache stacktrace elements - exception.setStackTrace(EMPTY_STACKTRACE); + exception.setStackTrace(new StackTraceElement[0]); return ImmutableMaybeUri.of(null, exception); } } From 64733f7ffc5ca9b9b2e655603741a034dee88384 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Sat, 23 Nov 2024 04:47:57 -0500 Subject: [PATCH 6/6] Use URL to set request scheme/authority/path --- .../hc5/ApacheHttpClientBlockingChannel.java | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java index 44be392da..26c7df2bd 100644 --- a/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java +++ b/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientBlockingChannel.java @@ -29,12 +29,13 @@ import com.palantir.dialogue.ResponseAttachments; import com.palantir.dialogue.blocking.BlockingChannel; import com.palantir.dialogue.core.BaseUrl; -import com.palantir.dialogue.core.Uris; import com.palantir.logsafe.Arg; import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; import com.palantir.logsafe.SafeLoggable; +import com.palantir.logsafe.UnsafeArg; import com.palantir.logsafe.exceptions.SafeExceptions; +import com.palantir.logsafe.exceptions.SafeIllegalArgumentException; import com.palantir.logsafe.exceptions.SafeRuntimeException; import com.palantir.logsafe.logger.SafeLogger; import com.palantir.logsafe.logger.SafeLoggerFactory; @@ -46,7 +47,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.net.InetAddress; -import java.net.URI; +import java.net.URISyntaxException; import java.net.URL; import java.util.Arrays; import java.util.Collections; @@ -69,6 +70,7 @@ import org.apache.hc.core5.http.NoHttpResponseException; import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; import org.apache.hc.core5.http.message.BasicHeader; +import org.apache.hc.core5.net.URIAuthority; final class ApacheHttpClientBlockingChannel implements BlockingChannel { private static final SafeLogger log = SafeLoggerFactory.get(ApacheHttpClientBlockingChannel.class); @@ -102,9 +104,11 @@ final class ApacheHttpClientBlockingChannel implements BlockingChannel { public Response execute(Endpoint endpoint, Request request) throws IOException { // Create base request given the URL URL target = baseUrl.render(endpoint, request); - URI targetUri = Uris.tryParse(target.toString()).uriOrThrow(); - ClassicRequestBuilder builder = - ClassicRequestBuilder.create(endpoint.httpMethod().name()).setUri(targetUri); + ClassicRequestBuilder builder = ClassicRequestBuilder.create( + endpoint.httpMethod().name()) + .setScheme(target.getProtocol()) + .setAuthority(parseAuthority(target)) + .setPath(target.getFile()); // Fill headers request.headerParams().forEach(builder::addHeader); @@ -170,6 +174,14 @@ public Response execute(Endpoint endpoint, Request request) throws IOException { } } + private static URIAuthority parseAuthority(URL url) { + try { + return URIAuthority.create(url.getAuthority()); + } catch (URISyntaxException e) { + throw new SafeIllegalArgumentException("Invalid URI authority", e, UnsafeArg.of("url", url)); + } + } + private Arg[] failureDiagnosticArgs(Endpoint endpoint, Request request, long startTimeNanos) { long durationMillis = TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - startTimeNanos); return new Arg[] {