From 484acb8140f4fc6d78cdd8d684f2a9b69d9fdf06 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Wed, 30 Dec 2020 17:28:03 +0100 Subject: [PATCH 1/5] Resolve servername from the localhost address. Fixes #1109. --- CHANGELOG.md | 1 + .../spring/boot/SentryAutoConfiguration.java | 6 + .../boot/SentryAutoConfigurationTest.kt | 9 ++ sentry/api/sentry.api | 5 + .../ServerNameResolvingEventProcessor.java | 134 ++++++++++++++++++ .../ServerNameResolvingEventProcessorTest.kt | 86 +++++++++++ 6 files changed, 241 insertions(+) create mode 100644 sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java create mode 100644 sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index c9c50978838..8a78e1bc99d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * Fix: Initialize Logback after context refreshes (#1129) * Ref: Return NoOpTransaction instead of null (#1126) * Fix: Do not crash when passing null values to @Nullable methods, eg User and Scope +* Feat: Resolve servername from the localhost address # 4.0.0-alpha.2 diff --git a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java index 6f185053da1..3e9a1bd148a 100644 --- a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java +++ b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java @@ -7,6 +7,7 @@ import io.sentry.Integration; import io.sentry.Sentry; import io.sentry.SentryOptions; +import io.sentry.ServerNameResolvingEventProcessor; import io.sentry.protocol.SdkVersion; import io.sentry.spring.SentryExceptionResolver; import io.sentry.spring.SentryRequestResolver; @@ -92,6 +93,11 @@ static class HubConfiguration { return new InAppIncludesResolver(); } + @Bean + public @NotNull ServerNameResolvingEventProcessor serverNameResolvingEventProcessor() { + return new ServerNameResolvingEventProcessor(); + } + @Bean public @NotNull IHub sentryHub( final @NotNull List> optionsConfigurations, diff --git a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt index 61febd0a9ee..8989009b3de 100644 --- a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt +++ b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt @@ -13,6 +13,7 @@ import io.sentry.Sentry import io.sentry.SentryEvent import io.sentry.SentryLevel import io.sentry.SentryOptions +import io.sentry.ServerNameResolvingEventProcessor import io.sentry.protocol.User import io.sentry.spring.HttpServletRequestSentryUserProvider import io.sentry.spring.SentryUserProvider @@ -452,6 +453,14 @@ class SentryAutoConfigurationTest { } } + @Test + fun `autoconfigures ServerNameResolvingEventProcessor`() { + contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj") + .run { + assertThat(it).hasSingleBean(ServerNameResolvingEventProcessor::class.java) + } + } + @Configuration(proxyBeanMethods = false) open class CustomOptionsConfigurationConfiguration { diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index fc39d98b396..c1841f294dc 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -804,6 +804,11 @@ public final class io/sentry/SentryTransaction : io/sentry/SentryBaseEvent, io/s public fun toSentryTrace ()Lio/sentry/SentryTraceHeader; } +public final class io/sentry/ServerNameResolvingEventProcessor : io/sentry/EventProcessor { + public fun ()V + public fun process (Lio/sentry/SentryEvent;Ljava/lang/Object;)Lio/sentry/SentryEvent; +} + public final class io/sentry/Session { public fun (Lio/sentry/Session$State;Ljava/util/Date;Ljava/util/Date;ILjava/lang/String;Ljava/util/UUID;Ljava/lang/Boolean;Ljava/lang/Long;Ljava/lang/Double;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V public fun (Ljava/lang/String;Lio/sentry/protocol/User;Ljava/lang/String;Ljava/lang/String;)V diff --git a/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java b/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java new file mode 100644 index 00000000000..fb22512adf0 --- /dev/null +++ b/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java @@ -0,0 +1,134 @@ +package io.sentry; + +import io.sentry.util.Objects; +import java.net.InetAddress; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.FutureTask; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +/** + * An {@link EventProcessor} that sets the local ip address to an {@link + * SentryEvent#getServerName()} property. + * + *

Note - this event processor is not used by SDK by default and must be configured manually on + * {@link SentryOptions} if there is intention to use it. + */ +public final class ServerNameResolvingEventProcessor implements EventProcessor { + + /** + * Duration of the hostname caching. + * + * @see HostnameCache + */ + private static final long HOSTNAME_CACHE_DURATION = TimeUnit.HOURS.toMillis(5); + + /** The global hostname cache to speed up localhost hostname resolution. */ + private final HostnameCache hostnameCache; + + public ServerNameResolvingEventProcessor() { + this(new HostnameCache(HOSTNAME_CACHE_DURATION)); + } + + ServerNameResolvingEventProcessor(final @NotNull HostnameCache hostnameCache) { + this.hostnameCache = hostnameCache; + } + + @Override + public @NotNull SentryEvent process( + final @NotNull SentryEvent event, final @Nullable Object hint) { + if (event.getServerName() == null) { + event.setServerName(hostnameCache.getHostname()); + } + return event; + } + + /** + * Time sensitive cache in charge of keeping track of the hostname. The {@code + * InetAddress.getLocalHost().getCanonicalHostName()} call can be quite expensive and could be + * called for the creation of each {@link SentryEvent}. This system will prevent unnecessary costs + * by keeping track of the hostname for a period defined during the construction. For performance + * purposes, the operation of retrieving the hostname will automatically fail after a period of + * time defined by {@link #GET_HOSTNAME_TIMEOUT} without result. + */ + static final class HostnameCache { + /** Time before the get hostname operation times out (in ms). */ + private static final long GET_HOSTNAME_TIMEOUT = TimeUnit.SECONDS.toMillis(1); + /** Time for which the cache is kept. */ + private final long cacheDuration; + /** Current value for hostname (might change over time). */ + @Nullable private volatile String hostname; + /** Time at which the cache should expire. */ + private volatile long expirationTimestamp; + /** Whether a cache update thread is currently running or not. */ + private final @NotNull AtomicBoolean updateRunning = new AtomicBoolean(false); + + private final @NotNull Callable getLocalhost; + + private HostnameCache(long cacheDuration) { + this(cacheDuration, InetAddress::getLocalHost); + } + + /** + * Sets up a cache for the hostname. + * + * @param cacheDuration cache duration in milliseconds. + * @param getLocalhost a callback to obtain the localhost address - this is mostly here because + * of testability + */ + HostnameCache(long cacheDuration, final @NotNull Callable getLocalhost) { + this.cacheDuration = cacheDuration; + this.getLocalhost = Objects.requireNonNull(getLocalhost, "getLocalhost is required"); + } + + /** + * Gets the hostname of the current machine. + * + *

Gets the value from the cache if possible otherwise calls {@link #updateCache()}. + * + * @return the hostname of the current machine. + */ + private @Nullable String getHostname() { + if (expirationTimestamp < System.currentTimeMillis() + && updateRunning.compareAndSet(false, true)) { + updateCache(); + } + + return hostname; + } + + /** Force an update of the cache to get the current value of the hostname. */ + private void updateCache() { + final Callable hostRetriever = + () -> { + try { + hostname = getLocalhost.call().getCanonicalHostName(); + expirationTimestamp = System.currentTimeMillis() + cacheDuration; + } finally { + updateRunning.set(false); + } + + return null; + }; + + try { + final FutureTask futureTask = new FutureTask<>(hostRetriever); + new Thread(futureTask).start(); + futureTask.get(GET_HOSTNAME_TIMEOUT, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + handleCacheUpdateFailure(); + } catch (ExecutionException | TimeoutException | RuntimeException e) { + handleCacheUpdateFailure(); + } + } + + private void handleCacheUpdateFailure() { + expirationTimestamp = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1); + } + } +} diff --git a/sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt b/sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt new file mode 100644 index 00000000000..ff8a362b1fc --- /dev/null +++ b/sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt @@ -0,0 +1,86 @@ +package io.sentry + +import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.reset +import com.nhaarman.mockitokotlin2.times +import com.nhaarman.mockitokotlin2.verify +import com.nhaarman.mockitokotlin2.whenever +import io.sentry.ServerNameResolvingEventProcessor.HostnameCache +import java.net.InetAddress +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull +import org.awaitility.kotlin.await + +class ServerNameResolvingEventProcessorTest { + class Fixture { + val getLocalhost = mock() + + fun getSut(host: String, delay: Long? = null, cacheDuration: Long = 10): ServerNameResolvingEventProcessor { + whenever(getLocalhost.canonicalHostName).thenAnswer { + if (delay != null) { + Thread.sleep(delay) + } + host + } + val hostnameCache = HostnameCache(cacheDuration) { getLocalhost } + return ServerNameResolvingEventProcessor(hostnameCache) + } + } + + val fixture = Fixture() + + @Test + fun `sets servername retrieved from the local address`() { + val processor = fixture.getSut(host = "aHost") + val event = SentryEvent() + processor.process(event, null) + assertEquals("aHost", event.serverName) + } + + @Test + fun `sets servername to null if retrieving takes longer time`() { + val processor = fixture.getSut(host = "aHost", delay = 2000) + val event = SentryEvent() + processor.process(event, null) + assertNull(event.serverName) + } + + @Test + fun `uses cache to retrieve servername for subsequent events`() { + val processor = fixture.getSut(host = "aHost", cacheDuration = 1000) + val firstEvent = SentryEvent() + processor.process(firstEvent, null) + assertEquals("aHost", firstEvent.serverName) + val secondEvent = SentryEvent() + processor.process(secondEvent, null) + assertEquals("aHost", secondEvent.serverName) + verify(fixture.getLocalhost, times(1)).canonicalHostName + } + + @Test + fun `when cache expires, retrieves new host name from the local address`() { + val processor = fixture.getSut(host = "aHost") + val firstEvent = SentryEvent() + processor.process(firstEvent, null) + assertEquals("aHost", firstEvent.serverName) + + reset(fixture.getLocalhost) + whenever(fixture.getLocalhost.canonicalHostName).thenReturn("newHost") + + await.untilAsserted { + val secondEvent = SentryEvent() + processor.process(secondEvent, null) + assertEquals("newHost", secondEvent.serverName) + } + } + + @Test + fun `does not set serverName on events that already have server names`() { + val processor = fixture.getSut(host = "aHost") + val event = SentryEvent() + event.serverName = "eventHost" + processor.process(event, null) + assertEquals("eventHost", event.serverName) + } +} From 2425c247e84ae6e7dd0b90ce207d9e7957321c87 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Mon, 4 Jan 2021 21:23:36 +0100 Subject: [PATCH 2/5] Do not use method reference. --- .../main/java/io/sentry/ServerNameResolvingEventProcessor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java b/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java index fb22512adf0..67bd3d7905b 100644 --- a/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java +++ b/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java @@ -70,7 +70,7 @@ static final class HostnameCache { private final @NotNull Callable getLocalhost; private HostnameCache(long cacheDuration) { - this(cacheDuration, InetAddress::getLocalHost); + this(cacheDuration, () -> InetAddress.getLocalHost()); } /** From dbb46030604c5b30a858c13ce5a3e9d6b41799cd Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Mon, 4 Jan 2021 21:44:17 +0100 Subject: [PATCH 3/5] Use executor service and daemon thread instead of starting a thread manually. --- .../ServerNameResolvingEventProcessor.java | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java b/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java index 67bd3d7905b..68fdf5e6d5f 100644 --- a/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java +++ b/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java @@ -4,7 +4,10 @@ import java.net.InetAddress; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; -import java.util.concurrent.FutureTask; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; @@ -69,6 +72,9 @@ static final class HostnameCache { private final @NotNull Callable getLocalhost; + private final @NotNull ExecutorService executorService = Executors.newSingleThreadExecutor(new HostnameCacheThreadFactory()); + + private HostnameCache(long cacheDuration) { this(cacheDuration, () -> InetAddress.getLocalHost()); } @@ -116,8 +122,7 @@ private void updateCache() { }; try { - final FutureTask futureTask = new FutureTask<>(hostRetriever); - new Thread(futureTask).start(); + final Future futureTask = executorService.submit(hostRetriever); futureTask.get(GET_HOSTNAME_TIMEOUT, TimeUnit.MILLISECONDS); } catch (InterruptedException e) { Thread.currentThread().interrupt(); @@ -131,4 +136,15 @@ private void handleCacheUpdateFailure() { expirationTimestamp = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1); } } + + private static final class HostnameCacheThreadFactory implements ThreadFactory { + private int cnt; + + @Override + public @NotNull Thread newThread(final @NotNull Runnable r) { + final Thread ret = new Thread(r, "SentryHostnameCache-" + cnt++); + ret.setDaemon(true); + return ret; + } + } } From 028e5a97c4325ddbb3a6a84cb967ff3779b332a3 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Tue, 5 Jan 2021 13:27:17 +0100 Subject: [PATCH 4/5] Move setting servername to MainEventProcessor. --- .../spring/boot/SentryAutoConfiguration.java | 6 - .../boot/SentryAutoConfigurationTest.kt | 9 -- sentry/api/sentry.api | 7 +- .../main/java/io/sentry/HostnameCache.java | 121 ++++++++++++++ .../java/io/sentry/MainEventProcessor.java | 14 +- .../main/java/io/sentry/SentryOptions.java | 21 +++ .../ServerNameResolvingEventProcessor.java | 150 ------------------ .../java/io/sentry/MainEventProcessorTest.kt | 83 +++++++++- .../ServerNameResolvingEventProcessorTest.kt | 86 ---------- 9 files changed, 237 insertions(+), 260 deletions(-) create mode 100644 sentry/src/main/java/io/sentry/HostnameCache.java delete mode 100644 sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java delete mode 100644 sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt diff --git a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java index 9573427b8b9..d33fc58f4d6 100644 --- a/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java +++ b/sentry-spring-boot-starter/src/main/java/io/sentry/spring/boot/SentryAutoConfiguration.java @@ -7,7 +7,6 @@ import io.sentry.Integration; import io.sentry.Sentry; import io.sentry.SentryOptions; -import io.sentry.ServerNameResolvingEventProcessor; import io.sentry.protocol.SdkVersion; import io.sentry.spring.SentryExceptionResolver; import io.sentry.spring.SentryRequestResolver; @@ -93,11 +92,6 @@ static class HubConfiguration { return new InAppIncludesResolver(); } - @Bean - public @NotNull ServerNameResolvingEventProcessor serverNameResolvingEventProcessor() { - return new ServerNameResolvingEventProcessor(); - } - @Bean public @NotNull IHub sentryHub( final @NotNull List> optionsConfigurations, diff --git a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt index 8989009b3de..61febd0a9ee 100644 --- a/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt +++ b/sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentryAutoConfigurationTest.kt @@ -13,7 +13,6 @@ import io.sentry.Sentry import io.sentry.SentryEvent import io.sentry.SentryLevel import io.sentry.SentryOptions -import io.sentry.ServerNameResolvingEventProcessor import io.sentry.protocol.User import io.sentry.spring.HttpServletRequestSentryUserProvider import io.sentry.spring.SentryUserProvider @@ -453,14 +452,6 @@ class SentryAutoConfigurationTest { } } - @Test - fun `autoconfigures ServerNameResolvingEventProcessor`() { - contextRunner.withPropertyValues("sentry.dsn=http://key@localhost/proj") - .run { - assertThat(it).hasSingleBean(ServerNameResolvingEventProcessor::class.java) - } - } - @Configuration(proxyBeanMethods = false) open class CustomOptionsConfigurationConfiguration { diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 048b87b4feb..5b14996102c 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -695,6 +695,7 @@ public class io/sentry/SentryOptions { public fun getTracesSampler ()Lio/sentry/SentryOptions$TracesSamplerCallback; public fun getTransport ()Lio/sentry/transport/ITransport; public fun getTransportGate ()Lio/sentry/transport/ITransportGate; + public fun isAttachServerName ()Z public fun isAttachStacktrace ()Z public fun isAttachThreads ()Z public fun isDebug ()Z @@ -704,6 +705,7 @@ public class io/sentry/SentryOptions { public fun isEnableSessionTracking ()Z public fun isEnableUncaughtExceptionHandler ()Z public fun isSendDefaultPii ()Z + public fun setAttachServerName (Z)V public fun setAttachStacktrace (Z)V public fun setAttachThreads (Z)V public fun setBeforeBreadcrumb (Lio/sentry/SentryOptions$BeforeBreadcrumbCallback;)V @@ -806,11 +808,6 @@ public final class io/sentry/SentryTransaction : io/sentry/SentryBaseEvent, io/s public fun toSentryTrace ()Lio/sentry/SentryTraceHeader; } -public final class io/sentry/ServerNameResolvingEventProcessor : io/sentry/EventProcessor { - public fun ()V - public fun process (Lio/sentry/SentryEvent;Ljava/lang/Object;)Lio/sentry/SentryEvent; -} - public final class io/sentry/Session { public fun (Lio/sentry/Session$State;Ljava/util/Date;Ljava/util/Date;ILjava/lang/String;Ljava/util/UUID;Ljava/lang/Boolean;Ljava/lang/Long;Ljava/lang/Double;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V public fun (Ljava/lang/String;Lio/sentry/protocol/User;Ljava/lang/String;Ljava/lang/String;)V diff --git a/sentry/src/main/java/io/sentry/HostnameCache.java b/sentry/src/main/java/io/sentry/HostnameCache.java new file mode 100644 index 00000000000..11743105dca --- /dev/null +++ b/sentry/src/main/java/io/sentry/HostnameCache.java @@ -0,0 +1,121 @@ +package io.sentry; + +import io.sentry.util.Objects; +import java.net.InetAddress; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +/** + * Time sensitive cache in charge of keeping track of the hostname. The {@code + * InetAddress.getLocalHost().getCanonicalHostName()} call can be quite expensive and could be + * called for the creation of each {@link SentryEvent}. This system will prevent unnecessary costs + * by keeping track of the hostname for a period defined during the construction. For performance + * purposes, the operation of retrieving the hostname will automatically fail after a period of time + * defined by {@link #GET_HOSTNAME_TIMEOUT} without result. + */ +final class HostnameCache { + private static final long HOSTNAME_CACHE_DURATION = TimeUnit.HOURS.toMillis(5); + + /** Time before the get hostname operation times out (in ms). */ + private static final long GET_HOSTNAME_TIMEOUT = TimeUnit.SECONDS.toMillis(1); + /** Time for which the cache is kept. */ + private final long cacheDuration; + /** Current value for hostname (might change over time). */ + @Nullable private volatile String hostname; + /** Time at which the cache should expire. */ + private volatile long expirationTimestamp; + /** Whether a cache update thread is currently running or not. */ + private final @NotNull AtomicBoolean updateRunning = new AtomicBoolean(false); + + private final @NotNull Callable getLocalhost; + + private final @NotNull ExecutorService executorService = + Executors.newSingleThreadExecutor(new HostnameCacheThreadFactory()); + + public HostnameCache() { + this(HOSTNAME_CACHE_DURATION); + } + + HostnameCache(long cacheDuration) { + this(cacheDuration, () -> InetAddress.getLocalHost()); + } + + /** + * Sets up a cache for the hostname. + * + * @param cacheDuration cache duration in milliseconds. + * @param getLocalhost a callback to obtain the localhost address - this is mostly here because of + * testability + */ + HostnameCache(long cacheDuration, final @NotNull Callable getLocalhost) { + this.cacheDuration = cacheDuration; + this.getLocalhost = Objects.requireNonNull(getLocalhost, "getLocalhost is required"); + updateCache(); + } + + /** + * Gets the hostname of the current machine. + * + *

Gets the value from the cache if possible otherwise calls {@link #updateCache()}. + * + * @return the hostname of the current machine. + */ + @Nullable + String getHostname() { + if (expirationTimestamp < System.currentTimeMillis() + && updateRunning.compareAndSet(false, true)) { + updateCache(); + } + + return hostname; + } + + /** Force an update of the cache to get the current value of the hostname. */ + private void updateCache() { + final Callable hostRetriever = + () -> { + try { + hostname = getLocalhost.call().getCanonicalHostName(); + expirationTimestamp = System.currentTimeMillis() + cacheDuration; + } finally { + updateRunning.set(false); + } + + return null; + }; + + try { + final Future futureTask = executorService.submit(hostRetriever); + futureTask.get(GET_HOSTNAME_TIMEOUT, TimeUnit.MILLISECONDS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + handleCacheUpdateFailure(); + } catch (ExecutionException | TimeoutException | RuntimeException e) { + handleCacheUpdateFailure(); + } + } + + private void handleCacheUpdateFailure() { + expirationTimestamp = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1); + } + + private static final class HostnameCacheThreadFactory implements ThreadFactory { + private int cnt; + + @Override + public @NotNull Thread newThread(final @NotNull Runnable r) { + final Thread ret = new Thread(r, "SentryHostnameCache-" + cnt++); + ret.setDaemon(true); + return ret; + } + } +} diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index 6440a88ba88..8ef40b6b38f 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -29,9 +29,16 @@ public final class MainEventProcessor implements EventProcessor { private final @NotNull SentryOptions options; private final @NotNull SentryThreadFactory sentryThreadFactory; private final @NotNull SentryExceptionFactory sentryExceptionFactory; + private final @NotNull HostnameCache hostnameCache; MainEventProcessor(final @NotNull SentryOptions options) { + this(options, new HostnameCache()); + } + + MainEventProcessor( + final @NotNull SentryOptions options, final @NotNull HostnameCache hostnameCache) { this.options = Objects.requireNonNull(options, "The SentryOptions is required."); + this.hostnameCache = Objects.requireNonNull(hostnameCache, "The HostnameCache is required"); final SentryStackTraceFactory sentryStackTraceFactory = new SentryStackTraceFactory( @@ -44,12 +51,14 @@ public final class MainEventProcessor implements EventProcessor { MainEventProcessor( final @NotNull SentryOptions options, final @NotNull SentryThreadFactory sentryThreadFactory, - final @NotNull SentryExceptionFactory sentryExceptionFactory) { + final @NotNull SentryExceptionFactory sentryExceptionFactory, + final @NotNull HostnameCache hostnameCache) { this.options = Objects.requireNonNull(options, "The SentryOptions is required."); this.sentryThreadFactory = Objects.requireNonNull(sentryThreadFactory, "The SentryThreadFactory is required."); this.sentryExceptionFactory = Objects.requireNonNull(sentryExceptionFactory, "The SentryExceptionFactory is required."); + this.hostnameCache = Objects.requireNonNull(hostnameCache, "The HostnameCache is required"); } @Override @@ -139,5 +148,8 @@ private void processNonCachedEvent(final @NotNull SentryEvent event) { event.getUser().setIpAddress(DEFAULT_IP_ADDRESS); } } + if (options.isAttachServerName() && event.getServerName() == null) { + event.setServerName(hostnameCache.getHostname()); + } } } diff --git a/sentry/src/main/java/io/sentry/SentryOptions.java b/sentry/src/main/java/io/sentry/SentryOptions.java index d403e27be04..3db7283d83b 100644 --- a/sentry/src/main/java/io/sentry/SentryOptions.java +++ b/sentry/src/main/java/io/sentry/SentryOptions.java @@ -203,6 +203,9 @@ public class SentryOptions { /** The server name used in the Sentry messages. */ private String serverName; + /** Automatically resolve server name. */ + private boolean attachServerName = true; + /* When enabled, Sentry installs UncaughtExceptionHandlerIntegration. */ @@ -852,6 +855,24 @@ public void setServerName(@Nullable String serverName) { this.serverName = serverName; } + /** + * Returns if SDK automatically resolves and attaches server name to events. + * + * @return true if enabled false if otherwise + */ + public boolean isAttachServerName() { + return attachServerName; + } + + /** + * Sets if SDK should automatically resolve and attache server name to events. + * + * @param attachServerName true if enabled false if otherwise + */ + public void setAttachServerName(boolean attachServerName) { + this.attachServerName = attachServerName; + } + /** * Returns the session tracking interval in millis * diff --git a/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java b/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java deleted file mode 100644 index 68fdf5e6d5f..00000000000 --- a/sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java +++ /dev/null @@ -1,150 +0,0 @@ -package io.sentry; - -import io.sentry.util.Objects; -import java.net.InetAddress; -import java.util.concurrent.Callable; -import java.util.concurrent.ExecutionException; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.Future; -import java.util.concurrent.ThreadFactory; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; -import java.util.concurrent.atomic.AtomicBoolean; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; - -/** - * An {@link EventProcessor} that sets the local ip address to an {@link - * SentryEvent#getServerName()} property. - * - *

Note - this event processor is not used by SDK by default and must be configured manually on - * {@link SentryOptions} if there is intention to use it. - */ -public final class ServerNameResolvingEventProcessor implements EventProcessor { - - /** - * Duration of the hostname caching. - * - * @see HostnameCache - */ - private static final long HOSTNAME_CACHE_DURATION = TimeUnit.HOURS.toMillis(5); - - /** The global hostname cache to speed up localhost hostname resolution. */ - private final HostnameCache hostnameCache; - - public ServerNameResolvingEventProcessor() { - this(new HostnameCache(HOSTNAME_CACHE_DURATION)); - } - - ServerNameResolvingEventProcessor(final @NotNull HostnameCache hostnameCache) { - this.hostnameCache = hostnameCache; - } - - @Override - public @NotNull SentryEvent process( - final @NotNull SentryEvent event, final @Nullable Object hint) { - if (event.getServerName() == null) { - event.setServerName(hostnameCache.getHostname()); - } - return event; - } - - /** - * Time sensitive cache in charge of keeping track of the hostname. The {@code - * InetAddress.getLocalHost().getCanonicalHostName()} call can be quite expensive and could be - * called for the creation of each {@link SentryEvent}. This system will prevent unnecessary costs - * by keeping track of the hostname for a period defined during the construction. For performance - * purposes, the operation of retrieving the hostname will automatically fail after a period of - * time defined by {@link #GET_HOSTNAME_TIMEOUT} without result. - */ - static final class HostnameCache { - /** Time before the get hostname operation times out (in ms). */ - private static final long GET_HOSTNAME_TIMEOUT = TimeUnit.SECONDS.toMillis(1); - /** Time for which the cache is kept. */ - private final long cacheDuration; - /** Current value for hostname (might change over time). */ - @Nullable private volatile String hostname; - /** Time at which the cache should expire. */ - private volatile long expirationTimestamp; - /** Whether a cache update thread is currently running or not. */ - private final @NotNull AtomicBoolean updateRunning = new AtomicBoolean(false); - - private final @NotNull Callable getLocalhost; - - private final @NotNull ExecutorService executorService = Executors.newSingleThreadExecutor(new HostnameCacheThreadFactory()); - - - private HostnameCache(long cacheDuration) { - this(cacheDuration, () -> InetAddress.getLocalHost()); - } - - /** - * Sets up a cache for the hostname. - * - * @param cacheDuration cache duration in milliseconds. - * @param getLocalhost a callback to obtain the localhost address - this is mostly here because - * of testability - */ - HostnameCache(long cacheDuration, final @NotNull Callable getLocalhost) { - this.cacheDuration = cacheDuration; - this.getLocalhost = Objects.requireNonNull(getLocalhost, "getLocalhost is required"); - } - - /** - * Gets the hostname of the current machine. - * - *

Gets the value from the cache if possible otherwise calls {@link #updateCache()}. - * - * @return the hostname of the current machine. - */ - private @Nullable String getHostname() { - if (expirationTimestamp < System.currentTimeMillis() - && updateRunning.compareAndSet(false, true)) { - updateCache(); - } - - return hostname; - } - - /** Force an update of the cache to get the current value of the hostname. */ - private void updateCache() { - final Callable hostRetriever = - () -> { - try { - hostname = getLocalhost.call().getCanonicalHostName(); - expirationTimestamp = System.currentTimeMillis() + cacheDuration; - } finally { - updateRunning.set(false); - } - - return null; - }; - - try { - final Future futureTask = executorService.submit(hostRetriever); - futureTask.get(GET_HOSTNAME_TIMEOUT, TimeUnit.MILLISECONDS); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - handleCacheUpdateFailure(); - } catch (ExecutionException | TimeoutException | RuntimeException e) { - handleCacheUpdateFailure(); - } - } - - private void handleCacheUpdateFailure() { - expirationTimestamp = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1); - } - } - - private static final class HostnameCacheThreadFactory implements ThreadFactory { - private int cnt; - - @Override - public @NotNull Thread newThread(final @NotNull Runnable r) { - final Thread ret = new Thread(r, "SentryHostnameCache-" + cnt++); - ret.setDaemon(true); - return ret; - } - } -} diff --git a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt index ca898018bc9..d7ec234a734 100644 --- a/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt +++ b/sentry/src/test/java/io/sentry/MainEventProcessorTest.kt @@ -1,11 +1,16 @@ package io.sentry import com.nhaarman.mockitokotlin2.mock +import com.nhaarman.mockitokotlin2.reset +import com.nhaarman.mockitokotlin2.times +import com.nhaarman.mockitokotlin2.verify +import com.nhaarman.mockitokotlin2.whenever import io.sentry.hints.ApplyScopeData import io.sentry.hints.Cached import io.sentry.protocol.SdkVersion import io.sentry.protocol.User import java.lang.RuntimeException +import java.net.InetAddress import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -13,6 +18,7 @@ import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue +import org.awaitility.kotlin.await class MainEventProcessorTest { class Fixture { @@ -20,21 +26,30 @@ class MainEventProcessorTest { dsn = dsnString release = "release" dist = "dist" - serverName = "server" sdkVersion = SdkVersion().apply { name = "test" version = "1.2.3" } } - fun getSut(attachThreads: Boolean = true, attachStackTrace: Boolean = true, environment: String? = "environment", tags: Map = emptyMap(), sendDefaultPii: Boolean? = null): MainEventProcessor { + val getLocalhost = mock() + + fun getSut(attachThreads: Boolean = true, attachStackTrace: Boolean = true, environment: String? = "environment", tags: Map = emptyMap(), sendDefaultPii: Boolean? = null, serverName: String? = "server", host: String? = null, resolveHostDelay: Long? = null, hostnameCacheDuration: Long = 10): MainEventProcessor { sentryOptions.isAttachThreads = attachThreads sentryOptions.isAttachStacktrace = attachStackTrace sentryOptions.environment = environment + sentryOptions.serverName = serverName if (sendDefaultPii != null) { sentryOptions.isSendDefaultPii = sendDefaultPii } tags.forEach { sentryOptions.setTag(it.key, it.value) } - return MainEventProcessor(sentryOptions) + whenever(getLocalhost.canonicalHostName).thenAnswer { + if (resolveHostDelay != null) { + Thread.sleep(resolveHostDelay) + } + host + } + val hostnameCache = HostnameCache(hostnameCacheDuration) { getLocalhost } + return MainEventProcessor(sentryOptions, hostnameCache) } } @@ -261,6 +276,68 @@ class MainEventProcessorTest { assertEquals("event-tag-value", event.tags["tag2"]) } + @Test + fun `sets servername retrieved from the local address`() { + val processor = fixture.getSut(serverName = null, host = "aHost") + val event = SentryEvent() + processor.process(event, null) + assertEquals("aHost", event.serverName) + } + + @Test + fun `sets servername to null if retrieving takes longer time`() { + val processor = fixture.getSut(serverName = null, host = "aHost", resolveHostDelay = 2000) + val event = SentryEvent() + processor.process(event, null) + assertNull(event.serverName) + } + + @Test + fun `uses cache to retrieve servername for subsequent events`() { + val processor = fixture.getSut(serverName = null, host = "aHost", hostnameCacheDuration = 1000) + val firstEvent = SentryEvent() + processor.process(firstEvent, null) + assertEquals("aHost", firstEvent.serverName) + val secondEvent = SentryEvent() + processor.process(secondEvent, null) + assertEquals("aHost", secondEvent.serverName) + verify(fixture.getLocalhost, times(1)).canonicalHostName + } + + @Test + fun `when cache expires, retrieves new host name from the local address`() { + val processor = fixture.getSut(serverName = null, host = "aHost") + val firstEvent = SentryEvent() + processor.process(firstEvent, null) + assertEquals("aHost", firstEvent.serverName) + + reset(fixture.getLocalhost) + whenever(fixture.getLocalhost.canonicalHostName).thenReturn("newHost") + + await.untilAsserted { + val secondEvent = SentryEvent() + processor.process(secondEvent, null) + assertEquals("newHost", secondEvent.serverName) + } + } + + @Test + fun `does not set serverName on events that already have server names`() { + val processor = fixture.getSut(serverName = null, host = "aHost") + val event = SentryEvent() + event.serverName = "eventHost" + processor.process(event, null) + assertEquals("eventHost", event.serverName) + } + + @Test + fun `does not set serverName on events if serverName is set on SentryOptions`() { + val processor = fixture.getSut(serverName = "optionsHost", host = "aHost") + val event = SentryEvent() + processor.process(event, null) + assertEquals("optionsHost", event.serverName) + } + private fun generateCrashedEvent(crashedThread: Thread = Thread.currentThread()) = SentryEvent().apply { val mockThrowable = mock() val actualThrowable = UncaughtExceptionHandlerIntegration.getUnhandledThrowable(crashedThread, mockThrowable) diff --git a/sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt b/sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt deleted file mode 100644 index ff8a362b1fc..00000000000 --- a/sentry/src/test/java/io/sentry/ServerNameResolvingEventProcessorTest.kt +++ /dev/null @@ -1,86 +0,0 @@ -package io.sentry - -import com.nhaarman.mockitokotlin2.mock -import com.nhaarman.mockitokotlin2.reset -import com.nhaarman.mockitokotlin2.times -import com.nhaarman.mockitokotlin2.verify -import com.nhaarman.mockitokotlin2.whenever -import io.sentry.ServerNameResolvingEventProcessor.HostnameCache -import java.net.InetAddress -import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertNull -import org.awaitility.kotlin.await - -class ServerNameResolvingEventProcessorTest { - class Fixture { - val getLocalhost = mock() - - fun getSut(host: String, delay: Long? = null, cacheDuration: Long = 10): ServerNameResolvingEventProcessor { - whenever(getLocalhost.canonicalHostName).thenAnswer { - if (delay != null) { - Thread.sleep(delay) - } - host - } - val hostnameCache = HostnameCache(cacheDuration) { getLocalhost } - return ServerNameResolvingEventProcessor(hostnameCache) - } - } - - val fixture = Fixture() - - @Test - fun `sets servername retrieved from the local address`() { - val processor = fixture.getSut(host = "aHost") - val event = SentryEvent() - processor.process(event, null) - assertEquals("aHost", event.serverName) - } - - @Test - fun `sets servername to null if retrieving takes longer time`() { - val processor = fixture.getSut(host = "aHost", delay = 2000) - val event = SentryEvent() - processor.process(event, null) - assertNull(event.serverName) - } - - @Test - fun `uses cache to retrieve servername for subsequent events`() { - val processor = fixture.getSut(host = "aHost", cacheDuration = 1000) - val firstEvent = SentryEvent() - processor.process(firstEvent, null) - assertEquals("aHost", firstEvent.serverName) - val secondEvent = SentryEvent() - processor.process(secondEvent, null) - assertEquals("aHost", secondEvent.serverName) - verify(fixture.getLocalhost, times(1)).canonicalHostName - } - - @Test - fun `when cache expires, retrieves new host name from the local address`() { - val processor = fixture.getSut(host = "aHost") - val firstEvent = SentryEvent() - processor.process(firstEvent, null) - assertEquals("aHost", firstEvent.serverName) - - reset(fixture.getLocalhost) - whenever(fixture.getLocalhost.canonicalHostName).thenReturn("newHost") - - await.untilAsserted { - val secondEvent = SentryEvent() - processor.process(secondEvent, null) - assertEquals("newHost", secondEvent.serverName) - } - } - - @Test - fun `does not set serverName on events that already have server names`() { - val processor = fixture.getSut(host = "aHost") - val event = SentryEvent() - event.serverName = "eventHost" - processor.process(event, null) - assertEquals("eventHost", event.serverName) - } -} From 59c4e8d79f5af8ca7acdf1d9f3dceb16a12cc2c6 Mon Sep 17 00:00:00 2001 From: Maciej Walkowiak Date: Tue, 5 Jan 2021 14:16:52 +0100 Subject: [PATCH 5/5] Do not resolve hostname in Android integration. --- .../io/sentry/android/core/SentryAndroidOptions.java | 1 + sentry/src/main/java/io/sentry/MainEventProcessor.java | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java index bbf7a26a328..931bc1928eb 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java @@ -39,6 +39,7 @@ public final class SentryAndroidOptions extends SentryOptions { public SentryAndroidOptions() { setSentryClientName(BuildConfig.SENTRY_ANDROID_SDK_NAME + "/" + BuildConfig.VERSION_NAME); setSdkVersion(createSdkVersion()); + setAttachServerName(false); } private @NotNull SdkVersion createSdkVersion() { diff --git a/sentry/src/main/java/io/sentry/MainEventProcessor.java b/sentry/src/main/java/io/sentry/MainEventProcessor.java index 8ef40b6b38f..6b59d7e20af 100644 --- a/sentry/src/main/java/io/sentry/MainEventProcessor.java +++ b/sentry/src/main/java/io/sentry/MainEventProcessor.java @@ -29,16 +29,16 @@ public final class MainEventProcessor implements EventProcessor { private final @NotNull SentryOptions options; private final @NotNull SentryThreadFactory sentryThreadFactory; private final @NotNull SentryExceptionFactory sentryExceptionFactory; - private final @NotNull HostnameCache hostnameCache; + private final @Nullable HostnameCache hostnameCache; MainEventProcessor(final @NotNull SentryOptions options) { - this(options, new HostnameCache()); + this(options, options.isAttachServerName() ? new HostnameCache() : null); } MainEventProcessor( - final @NotNull SentryOptions options, final @NotNull HostnameCache hostnameCache) { + final @NotNull SentryOptions options, final @Nullable HostnameCache hostnameCache) { this.options = Objects.requireNonNull(options, "The SentryOptions is required."); - this.hostnameCache = Objects.requireNonNull(hostnameCache, "The HostnameCache is required"); + this.hostnameCache = hostnameCache; final SentryStackTraceFactory sentryStackTraceFactory = new SentryStackTraceFactory( @@ -148,7 +148,7 @@ private void processNonCachedEvent(final @NotNull SentryEvent event) { event.getUser().setIpAddress(DEFAULT_IP_ADDRESS); } } - if (options.isAttachServerName() && event.getServerName() == null) { + if (options.isAttachServerName() && hostnameCache != null && event.getServerName() == null) { event.setServerName(hostnameCache.getHostname()); } }