diff --git a/changelog/@unreleased/pr-792.v2.yml b/changelog/@unreleased/pr-792.v2.yml new file mode 100644 index 000000000..82aa41d00 --- /dev/null +++ b/changelog/@unreleased/pr-792.v2.yml @@ -0,0 +1,10 @@ +type: improvement +improvement: + description: |- + Socket handshake metrics are not recorded by default to prevent new threads for each handshake + + Metrics and logging can be enabled by either enabling debug logging + on `com.palantir.tritium.metrics.HandshakeInstrumentation` or + by setting the `instrument.tls.socket` property to `true`. + links: + - https://github.com/palantir/tritium/pull/792 diff --git a/tritium-metrics/src/main/java/com/palantir/tritium/metrics/HandshakeInstrumentation.java b/tritium-metrics/src/main/java/com/palantir/tritium/metrics/HandshakeInstrumentation.java index 29d43ab25..c5786f8e6 100644 --- a/tritium-metrics/src/main/java/com/palantir/tritium/metrics/HandshakeInstrumentation.java +++ b/tritium-metrics/src/main/java/com/palantir/tritium/metrics/HandshakeInstrumentation.java @@ -17,6 +17,7 @@ package com.palantir.tritium.metrics; import com.palantir.logsafe.SafeArg; +import com.palantir.tritium.event.InstrumentationProperties; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -44,5 +45,14 @@ static void record(TlsMetrics metrics, String contextName, String cipherSuite, S } } + /** + * Socket metrics are more expensive than SSLEngine instrumentation due to HandshakeCompletedListener + * instances running on a new short-lived thread. When no HandshakeCompletedListeners are registered, + * the thread is completely avoided. + */ + static boolean isSocketInstrumentationEnabled() { + return log.isDebugEnabled() || InstrumentationProperties.isSpecificEnabled("tls.socket", false); + } + private HandshakeInstrumentation() {} } diff --git a/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslServerSocketFactory.java b/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslServerSocketFactory.java index 86db6e2c4..81aaad95e 100644 --- a/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslServerSocketFactory.java +++ b/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslServerSocketFactory.java @@ -222,7 +222,7 @@ public Socket accept() throws IOException { } private Socket wrap(Socket socket) { - if (socket instanceof SSLSocket && InstrumentedSslSocketFactory.tlsMetricsEnabled.getAsBoolean()) { + if (socket instanceof SSLSocket && HandshakeInstrumentation.isSocketInstrumentationEnabled()) { ((SSLSocket) socket).addHandshakeCompletedListener(listener); } return socket; diff --git a/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslSocketFactory.java b/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslSocketFactory.java index df4c57647..08a639332 100644 --- a/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslSocketFactory.java +++ b/tritium-metrics/src/main/java/com/palantir/tritium/metrics/InstrumentedSslSocketFactory.java @@ -16,20 +16,17 @@ package com.palantir.tritium.metrics; -import com.palantir.tritium.event.InstrumentationProperties; import java.io.IOException; import java.io.InputStream; import java.net.InetAddress; import java.net.Socket; import java.util.Objects; -import java.util.function.BooleanSupplier; import javax.net.ssl.HandshakeCompletedListener; import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory; final class InstrumentedSslSocketFactory extends SSLSocketFactory { - static final BooleanSupplier tlsMetricsEnabled = InstrumentationProperties.getSystemPropertySupplier("tls.socket"); private final SSLSocketFactory delegate; private final HandshakeCompletedListener listener; private final String name; @@ -109,7 +106,7 @@ public int hashCode() { } private Socket wrap(Socket socket) { - if (socket instanceof SSLSocket && tlsMetricsEnabled.getAsBoolean()) { + if (socket instanceof SSLSocket && HandshakeInstrumentation.isSocketInstrumentationEnabled()) { ((SSLSocket) socket).addHandshakeCompletedListener(listener); } return socket; diff --git a/tritium-metrics/src/test/java/com/palantir/tritium/metrics/InstrumentedSslContextTest.java b/tritium-metrics/src/test/java/com/palantir/tritium/metrics/InstrumentedSslContextTest.java index 30cff35b2..bda68c1c8 100644 --- a/tritium-metrics/src/test/java/com/palantir/tritium/metrics/InstrumentedSslContextTest.java +++ b/tritium-metrics/src/test/java/com/palantir/tritium/metrics/InstrumentedSslContextTest.java @@ -20,6 +20,7 @@ import static org.assertj.core.api.Assumptions.assumeThat; import com.google.common.collect.MoreCollectors; +import com.palantir.tritium.event.InstrumentationProperties; import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry; import com.palantir.tritium.metrics.registry.MetricName; import com.palantir.tritium.metrics.registry.TaggedMetricRegistry; @@ -47,6 +48,8 @@ import okhttp3.Protocol; import okhttp3.Request; import okhttp3.Response; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.xnio.Options; import org.xnio.Sequence; @@ -59,6 +62,18 @@ final class InstrumentedSslContextTest { private static final int PORT = 4483; + @BeforeEach + void beforeEach() { + System.setProperty("instrument.tls.socket", "true"); + InstrumentationProperties.reload(); + } + + @AfterEach + void afterEach() { + System.clearProperty("instrument.tls.socket"); + InstrumentationProperties.reload(); + } + @Test void testClientInstrumentationHttpsUrlConnection() throws Exception { TaggedMetricRegistry metrics = new DefaultTaggedMetricRegistry();