From 5c2cb846996d68a2425ee4a66d61ba097a2034e5 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 6 Aug 2024 09:31:43 +1000 Subject: [PATCH 01/26] Implement TestFixture for log collectors GitOrigin-RevId: 453df97428102c5f1959afd94c83d52623e8387c --- .../kotlin/misk/logging/LogCollectorModule.kt | 5 ++++- .../api/wisp-logging-testing.api | 4 ++-- wisp/wisp-logging-testing/build.gradle.kts | 15 ++++++++------- .../src/main/kotlin/wisp/logging/LogCollector.kt | 3 ++- .../kotlin/wisp/logging/WispQueuedLogCollector.kt | 5 +++-- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/misk-testing/src/main/kotlin/misk/logging/LogCollectorModule.kt b/misk-testing/src/main/kotlin/misk/logging/LogCollectorModule.kt index b785dcc7ea9..0ec308007df 100644 --- a/misk-testing/src/main/kotlin/misk/logging/LogCollectorModule.kt +++ b/misk-testing/src/main/kotlin/misk/logging/LogCollectorModule.kt @@ -6,12 +6,15 @@ import misk.inject.KAbstractModule import wisp.logging.LogCollector import wisp.logging.WispQueuedLogCollector import com.google.inject.Provider +import misk.testing.TestFixture class LogCollectorModule : KAbstractModule() { override fun configure() { bind().to() bind().to() - bind().toProvider(Provider { WispQueuedLogCollector() }) + multibind().to() + bind().toProvider { WispQueuedLogCollector() } + multibind().to() install(ServiceModule().enhancedBy()) } } diff --git a/wisp/wisp-logging-testing/api/wisp-logging-testing.api b/wisp/wisp-logging-testing/api/wisp-logging-testing.api index 3ebde292d52..3c2e472ff78 100644 --- a/wisp/wisp-logging-testing/api/wisp-logging-testing.api +++ b/wisp/wisp-logging-testing/api/wisp-logging-testing.api @@ -3,7 +3,7 @@ public final class org/assertj/core/api/AssertExtensionsKt { public static final fun isEqualToAsJson (Lorg/assertj/core/api/AbstractCharSequenceAssert;Ljava/lang/CharSequence;)Lorg/assertj/core/api/AbstractCharSequenceAssert; } -public abstract interface class wisp/logging/LogCollector { +public abstract interface class wisp/logging/LogCollector : misk/testing/TestFixture { public abstract fun takeEvent (Lkotlin/reflect/KClass;Lch/qos/logback/classic/Level;Lkotlin/text/Regex;)Lch/qos/logback/classic/spi/ILoggingEvent; public abstract fun takeEvent (Lkotlin/reflect/KClass;Lch/qos/logback/classic/Level;Lkotlin/text/Regex;Z)Lch/qos/logback/classic/spi/ILoggingEvent; public abstract fun takeEvents (Lkotlin/reflect/KClass;Lch/qos/logback/classic/Level;Lkotlin/text/Regex;)Ljava/util/List; @@ -25,7 +25,7 @@ public final class wisp/logging/LogCollector$DefaultImpls { public static synthetic fun takeMessages$default (Lwisp/logging/LogCollector;Lkotlin/reflect/KClass;Lch/qos/logback/classic/Level;Lkotlin/text/Regex;ZILjava/lang/Object;)Ljava/util/List; } -public final class wisp/logging/WispQueuedLogCollector : wisp/logging/LogCollector { +public final class wisp/logging/WispQueuedLogCollector : misk/testing/FakeFixture, wisp/logging/LogCollector { public fun ()V public final fun shutDown ()V public final fun startUp ()V diff --git a/wisp/wisp-logging-testing/build.gradle.kts b/wisp/wisp-logging-testing/build.gradle.kts index 1945d7c3034..353252b7182 100644 --- a/wisp/wisp-logging-testing/build.gradle.kts +++ b/wisp/wisp-logging-testing/build.gradle.kts @@ -5,12 +5,13 @@ plugins { } dependencies { - api(libs.logbackClassic) - api(libs.assertj) - implementation(libs.logbackCore) - implementation(libs.slf4jApi) + api(project(":misk-testing-api")) + api(libs.logbackClassic) + api(libs.assertj) + implementation(libs.logbackCore) + implementation(libs.slf4jApi) - testImplementation(libs.junitApi) - testImplementation(libs.kotlinTest) - testImplementation(project(":wisp:wisp-logging")) + testImplementation(libs.junitApi) + testImplementation(libs.kotlinTest) + testImplementation(project(":wisp:wisp-logging")) } diff --git a/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/LogCollector.kt b/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/LogCollector.kt index e6cd94ec72c..079a1a1ba41 100644 --- a/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/LogCollector.kt +++ b/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/LogCollector.kt @@ -2,6 +2,7 @@ package wisp.logging import ch.qos.logback.classic.Level import ch.qos.logback.classic.spi.ILoggingEvent +import misk.testing.TestFixture import kotlin.reflect.KClass /** @@ -9,7 +10,7 @@ import kotlin.reflect.KClass * * Use the optional parameters of [takeMessages] to constrain which log messages are returned. */ -interface LogCollector { +interface LogCollector: TestFixture { /** * Removes all currently-collected log messages and returns those that match the requested * criteria. diff --git a/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/WispQueuedLogCollector.kt b/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/WispQueuedLogCollector.kt index 8e68f82db98..49d4163e2d1 100644 --- a/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/WispQueuedLogCollector.kt +++ b/wisp/wisp-logging-testing/src/main/kotlin/wisp/logging/WispQueuedLogCollector.kt @@ -4,14 +4,15 @@ import ch.qos.logback.classic.Level import ch.qos.logback.classic.Logger import ch.qos.logback.classic.spi.ILoggingEvent import ch.qos.logback.core.UnsynchronizedAppenderBase +import misk.testing.FakeFixture import org.slf4j.LoggerFactory import java.lang.Thread.sleep import java.util.concurrent.LinkedBlockingDeque import java.util.concurrent.TimeUnit import kotlin.reflect.KClass -class WispQueuedLogCollector : LogCollector { - private val queue = LinkedBlockingDeque() +class WispQueuedLogCollector : LogCollector, FakeFixture() { + private val queue by resettable { LinkedBlockingDeque() } private var wasStarted = false From 97e07356511b498ae2dfaaf762bfcae2d28af552 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 6 Aug 2024 09:44:45 +1000 Subject: [PATCH 02/26] Implement TestFixture for metrics GitOrigin-RevId: c51ec0fb5dcc67f9e1b6b2564678fd86d3f3c416 --- misk-metrics/build.gradle.kts | 1 + .../metrics/v2/CollectorRegistryModule.kt | 69 +++++++++++++++++++ .../misk/metrics/v2/FakeMetricsModule.kt | 2 +- misk-testing/api/misk-testing.api | 2 + misk-testing/build.gradle.kts | 1 + .../kotlin/misk/MiskTestingServiceModule.kt | 10 ++- misk/api/misk.api | 3 +- .../src/main/kotlin/misk/MiskServiceModule.kt | 5 +- 8 files changed, 88 insertions(+), 5 deletions(-) create mode 100644 misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/CollectorRegistryModule.kt diff --git a/misk-metrics/build.gradle.kts b/misk-metrics/build.gradle.kts index acbcbc5a1a4..d0d9a743077 100644 --- a/misk-metrics/build.gradle.kts +++ b/misk-metrics/build.gradle.kts @@ -17,6 +17,7 @@ dependencies { implementation(libs.kotlinStdLibJdk8) testFixturesApi(project(":misk-inject")) + testFixturesApi(project(":misk-testing-api")) testFixturesApi(libs.prometheusClient) testFixturesImplementation(libs.guava) testFixturesImplementation(libs.guice) diff --git a/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/CollectorRegistryModule.kt b/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/CollectorRegistryModule.kt new file mode 100644 index 00000000000..ce782d382a5 --- /dev/null +++ b/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/CollectorRegistryModule.kt @@ -0,0 +1,69 @@ +package misk.metrics.v2 + +import io.prometheus.client.Collector +import io.prometheus.client.CollectorRegistry +import io.prometheus.client.Predicate +import io.prometheus.client.SimpleCollector +import misk.inject.KAbstractModule +import misk.testing.TestFixture +import java.util.Enumeration + +internal class CollectorRegistryModule : KAbstractModule() { + + override fun configure() { + val registry = CollectorRegistryFixture(CollectorRegistry(true)) + bind().toInstance(registry) + multibind().toInstance(registry) + } +} + +internal class CollectorRegistryFixture(val registry: CollectorRegistry) : CollectorRegistry(true), TestFixture { + + private val registeredCollectors = mutableSetOf() + + override fun reset() { + registeredCollectors.forEach { + if (it is SimpleCollector<*>) { + it.clear() + } + } + } + + override fun register(collector: Collector) { + registeredCollectors.add(collector) + registry.register(collector) + } + + override fun unregister(collector: Collector) { + registeredCollectors.remove(collector) + registry.unregister(collector) + } + + override fun metricFamilySamples(): Enumeration { + return registry.metricFamilySamples() + } + + override fun filteredMetricFamilySamples(includedNames: Set?): Enumeration { + return registry.filteredMetricFamilySamples(includedNames) + } + + override fun filteredMetricFamilySamples(sampleNameFilter: Predicate?): Enumeration { + return registry.filteredMetricFamilySamples(sampleNameFilter) + } + + override fun getSampleValue(name: String): Double? { + return registry.getSampleValue(name) + } + + override fun getSampleValue( + name: String, + labelNames: Array?, + labelValues: Array? + ): Double? { + return registry.getSampleValue(name, labelNames, labelValues) + } + + override fun clear() { + registry.clear() + } +} diff --git a/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt b/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt index 10177fe037a..4b4fe08f4d5 100644 --- a/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt +++ b/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt @@ -5,7 +5,7 @@ import misk.inject.KAbstractModule class FakeMetricsModule : KAbstractModule() { override fun configure() { - bind().toInstance(CollectorRegistry(true)) + install(CollectorRegistryModule()) bind().to() } } diff --git a/misk-testing/api/misk-testing.api b/misk-testing/api/misk-testing.api index 5adb3275b36..25652b1b88e 100644 --- a/misk-testing/api/misk-testing.api +++ b/misk-testing/api/misk-testing.api @@ -1,5 +1,7 @@ public final class misk/MiskTestingServiceModule : misk/inject/KAbstractModule { public fun ()V + public fun (Z)V + public synthetic fun (ZILkotlin/jvm/internal/DefaultConstructorMarker;)V } public final class misk/cloud/fake/security/keys/FakeKeyManagementModule : misk/inject/KAbstractModule { diff --git a/misk-testing/build.gradle.kts b/misk-testing/build.gradle.kts index 94f4d4112f1..769c063cdeb 100644 --- a/misk-testing/build.gradle.kts +++ b/misk-testing/build.gradle.kts @@ -40,6 +40,7 @@ dependencies { implementation(project(":misk-config")) implementation(project(":misk-service")) implementation(project(":misk-testing-api")) + implementation(testFixtures(project(":misk-metrics"))) testImplementation(libs.kotlinTest) } diff --git a/misk-testing/src/main/kotlin/misk/MiskTestingServiceModule.kt b/misk-testing/src/main/kotlin/misk/MiskTestingServiceModule.kt index 1ea87b33ef4..0fe8d43ae40 100644 --- a/misk-testing/src/main/kotlin/misk/MiskTestingServiceModule.kt +++ b/misk-testing/src/main/kotlin/misk/MiskTestingServiceModule.kt @@ -3,6 +3,7 @@ package misk import misk.concurrent.FakeSleeperModule import misk.environment.FakeEnvVarModule import misk.inject.KAbstractModule +import misk.metrics.FakeMetricsModule import misk.random.FakeRandomModule import misk.resources.TestingResourceLoaderModule import misk.time.FakeClockModule @@ -16,7 +17,9 @@ import misk.tokens.FakeTokenGeneratorModule * set of fake bindings to replace real bindings that cannot exist in a unit testing environment * (e.g system env vars and filesystem dependencies). */ -class MiskTestingServiceModule : KAbstractModule() { +class MiskTestingServiceModule @JvmOverloads constructor( + private val installFakeMetrics: Boolean = false +): KAbstractModule() { override fun configure() { install(TestingResourceLoaderModule()) install(FakeEnvVarModule()) @@ -25,6 +28,9 @@ class MiskTestingServiceModule : KAbstractModule() { install(FakeTickerModule()) install(FakeRandomModule()) install(FakeTokenGeneratorModule()) - install(MiskCommonServiceModule()) + if (installFakeMetrics) { + install(FakeMetricsModule()) + } + install(MiskCommonServiceModule(installMetrics = !installFakeMetrics)) } } diff --git a/misk/api/misk.api b/misk/api/misk.api index 563bfaf3e2f..4655c15570b 100644 --- a/misk/api/misk.api +++ b/misk/api/misk.api @@ -309,7 +309,8 @@ public abstract class misk/MiskCommand : java/lang/Runnable { public final class misk/MiskCommonServiceModule : misk/inject/KAbstractModule { public fun ()V public fun (Lmisk/ServiceManagerConfig;)V - public synthetic fun (Lmisk/ServiceManagerConfig;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lmisk/ServiceManagerConfig;Z)V + public synthetic fun (Lmisk/ServiceManagerConfig;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V } public final class misk/MiskRealServiceModule : misk/inject/KAbstractModule { diff --git a/misk/src/main/kotlin/misk/MiskServiceModule.kt b/misk/src/main/kotlin/misk/MiskServiceModule.kt index 22ab964366d..eb2e8c20638 100644 --- a/misk/src/main/kotlin/misk/MiskServiceModule.kt +++ b/misk/src/main/kotlin/misk/MiskServiceModule.kt @@ -42,6 +42,7 @@ class MiskRealServiceModule @JvmOverloads constructor( */ class MiskCommonServiceModule @JvmOverloads constructor( private val serviceManagerConfig: ServiceManagerConfig = ServiceManagerConfig(), + private val installMetrics: Boolean = true ) : KAbstractModule() { override fun configure() { binder().disableCircularProxies() @@ -49,7 +50,9 @@ class MiskCommonServiceModule @JvmOverloads constructor( install(MdcModule()) install(ExecutorsModule()) install(ServiceManagerModule(serviceManagerConfig)) - install(PrometheusMetricsClientModule()) + if (installMetrics) { + install(PrometheusMetricsClientModule()) + } install(MoshiModule(useWireToRead = true, useWireToWrite = true)) install(JvmManagementFactoryModule()) // Initialize empty sets for our multibindings. From b8aafdba67de3e66dcbd9c658a2136dc66071b13 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 6 Aug 2024 12:11:20 +1000 Subject: [PATCH 03/26] Implement TestFixture for JdbcTestingModule GitOrigin-RevId: 611be8a7a60a1decf8ea787aca23dacf01c1224a --- misk-jdbc/api/misk-jdbc.api | 12 ++- misk-jdbc/build.gradle.kts | 1 + .../kotlin/misk/jdbc/JdbcTestFixture.kt | 80 +++++++++++++++++++ .../kotlin/misk/jdbc/JdbcTestingModule.kt | 12 ++- .../kotlin/misk/jdbc/TruncateTablesService.kt | 69 +--------------- 5 files changed, 106 insertions(+), 68 deletions(-) create mode 100644 misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt diff --git a/misk-jdbc/api/misk-jdbc.api b/misk-jdbc/api/misk-jdbc.api index 4472052b5d1..e2900ddf150 100644 --- a/misk-jdbc/api/misk-jdbc.api +++ b/misk-jdbc/api/misk-jdbc.api @@ -510,6 +510,15 @@ public final class misk/jdbc/JdbcModule : misk/inject/KAbstractModule { public final fun getReaderConfig ()Lmisk/jdbc/DataSourceConfig; } +public final class misk/jdbc/JdbcTestFixture : misk/testing/TestFixture { + public static final field Companion Lmisk/jdbc/JdbcTestFixture$Companion; + public fun (Lkotlin/reflect/KClass;Lmisk/jdbc/DataSourceService;Lcom/google/inject/Provider;)V + public fun reset ()V +} + +public final class misk/jdbc/JdbcTestFixture$Companion { +} + public final class misk/jdbc/JdbcTestingModule : misk/inject/KAbstractModule { public fun (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;Z)V public synthetic fun (Lkotlin/reflect/KClass;Ljava/util/List;Ljava/util/List;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V @@ -663,7 +672,8 @@ public final class misk/jdbc/TruncateTablesService : com/google/common/util/conc public fun (Lkotlin/reflect/KClass;Lmisk/jdbc/DataSourceService;Lcom/google/inject/Provider;)V public fun (Lkotlin/reflect/KClass;Lmisk/jdbc/DataSourceService;Lcom/google/inject/Provider;Ljava/util/List;)V public fun (Lkotlin/reflect/KClass;Lmisk/jdbc/DataSourceService;Lcom/google/inject/Provider;Ljava/util/List;Ljava/util/List;)V - public synthetic fun (Lkotlin/reflect/KClass;Lmisk/jdbc/DataSourceService;Lcom/google/inject/Provider;Ljava/util/List;Ljava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lkotlin/reflect/KClass;Lmisk/jdbc/DataSourceService;Lcom/google/inject/Provider;Ljava/util/List;Ljava/util/List;Lmisk/jdbc/JdbcTestFixture;)V + public synthetic fun (Lkotlin/reflect/KClass;Lmisk/jdbc/DataSourceService;Lcom/google/inject/Provider;Ljava/util/List;Ljava/util/List;Lmisk/jdbc/JdbcTestFixture;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public final class misk/vitess/ConnectionExtensionsKt { diff --git a/misk-jdbc/build.gradle.kts b/misk-jdbc/build.gradle.kts index 6deaa20a84a..4cf969c5df4 100644 --- a/misk-jdbc/build.gradle.kts +++ b/misk-jdbc/build.gradle.kts @@ -42,6 +42,7 @@ dependencies { testFixturesApi(libs.okHttp) testFixturesApi(project(":misk-inject")) testFixturesApi(project(":misk-jdbc")) + testFixturesApi(project(":misk-testing-api")) testFixturesImplementation(libs.guice) testFixturesImplementation(libs.hikariCp) testFixturesImplementation(libs.kotlinLogging) diff --git a/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt new file mode 100644 index 00000000000..66bda0ff305 --- /dev/null +++ b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt @@ -0,0 +1,80 @@ +package misk.jdbc + +import com.google.common.base.Stopwatch +import com.google.inject.Provider +import misk.testing.TestFixture +import misk.vitess.shards +import misk.vitess.target +import wisp.logging.getLogger +import java.util.Locale +import kotlin.reflect.KClass + +class JdbcTestFixture( + private val qualifier: KClass, + private val dataSourceService: DataSourceService, + private val transacterProvider: Provider +) : TestFixture { + private val persistentTables = setOf("schema_version") + + override fun reset() { + val stopwatch = Stopwatch.createStarted() + + val truncatedTableNames = shards(dataSourceService).get().flatMap { shard -> + transacterProvider.get().transaction { connection -> + CheckDisabler.withoutChecks { + connection.target(shard) { + val config = dataSourceService.config() + val tableNamesQuery = when (config.type) { + DataSourceType.MYSQL, DataSourceType.TIDB -> { + "SELECT table_name FROM information_schema.tables where table_schema='${config.database}' AND table_type='BASE TABLE'" + } + DataSourceType.HSQLDB -> { + "SELECT TABLE_NAME FROM INFORMATION_SCHEMA.SYSTEM_TABLES WHERE TABLE_TYPE='TABLE'" + } + DataSourceType.VITESS_MYSQL -> { + "SHOW VSCHEMA TABLES" + } + DataSourceType.COCKROACHDB, DataSourceType.POSTGRESQL -> { + "SELECT table_name FROM information_schema.tables WHERE table_catalog='${config.database}' AND table_schema='public'" + } + } + + @Suppress("UNCHECKED_CAST") // createNativeQuery returns a raw Query. + val allTableNames = connection.createStatement().use { s -> + s.executeQuery(tableNamesQuery).map { rs -> rs.getString(1) } + } + + val truncatedTableNames = mutableListOf() + connection.createStatement().use { statement -> + for (tableName in allTableNames) { + if (persistentTables.contains(tableName.toLowerCase(Locale.ROOT))) continue + if (tableName.endsWith("_seq") || tableName.equals("dual")) continue + + if (config.type == DataSourceType.COCKROACHDB || config.type == DataSourceType.POSTGRESQL) { + statement.addBatch("TRUNCATE $tableName CASCADE") + } else { + statement.addBatch("DELETE FROM $tableName") + } + truncatedTableNames += tableName + } + statement.executeBatch() + } + + truncatedTableNames + } + } + } + } + + if (truncatedTableNames.isNotEmpty()) { + logger.info { + "@${qualifier.simpleName} TruncateTables truncated ${truncatedTableNames.size} " + + "tables in $stopwatch" + } + } + } + + companion object { + private val logger = getLogger() + } +} diff --git a/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestingModule.kt b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestingModule.kt index 1cec588de0c..6e995ccce96 100644 --- a/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestingModule.kt +++ b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestingModule.kt @@ -12,6 +12,7 @@ import misk.time.ForceUtcTimeZoneService import misk.vitess.VitessScaleSafetyChecks import okhttp3.OkHttpClient import com.google.inject.Provider +import misk.testing.TestFixture import kotlin.reflect.KClass /** @@ -45,7 +46,14 @@ class JdbcTestingModule( .dependsOn(qualifier) .enhancedBy() ) - bind(truncateTablesServiceKey).toProvider(Provider { + multibind().toProvider { + JdbcTestFixture( + qualifier = qualifier, + dataSourceService = dataSourceServiceProvider.get(), + transacterProvider = transacterProvider + ) + }.asSingleton() + bind(truncateTablesServiceKey).toProvider { TruncateTablesService( qualifier = qualifier, dataSourceService = dataSourceServiceProvider.get(), @@ -53,7 +61,7 @@ class JdbcTestingModule( startUpStatements = startUpStatements, shutDownStatements = shutDownStatements ) - }).asSingleton() + }.asSingleton() if (scaleSafetyChecks) bindScaleSafetyChecks() } diff --git a/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/TruncateTablesService.kt b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/TruncateTablesService.kt index f0b2a944d4f..ca4f15122be 100644 --- a/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/TruncateTablesService.kt +++ b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/TruncateTablesService.kt @@ -2,11 +2,8 @@ package misk.jdbc import com.google.common.base.Stopwatch import com.google.common.util.concurrent.AbstractIdleService -import misk.vitess.shards -import misk.vitess.target -import wisp.logging.getLogger -import java.util.* import com.google.inject.Provider +import wisp.logging.getLogger import kotlin.reflect.KClass private val logger = getLogger() @@ -25,12 +22,12 @@ class TruncateTablesService @JvmOverloads constructor( private val dataSourceService: DataSourceService, private val transacterProvider: Provider, private val startUpStatements: List = listOf(), - private val shutDownStatements: List = listOf() + private val shutDownStatements: List = listOf(), + private val jdbcTestFixture: JdbcTestFixture = JdbcTestFixture(qualifier, dataSourceService, transacterProvider), ) : AbstractIdleService() { - private val persistentTables = setOf("schema_version") override fun startUp() { - truncateUserTables() + jdbcTestFixture.reset() executeStatements(startUpStatements, "startup") } @@ -38,64 +35,6 @@ class TruncateTablesService @JvmOverloads constructor( executeStatements(shutDownStatements, "shutdown") } - private fun truncateUserTables() { - val stopwatch = Stopwatch.createStarted() - - val truncatedTableNames = shards(dataSourceService).get().flatMap { shard -> - transacterProvider.get().transaction { connection -> - CheckDisabler.withoutChecks { - connection.target(shard) { - val config = dataSourceService.config() - val tableNamesQuery = when (config.type) { - DataSourceType.MYSQL, DataSourceType.TIDB -> { - "SELECT table_name FROM information_schema.tables where table_schema='${config.database}' AND table_type='BASE TABLE'" - } - DataSourceType.HSQLDB -> { - "SELECT TABLE_NAME FROM INFORMATION_SCHEMA.SYSTEM_TABLES WHERE TABLE_TYPE='TABLE'" - } - DataSourceType.VITESS_MYSQL -> { - "SHOW VSCHEMA TABLES" - } - DataSourceType.COCKROACHDB, DataSourceType.POSTGRESQL -> { - "SELECT table_name FROM information_schema.tables WHERE table_catalog='${config.database}' AND table_schema='public'" - } - } - - @Suppress("UNCHECKED_CAST") // createNativeQuery returns a raw Query. - val allTableNames = connection.createStatement().use { s -> - s.executeQuery(tableNamesQuery).map { rs -> rs.getString(1) } - } - - val truncatedTableNames = mutableListOf() - connection.createStatement().use { statement -> - for (tableName in allTableNames) { - if (persistentTables.contains(tableName.toLowerCase(Locale.ROOT))) continue - if (tableName.endsWith("_seq") || tableName.equals("dual")) continue - - if (config.type == DataSourceType.COCKROACHDB || config.type == DataSourceType.POSTGRESQL) { - statement.addBatch("TRUNCATE $tableName CASCADE") - } else { - statement.addBatch("DELETE FROM $tableName") - } - truncatedTableNames += tableName - } - statement.executeBatch() - } - - truncatedTableNames - } - } - } - } - - if (truncatedTableNames.isNotEmpty()) { - logger.info { - "@${qualifier.simpleName} TruncateTablesService truncated ${truncatedTableNames.size} " + - "tables in $stopwatch" - } - } - } - private fun executeStatements(statements: List, name: String) { val stopwatch = Stopwatch.createStarted() From 0840104728b5b45046b65f5128b167d092c05be8 Mon Sep 17 00:00:00 2001 From: David Amar <100435712+damar-block@users.noreply.github.com> Date: Tue, 6 Aug 2024 13:16:09 -0700 Subject: [PATCH 04/26] Support more source formats and Guice explorer GitOrigin-RevId: b6a255f455c40ef5e21059ab5582a939be572b35 --- misk-admin/api/misk-admin.api | 12 +-- .../metadata/guice/GitHubSourceUrlProvider.kt | 73 ++++++++++++++++--- .../guice/GitHubSourceUrlProviderTest.kt | 44 ++++++++++- 3 files changed, 111 insertions(+), 18 deletions(-) diff --git a/misk-admin/api/misk-admin.api b/misk-admin/api/misk-admin.api index c18c60a2371..d2a238baa24 100644 --- a/misk-admin/api/misk-admin.api +++ b/misk-admin/api/misk-admin.api @@ -931,17 +931,19 @@ public class misk/web/metadata/guice/GitHubSourceUrlProvider : misk/web/metadata } protected final class misk/web/metadata/guice/GitHubSourceUrlProvider$SourceLocation { - public fun (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;I)V + public fun (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Integer;)V public final fun component1 ()Ljava/lang/String; public final fun component2 ()Ljava/lang/String; public final fun component3 ()Ljava/lang/String; - public final fun component4 ()I - public final fun copy (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;I)Lmisk/web/metadata/guice/GitHubSourceUrlProvider$SourceLocation; - public static synthetic fun copy$default (Lmisk/web/metadata/guice/GitHubSourceUrlProvider$SourceLocation;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;IILjava/lang/Object;)Lmisk/web/metadata/guice/GitHubSourceUrlProvider$SourceLocation; + public final fun component4 ()Ljava/lang/String; + public final fun component5 ()Ljava/lang/Integer; + public final fun copy (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Integer;)Lmisk/web/metadata/guice/GitHubSourceUrlProvider$SourceLocation; + public static synthetic fun copy$default (Lmisk/web/metadata/guice/GitHubSourceUrlProvider$SourceLocation;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Integer;ILjava/lang/Object;)Lmisk/web/metadata/guice/GitHubSourceUrlProvider$SourceLocation; public fun equals (Ljava/lang/Object;)Z public final fun getClassName ()Ljava/lang/String; public final fun getFunctionName ()Ljava/lang/String; - public final fun getLineNumber ()I + public final fun getInnerClassName ()Ljava/lang/String; + public final fun getLineNumber ()Ljava/lang/Integer; public final fun getPackageName ()Ljava/lang/String; public fun hashCode ()I public fun toString ()Ljava/lang/String; diff --git a/misk-admin/src/main/kotlin/misk/web/metadata/guice/GitHubSourceUrlProvider.kt b/misk-admin/src/main/kotlin/misk/web/metadata/guice/GitHubSourceUrlProvider.kt index 9f5a1f3aef4..9723ee8ff03 100644 --- a/misk-admin/src/main/kotlin/misk/web/metadata/guice/GitHubSourceUrlProvider.kt +++ b/misk-admin/src/main/kotlin/misk/web/metadata/guice/GitHubSourceUrlProvider.kt @@ -8,12 +8,15 @@ import java.net.URLEncoder */ open class GitHubSourceUrlProvider @Inject constructor() : GuiceSourceUrlProvider { - private val sourceWithLineNumberRegex = """^([\w.]+)\.((?:\w+\$?)+)\.(\w+)\(.*:(\d+)\)$""".toRegex() + private val sourceWithLineNumberRegex = Regex("""^([\w.]+)\.((?:\w+\$?)+)\.(\w+)\(.*:(\d+)\)$""") + private val classRegex = Regex("""class\s+([a-zA-Z0-9_.$]+)""") + private val functionRegex = Regex("""(?:\w+\s+)?(?:\w+\s+)?(?:\w+\s+)?([a-zA-Z0-9_.$]+)\.([a-zA-Z0-9_.$]+)\(([^)]*)\)""") protected data class SourceLocation( val packageName: String, val className: String, - val functionName: String, - val lineNumber: Int, + val innerClassName: String?, + val functionName: String?, + val lineNumber: Int?, ) override fun urlForSource(source: String): String? { val sourceLocation = maybeSourceLocation(source) ?: return null @@ -21,7 +24,16 @@ open class GitHubSourceUrlProvider @Inject constructor() : GuiceSourceUrlProvide } protected open fun generateQuery(source: SourceLocation): String { - return """"package ${source.packageName}" ${source.className.replace('$', ' ')} ${source.functionName}""" + val sb = StringBuilder() + sb.append(""""package ${source.packageName}"""") + sb.append(" ${source.className}") + if (source.innerClassName != null) { + sb.append(" ${source.innerClassName}") + } + if (source.functionName != null) { + sb.append(" ${source.functionName}") + } + return sb.toString() } private fun githubSearchUrl(source: SourceLocation): String { @@ -30,15 +42,54 @@ open class GitHubSourceUrlProvider @Inject constructor() : GuiceSourceUrlProvide } private fun maybeSourceLocation(source: String): SourceLocation? { - val matchResult = sourceWithLineNumberRegex.matchEntire(source) - - return if (matchResult != null) { - // Extract the package, class, function names, and line number from the match groups + sourceWithLineNumberRegex.matchEntire(source)?.let {matchResult -> val (packageName, className, functionName, lineNumberStr) = matchResult.destructured + val (innerClassName, outerClassName) = parseClass(className) val lineNumber = lineNumberStr.toInt() - SourceLocation(packageName, className, functionName, lineNumber) - } else { - null + return SourceLocation( + packageName = packageName, + className = outerClassName, + innerClassName = innerClassName, + functionName = functionName, + lineNumber = lineNumber + ) + } + + classRegex.find(source)?.let { matchResult -> + val fullClassName = matchResult.groupValues[1] + val packageName = fullClassName.substringBeforeLast('.') + val className = fullClassName.substringAfterLast('.') + val (innerClassName, outerClassName) = parseClass(className) + return SourceLocation( + packageName = packageName, + className = outerClassName, + innerClassName = innerClassName, + functionName = null, + lineNumber = null + ) } + + functionRegex.find(source)?.let {matchResult -> + val fullClassName = matchResult.groupValues[1] + val functionName = matchResult.groupValues[2].substringBefore('$') + val packageName = fullClassName.substringBeforeLast('.') + val className = fullClassName.substringAfterLast('.') + val (innerClassName, outerClassName) = parseClass(className) + return SourceLocation( + packageName = packageName, + className = outerClassName, + innerClassName = innerClassName, + functionName = functionName, + lineNumber = null + ) + } + + return null + } + + private fun parseClass(className: String): Pair { + val innerClassName = if ('$' in className) className.substringAfter('$') else null + val outerClassName = if ('$' in className) className.substringBefore('$') else className + return Pair(innerClassName, outerClassName) } } diff --git a/misk-admin/src/test/kotlin/misk/web/metadata/guice/GitHubSourceUrlProviderTest.kt b/misk-admin/src/test/kotlin/misk/web/metadata/guice/GitHubSourceUrlProviderTest.kt index dcd0964df53..f51a090ffa7 100644 --- a/misk-admin/src/test/kotlin/misk/web/metadata/guice/GitHubSourceUrlProviderTest.kt +++ b/misk-admin/src/test/kotlin/misk/web/metadata/guice/GitHubSourceUrlProviderTest.kt @@ -7,8 +7,8 @@ class GitHubSourceUrlProviderTest { @Test fun testUrlForSource() { verifyUrl( - "com.squareup.skim.dashboard.SkimDashboardModule.configure(SkimDashboardModule.kt:23)", - "https://github.com/search?q=%22package+com.squareup.skim.dashboard%22+SkimDashboardModule+configure&type=code" + "com.squareup.misk.dashboard.MiskDashboardModule.configure(MiskDashboardModule.kt:23)", + "https://github.com/search?q=%22package+com.squareup.misk.dashboard%22+MiskDashboardModule+configure&type=code" ) } @@ -28,6 +28,46 @@ class GitHubSourceUrlProviderTest { ) } + @Test + fun testUrlForFunction() { + verifyUrl( + "public final com.squareup.wire.reflector.SchemaReflector misk.grpc.reflect.GrpcReflectModule.provideServiceReflector(com.squareup.wire.schema.Schema, com.squareup.wire.schema.Schema)", + "https://github.com/search?q=%22package+misk.grpc.reflect%22+GrpcReflectModule+provideServiceReflector&type=code" + ) + } + + @Test + fun testUrlForFunctionWithDollar() { + verifyUrl( + "public final com.squareup.wire.reflector.SchemaReflector misk.grpc.reflect.GrpcReflectModule.provideServiceReflector\$service(com.squareup.wire.schema.Schema, com.squareup.wire.schema.Schema)", + "https://github.com/search?q=%22package+misk.grpc.reflect%22+GrpcReflectModule+provideServiceReflector&type=code" + ) + } + + @Test + fun testUrlForFunctionWithNoArguments() { + verifyUrl( + "public final com.squareup.wire.reflector.SchemaReflector misk.grpc.reflect.GrpcReflectModule.provideServiceReflector()", + "https://github.com/search?q=%22package+misk.grpc.reflect%22+GrpcReflectModule+provideServiceReflector&type=code" + ) + } + + @Test + fun testUrlForClass() { + verifyUrl( + "class com.squareup.skim.logging.SkimLoggingStartupCheck", + "https://github.com/search?q=%22package+com.squareup.skim.logging%22+SkimLoggingStartupCheck&type=code" + ) + } + + @Test + fun testUrlForInnerClass() { + verifyUrl( + "class misk.metrics.MetricsModule\$V2MetricsProvider", + "https://github.com/search?q=%22package+misk.metrics%22+MetricsModule+V2MetricsProvider&type=code" + ) + } + private fun verifyUrl(source: String, expectedUrl: String) { val provider = GitHubSourceUrlProvider() val url = provider.urlForSource(source) From e62370b1092605d64875ebe93f8d3e71ad78fb20 Mon Sep 17 00:00:00 2001 From: Giancarlo Salamanca Date: Tue, 6 Aug 2024 19:42:51 -0400 Subject: [PATCH 05/26] Updates the hashing algorithm used in the `ClusterHashRing` implementation of `ClusterResourceMapper` from `murmur3_32` in the `com.google.common.hash.Hashing` package, to `murmur3_32_fixed`, which addresses a bug when generating a hashed value from a string containing non-BMP characters. GitOrigin-RevId: f1d4efcd61eb40270b838221bf8a0fc1bca38b1d --- .../src/main/kotlin/misk/clustering/ClusterHashRing.kt | 2 +- .../test/kotlin/misk/clustering/ClusterHashRingTest.kt | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/misk-clustering/src/main/kotlin/misk/clustering/ClusterHashRing.kt b/misk-clustering/src/main/kotlin/misk/clustering/ClusterHashRing.kt index a8014d60e50..652c4ef21e2 100644 --- a/misk-clustering/src/main/kotlin/misk/clustering/ClusterHashRing.kt +++ b/misk-clustering/src/main/kotlin/misk/clustering/ClusterHashRing.kt @@ -8,7 +8,7 @@ import java.util.Objects /** A [ClusterHashRing] maps resources to cluster members based on a consistent hash */ class ClusterHashRing @JvmOverloads constructor( members: Collection, - private val hashFn: HashFunction = Hashing.murmur3_32(), + private val hashFn: HashFunction = Hashing.murmur3_32_fixed(), private val vnodesCount: Int = 16 ) : ClusterResourceMapper { private val vnodes: IntArray diff --git a/misk-clustering/src/test/kotlin/misk/clustering/ClusterHashRingTest.kt b/misk-clustering/src/test/kotlin/misk/clustering/ClusterHashRingTest.kt index 1dd23fbdb0a..d5588e67d21 100644 --- a/misk-clustering/src/test/kotlin/misk/clustering/ClusterHashRingTest.kt +++ b/misk-clustering/src/test/kotlin/misk/clustering/ClusterHashRingTest.kt @@ -11,7 +11,7 @@ internal class ClusterHashRingTest { @Test fun singleNode() { val zork = Cluster.Member("zork", "192.49.168.23") val hashRing = - ClusterHashRing(members = setOf(zork), hashFn = Hashing.murmur3_32(0)) + ClusterHashRing(members = setOf(zork), hashFn = Hashing.murmur3_32_fixed(0)) assertThat(listOf("foo", "bar", "zed").map { hashRing[it] }).containsExactly(zork, zork, zork) } @@ -23,7 +23,7 @@ internal class ClusterHashRingTest { // First version of hash ring val hashRing1 = ClusterHashRing( members = setOf(zork, mork, quark), - hashFn = Hashing.murmur3_32(0) + hashFn = Hashing.murmur3_32_fixed(0) ) assertThat( listOf("foo", "bar", "zed", "abadidea").map { @@ -34,7 +34,7 @@ internal class ClusterHashRingTest { // Remove one of the members, only the resources mapped to that member should change val hashRing2 = ClusterHashRing( members = setOf(zork, quark), - hashFn = Hashing.murmur3_32(0) + hashFn = Hashing.murmur3_32_fixed(0) ) assertThat(listOf("foo", "bar", "zed", "abadidea").map { hashRing2[it] }) .containsExactly(zork, quark, zork, zork) @@ -43,7 +43,7 @@ internal class ClusterHashRingTest { val bork = Cluster.Member("bork", "192.49.168.26") val hashRing3 = ClusterHashRing( members = setOf(zork, quark, bork), - hashFn = Hashing.murmur3_32(0) + hashFn = Hashing.murmur3_32_fixed(0) ) assertThat(listOf("foo", "bar", "zed", "abadidea").map { hashRing3[it] }) .containsExactly(zork, quark, zork, zork) @@ -51,7 +51,7 @@ internal class ClusterHashRingTest { @Test fun zeroNodes() { val hashRing = - ClusterHashRing(members = setOf(), hashFn = Hashing.murmur3_32(0)) + ClusterHashRing(members = setOf(), hashFn = Hashing.murmur3_32_fixed(0)) assertThrows { hashRing["foo"] } From aa49aca2e60f1691af6621a83a99a74312c18023 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Wed, 7 Aug 2024 11:58:07 +1000 Subject: [PATCH 06/26] Implement TestFixture for dynamodb GitOrigin-RevId: 43385fdc20d5036dd27002569f988ad76328c3e0 --- gradle/libs.versions.toml | 2 +- misk-aws-dynamodb/api/misk-aws-dynamodb.api | 3 ++- .../misk/aws/dynamodb/testing/DockerDynamoDbModule.kt | 2 ++ .../misk/aws/dynamodb/testing/InProcessDynamoDbModule.kt | 2 ++ .../kotlin/misk/aws/dynamodb/testing/TestDynamoDb.kt | 7 ++++++- misk-aws2-dynamodb/api/misk-aws2-dynamodb.api | 3 ++- .../misk/aws2/dynamodb/testing/DockerDynamoDbModule.kt | 2 ++ .../misk/aws2/dynamodb/testing/InProcessDynamoDbModule.kt | 2 ++ .../kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt | 7 ++++++- 9 files changed, 25 insertions(+), 5 deletions(-) diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 01092e2fdff..621bbe17863 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -187,7 +187,7 @@ tempest2Testing = { module = "app.cash.tempest:tempest2-testing" } tempest2TestingDocker = { module = "app.cash.tempest:tempest2-testing-docker" } tempest2TestingInternal = { module = "app.cash.tempest:tempest2-testing-internal" } tempest2TestingJvm = { module = "app.cash.tempest:tempest2-testing-jvm" } -tempestBom = { module = "app.cash.tempest:tempest-bom", version = "2024.03.25.180845-91fd675" } +tempestBom = { module = "app.cash.tempest:tempest-bom", version = "2024.08.07.002316-64f40ef" } tempestTesting = { module = "app.cash.tempest:tempest-testing" } tempestTestingDocker = { module = "app.cash.tempest:tempest-testing-docker" } tempestTestingInternal = { module = "app.cash.tempest:tempest-testing-internal" } diff --git a/misk-aws-dynamodb/api/misk-aws-dynamodb.api b/misk-aws-dynamodb/api/misk-aws-dynamodb.api index 3b359f5054c..0e26aaa9c82 100644 --- a/misk-aws-dynamodb/api/misk-aws-dynamodb.api +++ b/misk-aws-dynamodb/api/misk-aws-dynamodb.api @@ -33,7 +33,7 @@ public final class misk/aws/dynamodb/testing/InProcessDynamoDbModule : misk/inje public final fun providesDynamoDbServiceWrapper ()Lmisk/aws/dynamodb/testing/TestDynamoDb; } -public final class misk/aws/dynamodb/testing/TestDynamoDb : com/google/common/util/concurrent/Service { +public final class misk/aws/dynamodb/testing/TestDynamoDb : com/google/common/util/concurrent/Service, misk/testing/TestFixture { public fun (Lapp/cash/tempest/testing/internal/TestDynamoDbService;)V public fun addListener (Lcom/google/common/util/concurrent/Service$Listener;Ljava/util/concurrent/Executor;)V public fun awaitRunning ()V @@ -43,6 +43,7 @@ public final class misk/aws/dynamodb/testing/TestDynamoDb : com/google/common/ut public fun failureCause ()Ljava/lang/Throwable; public final fun getService ()Lapp/cash/tempest/testing/internal/TestDynamoDbService; public fun isRunning ()Z + public fun reset ()V public fun startAsync ()Lcom/google/common/util/concurrent/Service; public fun state ()Lcom/google/common/util/concurrent/Service$State; public fun stopAsync ()Lcom/google/common/util/concurrent/Service; diff --git a/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/DockerDynamoDbModule.kt b/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/DockerDynamoDbModule.kt index 70b3507d684..dd867f65e92 100644 --- a/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/DockerDynamoDbModule.kt +++ b/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/DockerDynamoDbModule.kt @@ -13,6 +13,7 @@ import misk.ServiceModule import misk.dynamodb.DynamoDbService import misk.dynamodb.RequiredDynamoDbTable import misk.inject.KAbstractModule +import misk.testing.TestFixture import kotlin.reflect.KClass /** @@ -35,6 +36,7 @@ class DockerDynamoDbModule( bind().to() install(ServiceModule().dependsOn()) install(ServiceModule()) + multibind().to() } @Provides @Singleton diff --git a/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/InProcessDynamoDbModule.kt b/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/InProcessDynamoDbModule.kt index 783a3fe426d..fef8321c9f6 100644 --- a/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/InProcessDynamoDbModule.kt +++ b/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/InProcessDynamoDbModule.kt @@ -13,6 +13,7 @@ import misk.ServiceModule import misk.dynamodb.DynamoDbService import misk.dynamodb.RequiredDynamoDbTable import misk.inject.KAbstractModule +import misk.testing.TestFixture import kotlin.reflect.KClass /** @@ -37,6 +38,7 @@ class InProcessDynamoDbModule( bind().to() install(ServiceModule().dependsOn()) install(ServiceModule()) + multibind().to() } @Provides diff --git a/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/TestDynamoDb.kt b/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/TestDynamoDb.kt index f5782bddb7e..62f74ed1d4b 100644 --- a/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/TestDynamoDb.kt +++ b/misk-aws-dynamodb/src/testFixtures/kotlin/misk/aws/dynamodb/testing/TestDynamoDb.kt @@ -4,6 +4,7 @@ import app.cash.tempest.testing.internal.TestDynamoDbService import com.google.common.util.concurrent.Service import jakarta.inject.Inject import jakarta.inject.Singleton +import misk.testing.TestFixture /** * Thin wrapper to make `TestDynamoDbService`, which is not a @Singleton, compatible with `ServiceModule`. @@ -11,4 +12,8 @@ import jakarta.inject.Singleton @Singleton class TestDynamoDb @Inject constructor( val service: TestDynamoDbService -) : Service by service +) : Service by service, TestFixture { + override fun reset() { + service.client.reset() + } +} diff --git a/misk-aws2-dynamodb/api/misk-aws2-dynamodb.api b/misk-aws2-dynamodb/api/misk-aws2-dynamodb.api index facd3f64d2e..a08b9f15ac1 100644 --- a/misk-aws2-dynamodb/api/misk-aws2-dynamodb.api +++ b/misk-aws2-dynamodb/api/misk-aws2-dynamodb.api @@ -65,7 +65,7 @@ public final class misk/aws2/dynamodb/testing/InProcessDynamoDbModule : misk/inj public final fun providesTestDynamoDb ()Lmisk/aws2/dynamodb/testing/TestDynamoDb; } -public final class misk/aws2/dynamodb/testing/TestDynamoDb : com/google/common/util/concurrent/Service { +public final class misk/aws2/dynamodb/testing/TestDynamoDb : com/google/common/util/concurrent/Service, misk/testing/TestFixture { public fun (Lapp/cash/tempest2/testing/internal/TestDynamoDbService;)V public fun addListener (Lcom/google/common/util/concurrent/Service$Listener;Ljava/util/concurrent/Executor;)V public fun awaitRunning ()V @@ -75,6 +75,7 @@ public final class misk/aws2/dynamodb/testing/TestDynamoDb : com/google/common/u public fun failureCause ()Ljava/lang/Throwable; public final fun getService ()Lapp/cash/tempest2/testing/internal/TestDynamoDbService; public fun isRunning ()Z + public fun reset ()V public fun startAsync ()Lcom/google/common/util/concurrent/Service; public fun state ()Lcom/google/common/util/concurrent/Service$State; public fun stopAsync ()Lcom/google/common/util/concurrent/Service; diff --git a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/DockerDynamoDbModule.kt b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/DockerDynamoDbModule.kt index cccf6ac4adc..e86a8f3ac8d 100644 --- a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/DockerDynamoDbModule.kt +++ b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/DockerDynamoDbModule.kt @@ -11,6 +11,7 @@ import misk.ServiceModule import misk.aws2.dynamodb.DynamoDbService import misk.aws2.dynamodb.RequiredDynamoDbTable import misk.inject.KAbstractModule +import misk.testing.TestFixture import software.amazon.awssdk.services.dynamodb.DynamoDbClient import software.amazon.awssdk.services.dynamodb.streams.DynamoDbStreamsClient @@ -33,6 +34,7 @@ class DockerDynamoDbModule( bind().to() install(ServiceModule().dependsOn()) install(ServiceModule()) + multibind().to() } @Provides diff --git a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/InProcessDynamoDbModule.kt b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/InProcessDynamoDbModule.kt index e2e5f142cef..d37aa54ecc0 100644 --- a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/InProcessDynamoDbModule.kt +++ b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/InProcessDynamoDbModule.kt @@ -11,6 +11,7 @@ import misk.ServiceModule import misk.aws2.dynamodb.DynamoDbService import misk.aws2.dynamodb.RequiredDynamoDbTable import misk.inject.KAbstractModule +import misk.testing.TestFixture import software.amazon.awssdk.services.dynamodb.DynamoDbClient import software.amazon.awssdk.services.dynamodb.streams.DynamoDbStreamsClient @@ -34,6 +35,7 @@ class InProcessDynamoDbModule( bind().to() install(ServiceModule().dependsOn()) install(ServiceModule()) + multibind().to() } @Provides diff --git a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt index cfbeeb34c95..33775f3777c 100644 --- a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt +++ b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt @@ -4,6 +4,7 @@ import app.cash.tempest2.testing.internal.TestDynamoDbService import com.google.common.util.concurrent.Service import jakarta.inject.Inject import jakarta.inject.Singleton +import misk.testing.TestFixture /** * Thin wrapper to make `TestDynamoDbService`, which is not a @Singleton, compatible with `ServiceModule`. @@ -11,4 +12,8 @@ import jakarta.inject.Singleton @Singleton class TestDynamoDb @Inject constructor( val service: TestDynamoDbService -) : Service by service +) : Service by service, TestFixture { + override fun reset() { + service.client.reset() + } +} From 4954777f6f183e4ac59a2f7621de4baf6febe849 Mon Sep 17 00:00:00 2001 From: Shivani Katukota <54708104+katukota@users.noreply.github.com> Date: Wed, 7 Aug 2024 16:47:26 -0400 Subject: [PATCH 07/26] Expose service graph in admin console using d3 graph template from https://observablehq.com/@d3/mobile-patent-suits?intent=fork GitOrigin-RevId: 02456e0997b5e0a5c4c5dd2229edc0326bdabfc1 --- misk-admin/api/misk-admin.api | 14 ++ misk-admin/build.gradle.kts | 1 + .../web/dashboard/AdminDashboardModule.kt | 2 + .../ServiceGraphDashboardTabModule.kt | 19 ++ .../ServiceGraphTabIndexAction.kt | 176 ++++++++++++++++++ .../web/metadata/all/AllMetadataActionTest.kt | 3 +- misk-service/api/misk-service.api | 55 ++++++ misk-service/build.gradle.kts | 2 +- .../main/kotlin/misk/CoordinatedService.kt | 12 +- .../main/kotlin/misk/ServiceGraphBuilder.kt | 20 +- .../main/kotlin/misk/ServiceManagerModule.kt | 1 + .../servicegraph/ServiceGraphMetadata.kt | 48 ++++- 12 files changed, 327 insertions(+), 26 deletions(-) create mode 100644 misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphDashboardTabModule.kt create mode 100644 misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt diff --git a/misk-admin/api/misk-admin.api b/misk-admin/api/misk-admin.api index d2a238baa24..7a778ae9af5 100644 --- a/misk-admin/api/misk-admin.api +++ b/misk-admin/api/misk-admin.api @@ -967,6 +967,20 @@ public final class misk/web/metadata/guice/GuiceTabIndexAction : misk/web/action public final class misk/web/metadata/guice/GuiceTabIndexAction$Companion { } +public final class misk/web/metadata/servicegraph/ServiceGraphDashboardTabModule : misk/inject/KAbstractModule { + public fun ()V +} + +public final class misk/web/metadata/servicegraph/ServiceGraphTabIndexAction : misk/web/actions/WebAction { + public static final field Companion Lmisk/web/metadata/servicegraph/ServiceGraphTabIndexAction$Companion; + public static final field PATH Ljava/lang/String; + public fun (Lmisk/web/v2/DashboardPageLayout;Lcom/google/inject/Provider;)V + public final fun get ()Ljava/lang/String; +} + +public final class misk/web/metadata/servicegraph/ServiceGraphTabIndexAction$Companion { +} + public final class misk/web/metadata/webaction/WebActionsDashboardTabModule : misk/inject/KAbstractModule { public fun (Z)V } diff --git a/misk-admin/build.gradle.kts b/misk-admin/build.gradle.kts index 044b71a23b4..4df860330d8 100644 --- a/misk-admin/build.gradle.kts +++ b/misk-admin/build.gradle.kts @@ -20,6 +20,7 @@ dependencies { api(project(":misk-actions")) api(project(":misk-config")) api(project(":misk-inject")) + api(project(":misk-service")) api(libs.kotlinxHtml) implementation(libs.kotlinLogging) implementation(libs.moshiCore) diff --git a/misk-admin/src/main/kotlin/misk/web/dashboard/AdminDashboardModule.kt b/misk-admin/src/main/kotlin/misk/web/dashboard/AdminDashboardModule.kt index 12cc0fb6a01..dd7e453f9df 100644 --- a/misk-admin/src/main/kotlin/misk/web/dashboard/AdminDashboardModule.kt +++ b/misk-admin/src/main/kotlin/misk/web/dashboard/AdminDashboardModule.kt @@ -7,6 +7,7 @@ import misk.web.metadata.config.ConfigDashboardTabModule import misk.web.metadata.config.ConfigMetadataAction import misk.web.metadata.database.DatabaseDashboardTabModule import misk.web.metadata.guice.GuiceDashboardTabModule +import misk.web.metadata.servicegraph.ServiceGraphDashboardTabModule import misk.web.metadata.webaction.WebActionsDashboardTabModule import misk.web.v2.NavbarModule @@ -35,6 +36,7 @@ class AdminDashboardModule @JvmOverloads constructor( install(ConfigDashboardTabModule(isDevelopment, configTabMode)) install(DatabaseDashboardTabModule(isDevelopment)) install(GuiceDashboardTabModule()) + install(ServiceGraphDashboardTabModule()) install(WebActionsDashboardTabModule(isDevelopment)) } } diff --git a/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphDashboardTabModule.kt b/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphDashboardTabModule.kt new file mode 100644 index 00000000000..c9042337eb6 --- /dev/null +++ b/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphDashboardTabModule.kt @@ -0,0 +1,19 @@ +package misk.web.metadata.servicegraph + +import misk.inject.KAbstractModule +import misk.web.WebActionModule +import misk.web.dashboard.AdminDashboard +import misk.web.dashboard.AdminDashboardAccess +import misk.web.dashboard.DashboardModule + +class ServiceGraphDashboardTabModule: KAbstractModule() { + override fun configure() { + install(WebActionModule.create()) + install(DashboardModule.createHotwireTab( + slug = "service-graph", + urlPathPrefix = ServiceGraphTabIndexAction.PATH, + menuCategory = "Container Admin", + menuLabel = "Service Graph", + )) + } +} diff --git a/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt b/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt new file mode 100644 index 00000000000..4e917340377 --- /dev/null +++ b/misk-admin/src/main/kotlin/misk/web/metadata/servicegraph/ServiceGraphTabIndexAction.kt @@ -0,0 +1,176 @@ +package misk.web.metadata.servicegraph + +import com.google.inject.Provider +import jakarta.inject.Inject +import jakarta.inject.Singleton +import kotlinx.html.div +import kotlinx.html.h1 +import kotlinx.html.script +import kotlinx.html.unsafe +import misk.metadata.servicegraph.ServiceGraphMetadata +import misk.web.Get +import misk.web.ResponseContentType +import misk.web.actions.WebAction +import misk.web.dashboard.AdminDashboardAccess +import misk.web.mediatype.MediaTypes +import misk.web.metadata.toFormattedJson +import misk.web.v2.DashboardPageLayout +import wisp.moshi.adapter +import wisp.moshi.defaultKotlinMoshi + +@Singleton +class ServiceGraphTabIndexAction @Inject constructor( + private val dashboardPageLayout: DashboardPageLayout, + private val serviceGraphMetadataProvider: Provider, +) : WebAction { + @Get(PATH) + @ResponseContentType(MediaTypes.TEXT_HTML) + @AdminDashboardAccess + fun get(): String = dashboardPageLayout + .newBuilder() + .headBlock { + script { + src = "https://cdn.jsdelivr.net/npm/d3@7" + type = "text/javascript" + } + } + .build { _, _, _ -> + val metadataArray = defaultKotlinMoshi + .adapter>() + .toFormattedJson(serviceGraphMetadataProvider.get().graphVisual) + + div("p-4 sm:p-6 lg:p-8") { + + h1("text-3xl font-medium mb-8") { + +"""Service Graph""" + } + + div("svg-container") { } + + // JavaScript code in a block + script { + unsafe { + +""" + var metadata = $metadataArray; + + var linkColor = "steelblue" // Define a single color for all links + + drag = simulation => { + + function dragstarted(event, d) { + if (!event.active) simulation.alphaTarget(0.3).restart(); + d.fx = d.x; + d.fy = d.y; + } + + function dragged(event, d) { + d.fx = event.x; + d.fy = event.y; + } + + function dragended(event, d) { + if (!event.active) simulation.alphaTarget(0); + d.fx = null; + d.fy = null; + } + + return d3.drag() + .on("start", dragstarted) + .on("drag", dragged) + .on("end", dragended); + } + + function linkArc(d) { + const r = Math.hypot(d.target.x - d.source.x, d.target.y - d.source.y); + return "M" + d.source.x + "," + d.source.y + "A" + r + "," + r + " 0 0,1 " + d.target.x + "," + d.target.y; + } + + var width = 1600; + var height = 1000; + var margin = 500; + var nodes = Array.from(new Set(metadata.flatMap(l => [l.source, l.target])), id => ({id})); + var links = metadata.map(d => Object.create(d)) + + // Set initial positions for the nodes + nodes.forEach(node => { + node.x = Math.random() * width + margin; + node.y = Math.random() * height + margin; + }); + + var simulation = d3.forceSimulation(nodes) + .force("link", d3.forceLink(links).id(d => d.id).distance(200)) + .force("charge", d3.forceManyBody().strength(-2000)) + .force("x", d3.forceX().strength(0.1)) + .force("y", d3.forceY().strength(0.1)); + + var svg = d3.select(".svg-container").append("svg") + .attr("viewBox", [-margin, -margin, width, height]) + .attr("width", "100%") + .attr("height", "100%") + .attr("style", "max-width: 100%; height: auto; font: 18px sans-serif;"); + + // Defines arrows on the links + svg.append("defs").selectAll("marker") + .data(["link"]) + .join("marker") + .attr("id", d => "arrow-" + d) + .attr("viewBox", "0 -5 10 10") + .attr("refX", 15) + .attr("refY", -0.5) + .attr("markerWidth", 6) + .attr("markerHeight", 6) + .attr("orient", "auto") + .append("path") + .attr("fill", linkColor) + .attr("d", "M0,-5L10,0L0,5"); + + // Defines links between nodes + var link = svg.append("g") + .attr("fill", "none") + .attr("stroke-width", 2.5) + .selectAll("path") + .data(links) + .join("path") + .attr("stroke", linkColor) + .attr("marker-end", d => "url(#arrow-link)"); + + // Defines actual nodes + var node = svg.append("g") + .attr("fill", "currentColor") + .attr("stroke-linecap", "round") + .attr("stroke-linejoin", "round") + .selectAll("g") + .data(nodes) + .join("g") + .call(drag(simulation)); + + node.append("circle") + .attr("stroke", "white") + .attr("stroke-width", 1.5) + .attr("r", 4); + + node.append("text") + .attr("x", 12) + .attr("y", "0.31em") + .text(d => d.id) + .clone(true).lower() + .attr("fill", "none") + .attr("stroke", "white") + .attr("stroke-width", 3); + + simulation.on("tick", () => { + link.attr("d", linkArc); + node.attr("transform", d => "translate(" + d.x + "," + d.y + ")"); + }); + + Object.assign(svg.node(), {scales: {linkColor}}); + """.trimIndent() + } + } + } + } + + companion object { + const val PATH = "/_admin/service-graph/" + } +} diff --git a/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt b/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt index f1cdb83259c..7cacc662036 100644 --- a/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt +++ b/misk-admin/src/test/kotlin/misk/web/metadata/all/AllMetadataActionTest.kt @@ -6,7 +6,6 @@ import misk.testing.MiskTestModule import misk.web.metadata.Metadata import misk.web.metadata.MetadataTestingModule import misk.web.metadata.database.DatabaseQueryMetadata -import misk.web.metadata.jvm.JvmMetadata import misk.web.metadata.jvm.JvmRuntime import misk.web.metadata.webaction.WebActionMetadata import org.assertj.core.api.Assertions.assertThat @@ -39,7 +38,7 @@ class AllMetadataActionTest { val actualServiceGraphMetadata = actual.all["service-graph"]!! assertEquals( """ - |Metadata(serviceMap={Key[type=misk.web.jetty.JettyService, annotation=[none]]=Metadata(dependencies=[], directDependsOn=[misk.ReadyService]), Key[type=misk.web.jetty.JettyThreadPoolMetricsCollector, annotation=[none]]=Metadata(dependencies=[misk.ReadyService], directDependsOn=[]), Key[type=misk.web.jetty.JettyConnectionMetricsCollector, annotation=[none]]=Metadata(dependencies=[misk.ReadyService], directDependsOn=[]), Key[type=misk.web.actions.ReadinessCheckService, annotation=[none]]=Metadata(dependencies=[misk.ReadyService], directDependsOn=[]), Key[type=misk.tasks.RepeatedTaskQueue, annotation=@misk.web.ReadinessRefreshQueue]=Metadata(dependencies=[], directDependsOn=[]), Key[type=misk.ReadyService, annotation=[none]]=Metadata(dependencies=[misk.web.jetty.JettyService], directDependsOn=[misk.web.jetty.JettyThreadPoolMetricsCollector, misk.web.jetty.JettyConnectionMetricsCollector, misk.web.actions.ReadinessCheckService])}, serviceNames={Key[type=misk.web.jetty.JettyService, annotation=[none]]=misk.web.jetty.JettyService, Key[type=misk.web.jetty.JettyThreadPoolMetricsCollector, annotation=[none]]=misk.web.jetty.JettyThreadPoolMetricsCollector, Key[type=misk.web.jetty.JettyConnectionMetricsCollector, annotation=[none]]=misk.web.jetty.JettyConnectionMetricsCollector, Key[type=misk.web.actions.ReadinessCheckService, annotation=[none]]=misk.web.actions.ReadinessCheckService, Key[type=misk.tasks.RepeatedTaskQueue, annotation=@misk.web.ReadinessRefreshQueue]=misk.tasks.RepeatedTaskQueue, Key[type=misk.ReadyService, annotation=[none]]=misk.ReadyService}, dependencyMap={Key[type=misk.ReadyService, annotation=[none]]=[Key[type=misk.web.jetty.JettyService, annotation=[none]]], Key[type=misk.web.jetty.JettyThreadPoolMetricsCollector, annotation=[none]]=[Key[type=misk.ReadyService, annotation=[none]]], Key[type=misk.web.jetty.JettyConnectionMetricsCollector, annotation=[none]]=[Key[type=misk.ReadyService, annotation=[none]]], Key[type=misk.web.actions.ReadinessCheckService, annotation=[none]]=[Key[type=misk.ReadyService, annotation=[none]]]}, asciiVisual=misk.web.jetty.JettyService + |ServiceGraphBuilderMetadata(serviceMap={Key[type=misk.web.jetty.JettyService, annotation=[none]]=CoordinatedServiceMetadata(dependencies=[], directDependsOn=[misk.ReadyService]), Key[type=misk.web.jetty.JettyThreadPoolMetricsCollector, annotation=[none]]=CoordinatedServiceMetadata(dependencies=[misk.ReadyService], directDependsOn=[]), Key[type=misk.web.jetty.JettyConnectionMetricsCollector, annotation=[none]]=CoordinatedServiceMetadata(dependencies=[misk.ReadyService], directDependsOn=[]), Key[type=misk.web.actions.ReadinessCheckService, annotation=[none]]=CoordinatedServiceMetadata(dependencies=[misk.ReadyService], directDependsOn=[]), Key[type=misk.tasks.RepeatedTaskQueue, annotation=@misk.web.ReadinessRefreshQueue]=CoordinatedServiceMetadata(dependencies=[], directDependsOn=[]), Key[type=misk.ReadyService, annotation=[none]]=CoordinatedServiceMetadata(dependencies=[misk.web.jetty.JettyService], directDependsOn=[misk.web.jetty.JettyThreadPoolMetricsCollector, misk.web.jetty.JettyConnectionMetricsCollector, misk.web.actions.ReadinessCheckService])}, serviceNames={Key[type=misk.web.jetty.JettyService, annotation=[none]]=misk.web.jetty.JettyService, Key[type=misk.web.jetty.JettyThreadPoolMetricsCollector, annotation=[none]]=misk.web.jetty.JettyThreadPoolMetricsCollector, Key[type=misk.web.jetty.JettyConnectionMetricsCollector, annotation=[none]]=misk.web.jetty.JettyConnectionMetricsCollector, Key[type=misk.web.actions.ReadinessCheckService, annotation=[none]]=misk.web.actions.ReadinessCheckService, Key[type=misk.tasks.RepeatedTaskQueue, annotation=@misk.web.ReadinessRefreshQueue]=misk.tasks.RepeatedTaskQueue, Key[type=misk.ReadyService, annotation=[none]]=misk.ReadyService}, dependencyMap={Key[type=misk.ReadyService, annotation=[none]]=[Key[type=misk.web.jetty.JettyService, annotation=[none]]], Key[type=misk.web.jetty.JettyThreadPoolMetricsCollector, annotation=[none]]=[Key[type=misk.ReadyService, annotation=[none]]], Key[type=misk.web.jetty.JettyConnectionMetricsCollector, annotation=[none]]=[Key[type=misk.ReadyService, annotation=[none]]], Key[type=misk.web.actions.ReadinessCheckService, annotation=[none]]=[Key[type=misk.ReadyService, annotation=[none]]]}, asciiVisual=misk.web.jetty.JettyService | \__ misk.ReadyService | |__ misk.web.jetty.JettyThreadPoolMetricsCollector | |__ misk.web.jetty.JettyConnectionMetricsCollector diff --git a/misk-service/api/misk-service.api b/misk-service/api/misk-service.api index 8be18d02999..91b2a9ed086 100644 --- a/misk-service/api/misk-service.api +++ b/misk-service/api/misk-service.api @@ -1,3 +1,16 @@ +public final class misk/CoordinatedServiceMetadata { + public fun (Ljava/util/Set;Ljava/util/Set;)V + public final fun component1 ()Ljava/util/Set; + public final fun component2 ()Ljava/util/Set; + public final fun copy (Ljava/util/Set;Ljava/util/Set;)Lmisk/CoordinatedServiceMetadata; + public static synthetic fun copy$default (Lmisk/CoordinatedServiceMetadata;Ljava/util/Set;Ljava/util/Set;ILjava/lang/Object;)Lmisk/CoordinatedServiceMetadata; + public fun equals (Ljava/lang/Object;)Z + public final fun getDependencies ()Ljava/util/Set; + public final fun getDirectDependsOn ()Ljava/util/Set; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + public abstract interface class misk/DelegatingService : com/google/common/util/concurrent/Service { public abstract fun getService ()Lcom/google/common/util/concurrent/Service; } @@ -10,6 +23,23 @@ public final class misk/ReadyService : com/google/common/util/concurrent/Abstrac public final class misk/ReadyService$Companion { } +public final class misk/ServiceGraphBuilderMetadata { + public fun (Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/lang/String;)V + public final fun component1 ()Ljava/util/Map; + public final fun component2 ()Ljava/util/Map; + public final fun component3 ()Ljava/util/Map; + public final fun component4 ()Ljava/lang/String; + public final fun copy (Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/lang/String;)Lmisk/ServiceGraphBuilderMetadata; + public static synthetic fun copy$default (Lmisk/ServiceGraphBuilderMetadata;Ljava/util/Map;Ljava/util/Map;Ljava/util/Map;Ljava/lang/String;ILjava/lang/Object;)Lmisk/ServiceGraphBuilderMetadata; + public fun equals (Ljava/lang/Object;)Z + public final fun getAsciiVisual ()Ljava/lang/String; + public final fun getDependencyMap ()Ljava/util/Map; + public final fun getServiceMap ()Ljava/util/Map; + public final fun getServiceNames ()Ljava/util/Map; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + public final class misk/ServiceManagerConfig : wisp/config/Config { public fun ()V public fun (Z)V @@ -47,3 +77,28 @@ public final class misk/ServiceModule : misk/inject/KAbstractModule { public final fun getKey ()Lcom/google/inject/Key; } +public final class misk/metadata/servicegraph/ServiceGraphMetadata : misk/web/metadata/Metadata { + public fun (Lmisk/ServiceGraphBuilderMetadata;)V + public final fun component1 ()Lmisk/ServiceGraphBuilderMetadata; + public final fun copy (Lmisk/ServiceGraphBuilderMetadata;)Lmisk/metadata/servicegraph/ServiceGraphMetadata; + public static synthetic fun copy$default (Lmisk/metadata/servicegraph/ServiceGraphMetadata;Lmisk/ServiceGraphBuilderMetadata;ILjava/lang/Object;)Lmisk/metadata/servicegraph/ServiceGraphMetadata; + public fun equals (Ljava/lang/Object;)Z + public final fun getBuilderMetadata ()Lmisk/ServiceGraphBuilderMetadata; + public final fun getGraphVisual ()Ljava/util/List; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + +public final class misk/metadata/servicegraph/ServiceGraphMetadata$GraphPairs { + public fun (Ljava/lang/String;Ljava/lang/String;)V + public final fun component1 ()Ljava/lang/String; + public final fun component2 ()Ljava/lang/String; + public final fun copy (Ljava/lang/String;Ljava/lang/String;)Lmisk/metadata/servicegraph/ServiceGraphMetadata$GraphPairs; + public static synthetic fun copy$default (Lmisk/metadata/servicegraph/ServiceGraphMetadata$GraphPairs;Ljava/lang/String;Ljava/lang/String;ILjava/lang/Object;)Lmisk/metadata/servicegraph/ServiceGraphMetadata$GraphPairs; + public fun equals (Ljava/lang/Object;)Z + public final fun getSource ()Ljava/lang/String; + public final fun getTarget ()Ljava/lang/String; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + diff --git a/misk-service/build.gradle.kts b/misk-service/build.gradle.kts index 32ec5cdc6a4..45f2ee58209 100644 --- a/misk-service/build.gradle.kts +++ b/misk-service/build.gradle.kts @@ -10,12 +10,12 @@ dependencies { api(libs.guava) api(libs.guice) api(libs.jakartaInject) + api(project(":misk-config")) api(project(":misk-inject")) api(project(":wisp:wisp-config")) implementation(libs.kotlinLogging) implementation(libs.kotlinStdLibJdk8) implementation(libs.moshiCore) - implementation(project(":misk-config")) implementation(project(":wisp:wisp-logging")) implementation(project(":wisp:wisp-moshi")) diff --git a/misk-service/src/main/kotlin/misk/CoordinatedService.kt b/misk-service/src/main/kotlin/misk/CoordinatedService.kt index d353104daa9..09a7bde4a9d 100644 --- a/misk-service/src/main/kotlin/misk/CoordinatedService.kt +++ b/misk-service/src/main/kotlin/misk/CoordinatedService.kt @@ -8,6 +8,11 @@ import com.google.common.util.concurrent.Service.State import com.google.inject.Provider import java.util.concurrent.atomic.AtomicBoolean +data class CoordinatedServiceMetadata( + val dependencies: Set, + val directDependsOn: Set, +) + internal class CoordinatedService( private val serviceProvider: Provider ) : AbstractService(), DelegatingService { @@ -154,12 +159,7 @@ internal class CoordinatedService( } } - data class Metadata( - val dependencies: Set, - val directDependsOn: Set, - ) - - fun toMetadata() = Metadata( + fun toMetadata() = CoordinatedServiceMetadata( dependencies = dependencies.map { it.serviceProvider.get().javaClass.name }.toSet(), directDependsOn = directDependsOn.map { it.serviceProvider.get().javaClass.name }.toSet(), ) diff --git a/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt b/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt index c2418aee299..d06b2d9f0a4 100644 --- a/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt +++ b/misk-service/src/main/kotlin/misk/ServiceGraphBuilder.kt @@ -4,8 +4,16 @@ import com.google.common.collect.LinkedHashMultimap import com.google.common.util.concurrent.Service import com.google.common.util.concurrent.ServiceManager import com.google.inject.Key -import misk.CoordinatedService.Companion.CycleValidity import com.google.inject.Provider +import misk.CoordinatedService.Companion.CycleValidity + +data class ServiceGraphBuilderMetadata( + val serviceMap: Map, + val serviceNames: Map, + /** A map of downstream services -> their upstreams. */ + val dependencyMap: Map, + val asciiVisual: String, +) /** * Builds a graph of [CoordinatedService]s which defer start up and shut down until their dependent @@ -146,15 +154,7 @@ internal class ServiceGraphBuilder { append(serviceNames[key]) } - data class Metadata( - val serviceMap: Map, - val serviceNames: Map, - /** A map of downstream services -> their upstreams. */ - val dependencyMap: Map, - val asciiVisual: String, - ) - - fun toMetadata() = Metadata( + fun toMetadata() = ServiceGraphBuilderMetadata( serviceMap = serviceMap.map { it.key.toString() to it.value.toMetadata() }.toMap(), serviceNames = serviceNames.mapKeys { it.key.toString() }, dependencyMap = dependencyMap.asMap().map { (k,v) -> k.toString() to v.toString() }.toMap(), diff --git a/misk-service/src/main/kotlin/misk/ServiceManagerModule.kt b/misk-service/src/main/kotlin/misk/ServiceManagerModule.kt index 053b4b312e1..412008f36ca 100644 --- a/misk-service/src/main/kotlin/misk/ServiceManagerModule.kt +++ b/misk-service/src/main/kotlin/misk/ServiceManagerModule.kt @@ -38,6 +38,7 @@ class ServiceManagerModule @JvmOverloads constructor( newMultibinder() install(MetadataModule(ServiceGraphMetadataProvider())) + bind().toProvider(ServiceGraphMetadataProvider()) } @Provides diff --git a/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt b/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt index 7d5dfd05a69..3800b9125e2 100644 --- a/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt +++ b/misk-service/src/main/kotlin/misk/metadata/servicegraph/ServiceGraphMetadata.kt @@ -1,23 +1,57 @@ package misk.metadata.servicegraph -import jakarta.inject.Inject import com.google.inject.Provider -import misk.ServiceGraphBuilder +import jakarta.inject.Inject +import misk.ServiceGraphBuilderMetadata import misk.web.metadata.Metadata import misk.web.metadata.MetadataProvider import misk.web.metadata.toFormattedJson import wisp.moshi.adapter import wisp.moshi.defaultKotlinMoshi -internal data class ServiceGraphMetadata( - val builderMetadata: ServiceGraphBuilder.Metadata, +data class ServiceGraphMetadata( + val builderMetadata: ServiceGraphBuilderMetadata, ) : Metadata( metadata = builderMetadata, prettyPrint = "Service Graph Ascii Visual\n\n${builderMetadata.asciiVisual}\n\nMetadata\n\n" + defaultKotlinMoshi - .adapter() + .adapter() .toFormattedJson(builderMetadata), descriptionString = "Guava service graph metadata, including a ASCII art visualization for easier debugging." -) +) { + val graphVisual = generateGraphVisual() + + data class GraphPairs( + val source: String, + val target: String, + ) + + private fun extractType(input: String): String { + // Find the start index of the type + val typeStartIndex = input.indexOf("type=") + if (typeStartIndex == -1) return input + + // Find the end index of the type + val typeEndIndex = input.indexOf(",", typeStartIndex) + if (typeEndIndex == -1) return input + + // Extract and return the type substring + return input.substring(typeStartIndex + "type=".length, typeEndIndex) + } + + private fun generateGraphVisual(): MutableList { + val output = mutableListOf() + + builderMetadata.serviceMap.forEach { (key, value) -> + val dependencies = value.dependencies + + dependencies.forEach { target -> + output.add(GraphPairs(extractType(key), extractType(target))) + } + } + + return output + } +} internal class ServiceGraphMetadataProvider : MetadataProvider { override val id: String = "service-graph" @@ -27,7 +61,7 @@ internal class ServiceGraphMetadataProvider : MetadataProvider + @Inject internal lateinit var metadataProvider: Provider override fun get() = ServiceGraphMetadata( builderMetadata = metadataProvider.get() From ec5fdd743e56a021abe27b0f344c536ec43664a5 Mon Sep 17 00:00:00 2001 From: Terry Yiu <963907+tyiu@users.noreply.github.com> Date: Wed, 7 Aug 2024 18:58:04 -0400 Subject: [PATCH 08/26] Fix InProcessDynamoDbTests for Apple Silicon GitOrigin-RevId: 294c578de959770de4a3b1de8d99c5b122b85984 --- misk-aws-dynamodb/build.gradle.kts | 7 +++++++ misk-aws2-dynamodb/build.gradle.kts | 7 +++++++ misk-clustering-dynamodb/build.gradle.kts | 5 +++-- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/misk-aws-dynamodb/build.gradle.kts b/misk-aws-dynamodb/build.gradle.kts index b58281bef3d..f82a1e90e08 100644 --- a/misk-aws-dynamodb/build.gradle.kts +++ b/misk-aws-dynamodb/build.gradle.kts @@ -35,6 +35,13 @@ dependencies { testImplementation(libs.assertj) testImplementation(libs.junitApi) + + if (org.apache.tools.ant.taskdefs.condition.Os.isArch("aarch64")) { + // Without this, we can't compile on Apple Silicon currently. + // This is likely not necessary to have long term, + // so we should remove it when things get fixed upstream. + testImplementation("io.github.ganadist.sqlite4java:libsqlite4java-osx-aarch64:1.0.392") + } } mavenPublishing { diff --git a/misk-aws2-dynamodb/build.gradle.kts b/misk-aws2-dynamodb/build.gradle.kts index 598e5b0f32b..d8a0245d735 100644 --- a/misk-aws2-dynamodb/build.gradle.kts +++ b/misk-aws2-dynamodb/build.gradle.kts @@ -44,6 +44,13 @@ dependencies { strictly("4.9.3") } } + + if (org.apache.tools.ant.taskdefs.condition.Os.isArch("aarch64")) { + // Without this, we can't compile on Apple Silicon currently. + // This is likely not necessary to have long term, + // so we should remove it when things get fixed upstream. + testImplementation("io.github.ganadist.sqlite4java:libsqlite4java-osx-aarch64:1.0.392") + } } mavenPublishing { diff --git a/misk-clustering-dynamodb/build.gradle.kts b/misk-clustering-dynamodb/build.gradle.kts index db59dac5fc0..e41073ecaeb 100644 --- a/misk-clustering-dynamodb/build.gradle.kts +++ b/misk-clustering-dynamodb/build.gradle.kts @@ -27,8 +27,9 @@ dependencies { testImplementation(testFixtures(project(":misk-aws2-dynamodb"))) if (org.apache.tools.ant.taskdefs.condition.Os.isArch("aarch64")) { - // Without this, we can't compile on Apple Silicon currently. This is likely not necessary to - // have longterm, so we should remove it when platform fixes things across Square. + // Without this, we can't compile on Apple Silicon currently. + // This is likely not necessary to have long term, + // so we should remove it when things get fixed upstream. testImplementation("io.github.ganadist.sqlite4java:libsqlite4java-osx-aarch64:1.0.392") } From bd0bb911032c5579d09ccdb0eb0fc30ade6fa051 Mon Sep 17 00:00:00 2001 From: Evan Nelson Date: Wed, 7 Aug 2024 17:00:21 -0700 Subject: [PATCH 09/26] Update misk.feature.Attributes to have a `with` method that returns a misk class rather than inheriting one that returns a wisp class. Previously, if someone with a misk.feature.Attribute called `.with`, they would get back a wisp class which would be incompatible with the rest of the misk.feature.* classes they were using. GitOrigin-RevId: 7c5181263816f14e27fdaf36b4287840119d5ce4 --- misk-feature/api/misk-feature.api | 6 ++++++ .../src/main/kotlin/misk/feature/Feature.kt | 16 +++++++++++++++- wisp/wisp-feature/api/wisp-feature.api | 6 +++--- .../src/main/kotlin/wisp/feature/FeatureFlags.kt | 6 +++--- 4 files changed, 27 insertions(+), 7 deletions(-) diff --git a/misk-feature/api/misk-feature.api b/misk-feature/api/misk-feature.api index bcf99068c8b..f92c87f2b62 100644 --- a/misk-feature/api/misk-feature.api +++ b/misk-feature/api/misk-feature.api @@ -4,6 +4,12 @@ public final class misk/feature/Attributes : wisp/feature/Attributes { public fun (Ljava/util/Map;Ljava/util/Map;)V public fun (Ljava/util/Map;Ljava/util/Map;Z)V public synthetic fun (Ljava/util/Map;Ljava/util/Map;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun copy (Ljava/util/Map;Ljava/util/Map;Z)Lmisk/feature/Attributes; + public synthetic fun copy (Ljava/util/Map;Ljava/util/Map;Z)Lwisp/feature/Attributes; + public fun with (Ljava/lang/String;Ljava/lang/Number;)Lmisk/feature/Attributes; + public synthetic fun with (Ljava/lang/String;Ljava/lang/Number;)Lwisp/feature/Attributes; + public fun with (Ljava/lang/String;Ljava/lang/String;)Lmisk/feature/Attributes; + public synthetic fun with (Ljava/lang/String;Ljava/lang/String;)Lwisp/feature/Attributes; } public abstract interface class misk/feature/DynamicConfig { diff --git a/misk-feature/src/main/kotlin/misk/feature/Feature.kt b/misk-feature/src/main/kotlin/misk/feature/Feature.kt index dc940c40f7c..4a7eb3fe1a2 100644 --- a/misk-feature/src/main/kotlin/misk/feature/Feature.kt +++ b/misk-feature/src/main/kotlin/misk/feature/Feature.kt @@ -11,4 +11,18 @@ class Attributes @JvmOverloads constructor( // Indicates that the user is anonymous, which may have backend-specific behavior, like not // including the user in analytics. anonymous: Boolean = false -) : wisp.feature.Attributes(text, number, anonymous) +) : wisp.feature.Attributes(text, number, anonymous) { + override fun with(name: String, value: String): Attributes = + copy(text = text.plus(name to value)) + + override fun with(name: String, value: Number): Attributes { + val number = number ?: mapOf() + return copy(number = number.plus(name to value)) + } + + override fun copy( + text: Map, + number: Map?, + anonymous: Boolean + ): Attributes = Attributes(text, number, anonymous) +} diff --git a/wisp/wisp-feature/api/wisp-feature.api b/wisp/wisp-feature/api/wisp-feature.api index 3333b9bee83..14e996483c3 100644 --- a/wisp/wisp-feature/api/wisp-feature.api +++ b/wisp/wisp-feature/api/wisp-feature.api @@ -7,7 +7,7 @@ public class wisp/feature/Attributes { public final fun copy ()Lwisp/feature/Attributes; public final fun copy (Ljava/util/Map;)Lwisp/feature/Attributes; public final fun copy (Ljava/util/Map;Ljava/util/Map;)Lwisp/feature/Attributes; - public final fun copy (Ljava/util/Map;Ljava/util/Map;Z)Lwisp/feature/Attributes; + public fun copy (Ljava/util/Map;Ljava/util/Map;Z)Lwisp/feature/Attributes; public static synthetic fun copy$default (Lwisp/feature/Attributes;Ljava/util/Map;Ljava/util/Map;ZILjava/lang/Object;)Lwisp/feature/Attributes; public fun equals (Ljava/lang/Object;)Z public final fun getAnonymous ()Z @@ -15,8 +15,8 @@ public class wisp/feature/Attributes { public final fun getText ()Ljava/util/Map; public fun hashCode ()I public fun toString ()Ljava/lang/String; - public final fun with (Ljava/lang/String;Ljava/lang/Number;)Lwisp/feature/Attributes; - public final fun with (Ljava/lang/String;Ljava/lang/String;)Lwisp/feature/Attributes; + public fun with (Ljava/lang/String;Ljava/lang/Number;)Lwisp/feature/Attributes; + public fun with (Ljava/lang/String;Ljava/lang/String;)Lwisp/feature/Attributes; } public abstract interface class wisp/feature/BooleanFeatureFlag : wisp/feature/FeatureFlag { diff --git a/wisp/wisp-feature/src/main/kotlin/wisp/feature/FeatureFlags.kt b/wisp/wisp-feature/src/main/kotlin/wisp/feature/FeatureFlags.kt index dcc6dbe0ed9..c3b5157c955 100644 --- a/wisp/wisp-feature/src/main/kotlin/wisp/feature/FeatureFlags.kt +++ b/wisp/wisp-feature/src/main/kotlin/wisp/feature/FeatureFlags.kt @@ -372,16 +372,16 @@ open class Attributes @JvmOverloads constructor( // including the user in analytics. val anonymous: Boolean = false ) { - fun with(name: String, value: String): Attributes = + open fun with(name: String, value: String): Attributes = copy(text = text.plus(name to value)) - fun with(name: String, value: Number): Attributes { + open fun with(name: String, value: Number): Attributes { val number = number ?: mapOf() return copy(number = number.plus(name to value)) } @JvmOverloads - fun copy( + open fun copy( text: Map = this.text, number: Map? = this.number, anonymous: Boolean = this.anonymous From 6c4a9be19f48eabfeb827f3776e71515611bab8f Mon Sep 17 00:00:00 2001 From: Kevin Kim <95660882+kevink-sq@users.noreply.github.com> Date: Fri, 9 Aug 2024 06:23:42 +0900 Subject: [PATCH 10/26] Defer the shutdown of JettyService until after Guava managed services are shutdown by default GitOrigin-RevId: bce36d20af9ef0221f5179e4c7382854e3d7e04d --- .../kotlin/misk/testing/MiskTestExtension.kt | 18 ++++++ misk/api/misk.api | 1 + misk/src/main/kotlin/misk/web/WebConfig.kt | 6 +- .../misk/web/actions/LivenessCheckAction.kt | 3 +- .../kotlin/misk/web/jetty/JettyService.kt | 6 +- .../test/kotlin/misk/web/JettyShutdownTest.kt | 3 +- .../web/actions/LivenessCheckActionTest.kt | 59 ++++++++++++++----- 7 files changed, 76 insertions(+), 20 deletions(-) diff --git a/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt b/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt index 471172b73b8..596cc04e7cd 100644 --- a/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt +++ b/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt @@ -12,6 +12,7 @@ import misk.inject.KAbstractModule import misk.inject.ReusableTestModule import misk.inject.getInstance import misk.inject.uninject +import misk.web.jetty.JettyService import org.junit.jupiter.api.extension.AfterEachCallback import org.junit.jupiter.api.extension.BeforeEachCallback import org.junit.jupiter.api.extension.ExtensionContext @@ -124,11 +125,28 @@ internal class MiskTestExtension : BeforeEachCallback, AfterEachCallback { lateinit var serviceManager: ServiceManager override fun afterEach(context: ExtensionContext) { + if (context.startService()) { serviceManager.stopAsync() serviceManager.awaitStopped(30, TimeUnit.SECONDS) + + /** + * By default, Jetty shutdown is not managed by Guava so needs to be + * done explicitly. This is being done in MiskApplication. + */ + if (jettyIsRunning()) { + context.retrieve("injector").getInstance().stop() + } } } + + private fun jettyIsRunning() : Boolean { + return serviceManager + .servicesByState() + .values() + .toList() + .any { it.toString().startsWith("JettyService") } + } } /** We inject after starting services and uninject after stopping services. */ diff --git a/misk/api/misk.api b/misk/api/misk.api index 4655c15570b..21db3c4c4bd 100644 --- a/misk/api/misk.api +++ b/misk/api/misk.api @@ -1943,6 +1943,7 @@ public final class misk/web/jetty/JettyService : com/google/common/util/concurre public final fun getHealthServerUrl ()Lokhttp3/HttpUrl; public final fun getHttpServerUrl ()Lokhttp3/HttpUrl; public final fun getHttpsServerUrl ()Lokhttp3/HttpUrl; + public final fun stop ()V } public final class misk/web/jetty/JettyService$Companion { diff --git a/misk/src/main/kotlin/misk/web/WebConfig.kt b/misk/src/main/kotlin/misk/web/WebConfig.kt index bfd6a99999e..99b3936da81 100644 --- a/misk/src/main/kotlin/misk/web/WebConfig.kt +++ b/misk/src/main/kotlin/misk/web/WebConfig.kt @@ -105,7 +105,11 @@ data class WebConfig @JvmOverloads constructor( log_level = concurrency_limiter_log_level, ), - /** The number of milliseconds to sleep before commencing service shutdown. */ + /** + * The number of milliseconds to sleep before commencing service shutdown. 0 or + * greater will defer the actual shutdown of Jetty to after all Guava managed + * services are shutdown. + * */ val shutdown_sleep_ms: Int = 0, /** The maximum allowed size in bytes for the HTTP request line and HTTP request headers. */ diff --git a/misk/src/main/kotlin/misk/web/actions/LivenessCheckAction.kt b/misk/src/main/kotlin/misk/web/actions/LivenessCheckAction.kt index 4867248f307..7138832d2d7 100644 --- a/misk/src/main/kotlin/misk/web/actions/LivenessCheckAction.kt +++ b/misk/src/main/kotlin/misk/web/actions/LivenessCheckAction.kt @@ -24,8 +24,7 @@ class LivenessCheckAction @Inject internal constructor( @AvailableWhenDegraded fun livenessCheck(): Response { val serviceManager = serviceManagerProvider.get() - val failedServices = serviceManager.servicesByState().get(State.FAILED) + - serviceManager.servicesByState().get(State.TERMINATED) + val failedServices = serviceManager.servicesByState().get(State.FAILED) if (failedServices.isEmpty()) { return Response("", statusCode = 200) diff --git a/misk/src/main/kotlin/misk/web/jetty/JettyService.kt b/misk/src/main/kotlin/misk/web/jetty/JettyService.kt index 2a18960fd7a..9ca4305c065 100644 --- a/misk/src/main/kotlin/misk/web/jetty/JettyService.kt +++ b/misk/src/main/kotlin/misk/web/jetty/JettyService.kt @@ -368,7 +368,7 @@ class JettyService @Inject internal constructor( } } - internal fun stop() { + fun stop() { if (server.isRunning) { val stopwatch = Stopwatch.createStarted() logger.info("Stopping Jetty") @@ -400,7 +400,9 @@ class JettyService @Inject internal constructor( // // Ideally we could call jetty.awaitInflightRequests() but that's not available // for us. - if (webConfig.shutdown_sleep_ms > 0) { + // + // Default is to shutdown jetty after all guava managed services are shutdown. + if (webConfig.shutdown_sleep_ms >= 0) { sleep(webConfig.shutdown_sleep_ms.toLong()) } else { stop() diff --git a/misk/src/test/kotlin/misk/web/JettyShutdownTest.kt b/misk/src/test/kotlin/misk/web/JettyShutdownTest.kt index 5995dfa3866..6211a57f1e5 100644 --- a/misk/src/test/kotlin/misk/web/JettyShutdownTest.kt +++ b/misk/src/test/kotlin/misk/web/JettyShutdownTest.kt @@ -1,5 +1,6 @@ package misk.web +import jakarta.inject.Inject import misk.MiskTestingServiceModule import misk.inject.KAbstractModule import misk.testing.MiskTest @@ -13,7 +14,6 @@ import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.Test import java.util.concurrent.TimeUnit import java.util.concurrent.TimeoutException -import jakarta.inject.Inject @MiskTest internal class ZeroIdleTimeoutTest : AbstractJettyShutdownTest() { @@ -80,6 +80,7 @@ internal abstract class AbstractJettyShutdownTest { jetty.stopAsync() try { + jetty.stop() jetty.awaitTerminated(timeoutMs, TimeUnit.MILLISECONDS) } catch (e: TimeoutException) { assertThat(timeoutExpected).isTrue() diff --git a/misk/src/test/kotlin/misk/web/actions/LivenessCheckActionTest.kt b/misk/src/test/kotlin/misk/web/actions/LivenessCheckActionTest.kt index f39463ffa9e..bc37624f9b6 100644 --- a/misk/src/test/kotlin/misk/web/actions/LivenessCheckActionTest.kt +++ b/misk/src/test/kotlin/misk/web/actions/LivenessCheckActionTest.kt @@ -2,34 +2,65 @@ package misk.web.actions import com.google.common.util.concurrent.ServiceManager import com.google.inject.util.Modules +import jakarta.inject.Inject import misk.MiskTestingServiceModule -import misk.services.FakeServiceModule +import misk.ReadyService +import misk.ServiceModule +import misk.inject.KAbstractModule import misk.testing.MiskTest import misk.testing.MiskTestModule +import misk.time.ClockModule +import misk.web.FakeService import misk.web.WebActionModule +import misk.web.WebServerTestingModule +import misk.web.jetty.JettyService +import okhttp3.HttpUrl +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.Response import org.assertj.core.api.Assertions.assertThat +import org.assertj.core.api.Assertions.assertThatThrownBy import org.junit.jupiter.api.Test -import jakarta.inject.Inject +import java.time.Duration -@MiskTest +@MiskTest(startService = true) class LivenessCheckActionTest { @MiskTestModule - val module = Modules.combine( - MiskTestingServiceModule(), - FakeServiceModule(), - WebActionModule.create() - ) + val module = TestModule() + class TestModule : KAbstractModule() { + override fun configure() { + install(Modules.override(MiskTestingServiceModule()).with(ClockModule())) + install(WebServerTestingModule()) + install(WebActionModule.create()) + install(ServiceModule().enhancedBy()) + } + } - @Inject lateinit var livenessCheckAction: LivenessCheckAction + @Inject lateinit var jettyService: JettyService @Inject lateinit var serviceManager: ServiceManager + private val client = OkHttpClient().newBuilder() + .readTimeout(Duration.ofSeconds(30)) + .connectTimeout(Duration.ofSeconds(30)) + .writeTimeout(Duration.ofSeconds(30)) + .build() + + /** + * Liveness should only fail when Jetty is shut down. + */ @Test - fun livenessDependsOnServiceState() { - serviceManager.startAsync() - serviceManager.awaitHealthy() - assertThat(livenessCheckAction.livenessCheck().statusCode).isEqualTo(200) + fun liveness() { + val url = jettyService.httpServerUrl.resolve("/_liveness")!! + assertThat(get(url).code).isEqualTo(200) serviceManager.stopAsync() serviceManager.awaitStopped() - assertThat(livenessCheckAction.livenessCheck().statusCode).isEqualTo(503) + assertThat(get(url).code).isEqualTo(200) + jettyService.stop() + assertThatThrownBy { get(url)} + } + + private fun get(url: HttpUrl) : Response{ + val req = Request.Builder().url(url).build(); + return client.newCall(req).execute(); } } From 8c03cf9568313144a08a570821ccc1931718920f Mon Sep 17 00:00:00 2001 From: Raine <135167440+rainecp@users.noreply.github.com> Date: Fri, 9 Aug 2024 09:25:57 -0700 Subject: [PATCH 11/26] feat: Add new param to Redis modules for specific config Currently, the [RedisModule](https://github.com/cashapp/misk/blob/8711bf7a6f68ed7b31237d37d811649d958486f2/misk-redis/src/main/kotlin/misk/redis/RedisModule.kt#L74) in Misk uses a [RedisConfig](https://github.com/cashapp/misk/blob/bdb3d56f8a418fa630433a6a8e374405c5a21f1e/misk-redis/src/main/kotlin/misk/redis/RedisConfig.kt#L7) map where the Jedis client is initialized using the first item in the map. The map is ordered by insertion which is based on the order of how configurations are loaded. This setup can lead to unintended consequences, particularly when multiple Redis clusters are provisioned for a single environment. This adds a new constructor in `RedisModule` and `RedisClusterModule` to improve config management by allowing consumers to provide a specific config to use instead of letting the module choose from a given configuration map. GitOrigin-RevId: 5175feb11d51f78f77b723dee9943741bf59b8fd --- .../bucket4j/redis/RedisRateLimiterTest.kt | 2 +- misk-redis/api/misk-redis.api | 14 ++++++- .../kotlin/misk/redis/RedisClusterModule.kt | 28 +++++++++----- .../src/main/kotlin/misk/redis/RedisModule.kt | 38 +++++++++++++------ .../misk/redis/PipelinedRedisClusterTest.kt | 2 +- .../kotlin/misk/redis/PipelinedRedisTest.kt | 3 +- .../kotlin/misk/redis/RealRedisClusterTest.kt | 10 +---- .../test/kotlin/misk/redis/RealRedisTest.kt | 2 +- .../misk/redis/RedisAuthPasswordEnvTest.kt | 6 +-- .../misk/redis/RedisClientMetricsTest.kt | 2 +- .../kotlin/misk/redis/testing/DockerRedis.kt | 6 +-- .../misk/redis/testing/DockerRedisCluster.kt | 6 +-- .../exemplar/RedisRateLimitedActionTests.kt | 2 +- .../kotlin/com/squareup/chat/ChatService.kt | 2 +- .../squareup/chat/ChatWebSocketActionTest.kt | 2 +- 15 files changed, 76 insertions(+), 49 deletions(-) diff --git a/misk-rate-limiting-bucket4j-redis/src/test/kotlin/misk/ratelimiting/bucket4j/redis/RedisRateLimiterTest.kt b/misk-rate-limiting-bucket4j-redis/src/test/kotlin/misk/ratelimiting/bucket4j/redis/RedisRateLimiterTest.kt index 9636a459a5a..b7a5438d2bb 100644 --- a/misk-rate-limiting-bucket4j-redis/src/test/kotlin/misk/ratelimiting/bucket4j/redis/RedisRateLimiterTest.kt +++ b/misk-rate-limiting-bucket4j-redis/src/test/kotlin/misk/ratelimiting/bucket4j/redis/RedisRateLimiterTest.kt @@ -26,7 +26,7 @@ class RedisRateLimiterTest { @MiskTestModule private val module: Module = object : KAbstractModule() { override fun configure() { - install(RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) install(RedisBucket4jRateLimiterModule()) install(MiskTestingServiceModule()) install(RedisTestFlushModule()) diff --git a/misk-redis/api/misk-redis.api b/misk-redis/api/misk-redis.api index a02af877f18..0eeafcd319a 100644 --- a/misk-redis/api/misk-redis.api +++ b/misk-redis/api/misk-redis.api @@ -379,9 +379,11 @@ public final class misk/redis/RedisClusterConfig : java/util/LinkedHashMap, wisp } public final class misk/redis/RedisClusterModule : misk/inject/KAbstractModule { - public fun (Lmisk/redis/RedisClusterConfig;Lredis/clients/jedis/ConnectionPoolConfig;)V public fun (Lmisk/redis/RedisClusterConfig;Lredis/clients/jedis/ConnectionPoolConfig;Z)V public synthetic fun (Lmisk/redis/RedisClusterConfig;Lredis/clients/jedis/ConnectionPoolConfig;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lmisk/redis/RedisClusterReplicationGroupConfig;Lredis/clients/jedis/ConnectionPoolConfig;)V + public fun (Lmisk/redis/RedisClusterReplicationGroupConfig;Lredis/clients/jedis/ConnectionPoolConfig;Z)V + public synthetic fun (Lmisk/redis/RedisClusterReplicationGroupConfig;Lredis/clients/jedis/ConnectionPoolConfig;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V } public final class misk/redis/RedisClusterReplicationGroupConfig { @@ -444,11 +446,17 @@ public final class misk/redis/RedisKt { } public final class misk/redis/RedisModule : misk/inject/KAbstractModule { - public fun (Lmisk/redis/RedisConfig;Lredis/clients/jedis/ConnectionPoolConfig;)V + public static final field Companion Lmisk/redis/RedisModule$Companion; public fun (Lmisk/redis/RedisConfig;Lredis/clients/jedis/ConnectionPoolConfig;Z)V public synthetic fun (Lmisk/redis/RedisConfig;Lredis/clients/jedis/ConnectionPoolConfig;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V public fun (Lmisk/redis/RedisConfig;Lredis/clients/jedis/JedisPoolConfig;Z)V public synthetic fun (Lmisk/redis/RedisConfig;Lredis/clients/jedis/JedisPoolConfig;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lmisk/redis/RedisReplicationGroupConfig;Lredis/clients/jedis/ConnectionPoolConfig;)V + public fun (Lmisk/redis/RedisReplicationGroupConfig;Lredis/clients/jedis/ConnectionPoolConfig;Z)V + public synthetic fun (Lmisk/redis/RedisReplicationGroupConfig;Lredis/clients/jedis/ConnectionPoolConfig;ZILkotlin/jvm/internal/DefaultConstructorMarker;)V +} + +public final class misk/redis/RedisModule$Companion { } public final class misk/redis/RedisNodeConfig { @@ -498,6 +506,7 @@ public final class misk/redis/testing/DockerRedis : misk/testing/ExternalDepende public fun beforeEach ()V public final fun getConfig ()Lmisk/redis/RedisConfig; public fun getId ()Ljava/lang/String; + public final fun getReplicationGroupConfig ()Lmisk/redis/RedisReplicationGroupConfig; public fun shutdown ()V public fun startup ()V } @@ -508,6 +517,7 @@ public final class misk/redis/testing/DockerRedisCluster : misk/testing/External public fun beforeEach ()V public final fun getConfig ()Lmisk/redis/RedisClusterConfig; public fun getId ()Ljava/lang/String; + public final fun getReplicationGroupConfig ()Lmisk/redis/RedisClusterReplicationGroupConfig; public fun shutdown ()V public fun startup ()V } diff --git a/misk-redis/src/main/kotlin/misk/redis/RedisClusterModule.kt b/misk-redis/src/main/kotlin/misk/redis/RedisClusterModule.kt index cc486b27bf2..bcae329ac3e 100644 --- a/misk-redis/src/main/kotlin/misk/redis/RedisClusterModule.kt +++ b/misk-redis/src/main/kotlin/misk/redis/RedisClusterModule.kt @@ -31,9 +31,9 @@ import wisp.deployment.Deployment * ``` * * - * [redisClusterConfig]: Only one replication group config is supported; this module will use the first - * configuration it finds. An empty [RedisReplicationGroupConfig.redis_auth_password] is only - * permitted in fake environments. See [Deployment]. + * [redisClusterGroupConfig]: Only one replication group config is supported. + * An empty [RedisReplicationGroupConfig.redis_auth_password] is only permitted in fake + * environments. See [Deployment]. * * This initiates a [JedisCluster] which automatically discovers the topology of the Redis cluster, * and routes commands to the appropriate node based on the hash slot of the key. @@ -47,13 +47,26 @@ import wisp.deployment.Deployment * https://redis.com/blog/redis-clustering-best-practices-with-keys/ */ class RedisClusterModule @JvmOverloads constructor( - private val redisClusterConfig: RedisClusterConfig, + private val redisClusterGroupConfig: RedisClusterReplicationGroupConfig, private val connectionPoolConfig: ConnectionPoolConfig, private val useSsl: Boolean = true ) : KAbstractModule() { + @Deprecated("Please use RedisClusterReplicationGroupConfig to pass specific redis cluster configuration.") + constructor( + redisClusterConfig: RedisClusterConfig, + connectionPoolConfig: ConnectionPoolConfig, + useSsl: Boolean = true, + ) : this( + // Get the first replication group, we only support 1 replication group per service. + redisClusterConfig.values.firstOrNull() + ?: throw RuntimeException("At least 1 replication group must be specified"), + connectionPoolConfig = connectionPoolConfig, + useSsl = useSsl, + ) + override fun configure() { - bind().toInstance(redisClusterConfig) + bind().toInstance(redisClusterGroupConfig) install(ServiceModule().enhancedBy()) requireBinding() } @@ -66,12 +79,9 @@ class RedisClusterModule @JvmOverloads constructor( @Provides @Singleton internal fun provideUnifiedJedis( - config: RedisClusterConfig, + replicationGroup: RedisClusterReplicationGroupConfig, deployment: Deployment ): UnifiedJedis { - // Get the first replication group, we only support 1 replication group per service. - val replicationGroup = config[config.keys.first()] - ?: throw RuntimeException("At least 1 replication group must be specified") // Create our jedis pool with client-side metrics. val jedisClientConfig = DefaultJedisClientConfig.builder() diff --git a/misk-redis/src/main/kotlin/misk/redis/RedisModule.kt b/misk-redis/src/main/kotlin/misk/redis/RedisModule.kt index 1f59955f60f..29bf8727963 100644 --- a/misk-redis/src/main/kotlin/misk/redis/RedisModule.kt +++ b/misk-redis/src/main/kotlin/misk/redis/RedisModule.kt @@ -25,9 +25,9 @@ import wisp.deployment.Deployment * * You must pass in configuration for your Redis client. * - * [redisConfig]: Only one replication group config is supported; this module will use the first - * configuration it finds. An empty [RedisReplicationGroupConfig.redis_auth_password] is only - * permitted in fake environments. See [Deployment]. + * [redisReplicationGroupConfig]: Only one replication group config is supported. + * An empty [RedisReplicationGroupConfig.redis_auth_password] is only permitted in fake + * environments. See [Deployment]. * * [connectionPoolConfig]: Misk-redis is backed by a [JedisPooled], you may not want to use the * [ConnectionPoolConfig] defaults! Be sure to understand them! @@ -35,7 +35,7 @@ import wisp.deployment.Deployment * See: https://github.com/xetorthio/jedis/wiki/Getting-started#using-jedis-in-a-multithreaded-environment */ class RedisModule @JvmOverloads constructor( - private val redisConfig: RedisConfig, + private val redisReplicationGroupConfig: RedisReplicationGroupConfig, private val connectionPoolConfig: ConnectionPoolConfig, private val useSsl: Boolean = true, ) : KAbstractModule() { @@ -46,13 +46,24 @@ class RedisModule @JvmOverloads constructor( jedisPoolConfig: JedisPoolConfig, useSsl: Boolean = true, ) : this( - redisConfig, + getFirstReplicationGroup(redisConfig), connectionPoolConfig = jedisPoolConfig.toConnectionPoolConfig(), useSsl = useSsl ) + @Deprecated("Please use RedisReplicationGroupConfig to pass specific redis cluster configuration.") + constructor( + redisConfig: RedisConfig, + connectionPoolConfig: ConnectionPoolConfig, + useSsl: Boolean = true, + ) : this( + getFirstReplicationGroup(redisConfig), + connectionPoolConfig = connectionPoolConfig, + useSsl = useSsl, + ) + override fun configure() { - bind().toInstance(redisConfig) + bind().toInstance(redisReplicationGroupConfig) install(ServiceModule().enhancedBy()) requireBinding() } @@ -66,21 +77,26 @@ class RedisModule @JvmOverloads constructor( @Provides @Singleton internal fun provideUnifiedJedis( clientMetrics: RedisClientMetrics, - config: RedisConfig, + redisReplicationGroupConfig: RedisReplicationGroupConfig, deployment: Deployment, ): UnifiedJedis { - // Get the first replication group, we only support 1 replication group per service. - val replicationGroup = config[config.keys.first()] - ?: throw RuntimeException("At least 1 replication group must be specified") return JedisPooledWithMetrics( metrics = clientMetrics, poolConfig = connectionPoolConfig, - replicationGroupConfig = replicationGroup, + replicationGroupConfig = redisReplicationGroupConfig, ssl = useSsl, requiresPassword = deployment.isReal ) } + + companion object { + private fun getFirstReplicationGroup(redisConfig: RedisConfig): RedisReplicationGroupConfig { + // Get the first replication group, we only support 1 replication group per service. + return redisConfig.values.firstOrNull() + ?: throw RuntimeException("At least 1 replication group must be specified") + } + } } private fun JedisPoolConfig.toConnectionPoolConfig() = ConnectionPoolConfig().apply { diff --git a/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisClusterTest.kt b/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisClusterTest.kt index 98b67b2705f..48205519f3a 100644 --- a/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisClusterTest.kt +++ b/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisClusterTest.kt @@ -23,7 +23,7 @@ class PipelinedRedisClusterTest : AbstractRedisTest() { @MiskTestModule private val module: Module = object : KAbstractModule() { override fun configure() { - install(RedisClusterModule(DockerRedisCluster.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisClusterModule(DockerRedisCluster.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) install(MiskTestingServiceModule()) install(DeploymentModule(TESTING)) diff --git a/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisTest.kt b/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisTest.kt index c7d6aedade3..d15353702c7 100644 --- a/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisTest.kt +++ b/misk-redis/src/test/kotlin/misk/redis/PipelinedRedisTest.kt @@ -1,7 +1,6 @@ package misk.redis import com.google.inject.Module -import com.google.inject.Provider import jakarta.inject.Inject import misk.MiskTestingServiceModule import misk.environment.DeploymentModule @@ -23,7 +22,7 @@ class PipelinedRedisTest : AbstractRedisTest() { @MiskTestModule private val module: Module = object : KAbstractModule() { override fun configure() { - install(RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) install(MiskTestingServiceModule()) install(DeploymentModule(TESTING)) diff --git a/misk-redis/src/test/kotlin/misk/redis/RealRedisClusterTest.kt b/misk-redis/src/test/kotlin/misk/redis/RealRedisClusterTest.kt index 55101d5162f..ef57fe422d8 100644 --- a/misk-redis/src/test/kotlin/misk/redis/RealRedisClusterTest.kt +++ b/misk-redis/src/test/kotlin/misk/redis/RealRedisClusterTest.kt @@ -9,16 +9,8 @@ import misk.redis.testing.DockerRedisCluster import misk.testing.MiskExternalDependency import misk.testing.MiskTest import misk.testing.MiskTestModule -import okio.ByteString.Companion.encodeUtf8 -import org.assertj.core.api.Assertions.assertThat -import org.assertj.core.api.Assertions.assertThatThrownBy -import org.junit.jupiter.api.Test import redis.clients.jedis.ConnectionPoolConfig -import redis.clients.jedis.args.ListDirection -import redis.clients.jedis.exceptions.JedisClusterOperationException import wisp.deployment.TESTING -import kotlin.test.assertEquals -import kotlin.test.assertNull @MiskTest class RealRedisClusterTest : AbstractRedisClusterTest() { @@ -26,7 +18,7 @@ class RealRedisClusterTest : AbstractRedisClusterTest() { @MiskTestModule private val module: Module = object : KAbstractModule() { override fun configure() { - install(RedisClusterModule(DockerRedisCluster.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisClusterModule(DockerRedisCluster.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) install(MiskTestingServiceModule()) install(DeploymentModule(TESTING)) } diff --git a/misk-redis/src/test/kotlin/misk/redis/RealRedisTest.kt b/misk-redis/src/test/kotlin/misk/redis/RealRedisTest.kt index 855887ec53f..90ba24f5673 100644 --- a/misk-redis/src/test/kotlin/misk/redis/RealRedisTest.kt +++ b/misk-redis/src/test/kotlin/misk/redis/RealRedisTest.kt @@ -20,7 +20,7 @@ class RealRedisTest : AbstractRedisTest() { @MiskTestModule private val module: Module = object : KAbstractModule() { override fun configure() { - install(RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) install(MiskTestingServiceModule()) install(DeploymentModule(TESTING)) } diff --git a/misk-redis/src/test/kotlin/misk/redis/RedisAuthPasswordEnvTest.kt b/misk-redis/src/test/kotlin/misk/redis/RedisAuthPasswordEnvTest.kt index 33a85469dfb..32a983cc7e1 100644 --- a/misk-redis/src/test/kotlin/misk/redis/RedisAuthPasswordEnvTest.kt +++ b/misk-redis/src/test/kotlin/misk/redis/RedisAuthPasswordEnvTest.kt @@ -20,7 +20,7 @@ import redis.clients.jedis.ConnectionPoolConfig @MiskTest class RedisAuthPasswordEnvTest { @Test fun `injection succeeds with password-less config in fake environments`() { - assertThat(DockerRedis.config.values.first().redis_auth_password).isEmpty() + assertThat(DockerRedis.replicationGroupConfig.redis_auth_password).isEmpty() val injector = createInjector(fakeEnv, realRedisModule) val redis = injector.getInstance(keyOf()).redis assertThat(redis).isInstanceOf(RealRedis::class.java) @@ -29,7 +29,7 @@ class RedisAuthPasswordEnvTest { } @Test fun `injection fails with password-less config in real environments`() { - assertThat(DockerRedis.config.values.first().redis_auth_password).isEmpty() + assertThat(DockerRedis.replicationGroupConfig.redis_auth_password).isEmpty() val injector = createInjector(realEnv, realRedisModule) val ex = assertThrows { injector.getInstance(keyOf()) } assertThat(ex).hasCauseInstanceOf(IllegalStateException::class.java) @@ -45,7 +45,7 @@ class RedisAuthPasswordEnvTest { private val realRedisModule = object : KAbstractModule() { override fun configure() { - install(RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) install(MiskTestingServiceModule()) } } diff --git a/misk-redis/src/test/kotlin/misk/redis/RedisClientMetricsTest.kt b/misk-redis/src/test/kotlin/misk/redis/RedisClientMetricsTest.kt index b17bee3e8f1..e126d5652ea 100644 --- a/misk-redis/src/test/kotlin/misk/redis/RedisClientMetricsTest.kt +++ b/misk-redis/src/test/kotlin/misk/redis/RedisClientMetricsTest.kt @@ -27,7 +27,7 @@ class RedisClientMetricsTest { override fun configure() { install(DeploymentModule(TESTING)) install(MiskTestingServiceModule()) - install(RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) } } diff --git a/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedis.kt b/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedis.kt index 82b52262a7a..47dfba58d18 100644 --- a/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedis.kt +++ b/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedis.kt @@ -23,7 +23,7 @@ import java.time.Duration * To use this in tests: * * 1. Install a `RedisModule` instead of a `FakeRedisModule`. - * Make sure to supply the [DockerRedis.config] as the [RedisConfig]. + * Make sure to supply the [DockerRedis.replicationGroupConfig] as the [RedisReplicationGroupConfig]. * 2. Add `@MiskExternalDependency private val dockerRedis: DockerRedis` to your test class. */ object DockerRedis : ExternalDependency { @@ -35,7 +35,7 @@ object DockerRedis : ExternalDependency { private val jedis by lazy { JedisPooled(hostname, port) } private val redisNodeConfig = RedisNodeConfig(hostname, port) - private val groupConfig = RedisReplicationGroupConfig( + val replicationGroupConfig = RedisReplicationGroupConfig( writer_endpoint = redisNodeConfig, reader_endpoint = redisNodeConfig, // NB: Docker redis images won't accept a start-up password via Container->withCmd. @@ -46,7 +46,7 @@ object DockerRedis : ExternalDependency { redis_auth_password = "", timeout_ms = 1_000, // 1 second. ) - val config = RedisConfig(mapOf("test-group" to groupConfig)) + val config = RedisConfig(mapOf("test-group" to replicationGroupConfig)) private val composer = Composer( "misk-redis-testing", diff --git a/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedisCluster.kt b/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedisCluster.kt index 16bc2efd85e..b9f722c2671 100644 --- a/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedisCluster.kt +++ b/misk-redis/src/testFixtures/kotlin/misk/redis/testing/DockerRedisCluster.kt @@ -24,7 +24,7 @@ import java.time.Duration * To use this in tests: * * 1. Install a `RedisClusterModule` instead of a `FakeRedisModule`. - * Make sure to supply the [DockerRedisCluster.config] as the [RedisClusterConfig]. + * Make sure to supply the [DockerRedisCluster.replicationGroupConfig] as the [RedisClusterReplicationGroupConfig]. * 2. Add `@MiskExternalDependency private val dockerRedis: DockerRedisCluster` to your test class. */ object DockerRedisCluster : ExternalDependency { @@ -37,12 +37,12 @@ object DockerRedisCluster : ExternalDependency { private val jedis by lazy { JedisCluster(HostAndPort(hostname, initialPort)) } private val redisNodeConfig = RedisNodeConfig(hostname, initialPort) - private val groupConfig = RedisClusterReplicationGroupConfig( + val replicationGroupConfig = RedisClusterReplicationGroupConfig( configuration_endpoint = redisNodeConfig, redis_auth_password = "", timeout_ms = 1_000, ) - val config = RedisClusterConfig(mapOf("test-group" to groupConfig)) + val config = RedisClusterConfig(mapOf("test-group" to replicationGroupConfig)) private const val containerName = "misk-redis-cluster-testing" private val composer = Composer( diff --git a/samples/exemplar/src/test/kotlin/com/squareup/exemplar/RedisRateLimitedActionTests.kt b/samples/exemplar/src/test/kotlin/com/squareup/exemplar/RedisRateLimitedActionTests.kt index 530efd289ac..87193615d53 100644 --- a/samples/exemplar/src/test/kotlin/com/squareup/exemplar/RedisRateLimitedActionTests.kt +++ b/samples/exemplar/src/test/kotlin/com/squareup/exemplar/RedisRateLimitedActionTests.kt @@ -20,7 +20,7 @@ class RedisRateLimitedActionTests : AbstractRateLimitedActionTests() { @MiskTestModule val module: Module = object : KAbstractModule() { override fun configure() { install(ExemplarTestModule()) - install(RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false)) + install(RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false)) install(RedisBucket4jRateLimiterModule()) install(RedisTestFlushModule()) bind().toInstance(SimpleMeterRegistry()) diff --git a/samples/exemplarchat/src/main/kotlin/com/squareup/chat/ChatService.kt b/samples/exemplarchat/src/main/kotlin/com/squareup/chat/ChatService.kt index 7f2eee1c683..fc22ea23ad0 100644 --- a/samples/exemplarchat/src/main/kotlin/com/squareup/chat/ChatService.kt +++ b/samples/exemplarchat/src/main/kotlin/com/squareup/chat/ChatService.kt @@ -19,7 +19,7 @@ fun main(args: Array) { MiskRealServiceModule(), MiskWebModule(config.web), ChatModule(), - RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false), + RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false), ConfigModule.create("chat", config), DeploymentModule(deployment), PrometheusMetricsServiceModule(config.prometheus) diff --git a/samples/exemplarchat/src/test/kotlin/com/squareup/chat/ChatWebSocketActionTest.kt b/samples/exemplarchat/src/test/kotlin/com/squareup/chat/ChatWebSocketActionTest.kt index 9d095411086..a059b672fc9 100644 --- a/samples/exemplarchat/src/test/kotlin/com/squareup/chat/ChatWebSocketActionTest.kt +++ b/samples/exemplarchat/src/test/kotlin/com/squareup/chat/ChatWebSocketActionTest.kt @@ -24,7 +24,7 @@ class ChatWebSocketActionTest { private val module = Modules.combine( MiskTestingServiceModule(), DeploymentModule(TESTING), - RedisModule(DockerRedis.config, ConnectionPoolConfig(), useSsl = false), + RedisModule(DockerRedis.replicationGroupConfig, ConnectionPoolConfig(), useSsl = false), RedisTestFlushModule(), WebActionModule.create() ) From 8cffeac185c4b16bee379acfd112670656588d36 Mon Sep 17 00:00:00 2001 From: afkelsall <56988739+afkelsall@users.noreply.github.com> Date: Sun, 11 Aug 2024 19:59:33 -0400 Subject: [PATCH 12/26] The original implementation of `TaggedLogger` became heavy, stateful and opinionated. We've reconsidered this approach and simplified it to just a function call `withSmartTags` doing away with the stateful builders etc. COPYBARA_INTEGRATE_REVIEW=https://github.com/cashapp/misk/pull/3366 from GitOrigin-RevId: 508c16fd4046947e33c3e282533841978ef2d1a8 --- .../misk/jobqueue/sqs/SqsJobConsumer.kt | 4 +- ...bQueueTest.kt => SmartTagsJobQueueTest.kt} | 69 +-- .../ExceptionHandlingInterceptor.kt | 4 +- ...rtTagsExceptionHandlingInterceptorTest.kt} | 476 +++++++++++------- wisp/wisp-logging/api/wisp-logging.api | 11 +- .../src/main/kotlin/wisp/logging/Logging.kt | 156 ++++-- .../logging/SmartTagsThreadLocalHandler.kt | 67 +++ .../main/kotlin/wisp/logging/TaggedLogger.kt | 61 +-- 8 files changed, 510 insertions(+), 338 deletions(-) rename misk-aws/src/test/kotlin/misk/jobqueue/sqs/{TaggedLoggerJobQueueTest.kt => SmartTagsJobQueueTest.kt} (67%) rename misk/src/test/kotlin/misk/web/exceptions/{TaggedLoggerExceptionHandlingInterceptorTest.kt => SmartTagsExceptionHandlingInterceptorTest.kt} (51%) create mode 100644 wisp/wisp-logging/src/main/kotlin/wisp/logging/SmartTagsThreadLocalHandler.kt diff --git a/misk-aws/src/main/kotlin/misk/jobqueue/sqs/SqsJobConsumer.kt b/misk-aws/src/main/kotlin/misk/jobqueue/sqs/SqsJobConsumer.kt index 58223e15c56..379242ae8d8 100644 --- a/misk-aws/src/main/kotlin/misk/jobqueue/sqs/SqsJobConsumer.kt +++ b/misk-aws/src/main/kotlin/misk/jobqueue/sqs/SqsJobConsumer.kt @@ -21,7 +21,7 @@ import misk.tasks.RepeatedTaskQueue import misk.tasks.Status import misk.time.timed import org.slf4j.MDC -import wisp.logging.TaggedLogger +import wisp.logging.SmartTagsThreadLocalHandler import wisp.logging.error import wisp.logging.getLogger import wisp.tracing.traceWithNewRootSpan @@ -212,7 +212,7 @@ internal class SqsJobConsumer @Inject internal constructor( ) Status.OK } catch (th: Throwable) { - val mdcTags = TaggedLogger.popThreadLocalMdcContext() + val mdcTags = SmartTagsThreadLocalHandler.popThreadLocalSmartTags() log.error(th, *mdcTags.toTypedArray()) { "error handling job from ${queue.queueName}" } diff --git a/misk-aws/src/test/kotlin/misk/jobqueue/sqs/TaggedLoggerJobQueueTest.kt b/misk-aws/src/test/kotlin/misk/jobqueue/sqs/SmartTagsJobQueueTest.kt similarity index 67% rename from misk-aws/src/test/kotlin/misk/jobqueue/sqs/TaggedLoggerJobQueueTest.kt rename to misk-aws/src/test/kotlin/misk/jobqueue/sqs/SmartTagsJobQueueTest.kt index 0d0407e4811..76f7fd1257c 100644 --- a/misk-aws/src/test/kotlin/misk/jobqueue/sqs/TaggedLoggerJobQueueTest.kt +++ b/misk-aws/src/test/kotlin/misk/jobqueue/sqs/SmartTagsJobQueueTest.kt @@ -6,15 +6,12 @@ import com.amazonaws.services.sqs.AmazonSQS import com.amazonaws.services.sqs.model.CreateQueueRequest import jakarta.inject.Inject import misk.annotation.ExperimentalMiskApi -import misk.clustering.fake.lease.FakeLeaseManager import misk.inject.KAbstractModule import misk.jobqueue.JobQueue import misk.jobqueue.QueueName import misk.jobqueue.sqs.SqsJobConsumer.Companion.CONSUMERS_BATCH_SIZE -import misk.jobqueue.sqs.TaggedLoggerJobQueueTest.SqsJobQueueTestTaggedLogger.Companion.getTaggedLogger import misk.jobqueue.subscribe import misk.logging.LogCollectorModule -import misk.tasks.RepeatedTaskQueue import misk.testing.MiskExternalDependency import misk.testing.MiskTest import misk.testing.MiskTestModule @@ -22,20 +19,18 @@ import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import wisp.feature.testing.FakeFeatureFlags -import wisp.logging.Copyable import wisp.logging.LogCollector -import wisp.logging.Tag -import wisp.logging.TaggedLogger import wisp.logging.getLogger +import wisp.logging.withSmartTags import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicInteger -import kotlin.reflect.KClass +@OptIn(ExperimentalMiskApi::class) @MiskTest(startService = true) -internal class TaggedLoggerJobQueueTest { +internal class SmartTagsJobQueueTest { @MiskExternalDependency private val dockerSqs = DockerSqs - @MiskTestModule private val module = object: KAbstractModule() { + @MiskTestModule private val module = object : KAbstractModule() { override fun configure() { install(SqsJobQueueTestModule(dockerSqs.credentials, dockerSqs.client)) install(LogCollectorModule()) @@ -46,14 +41,9 @@ internal class TaggedLoggerJobQueueTest { @Inject private lateinit var queue: JobQueue @Inject private lateinit var consumer: SqsJobConsumer @Inject private lateinit var logCollector: LogCollector - @Inject private lateinit var sqsMetrics: SqsMetrics - @Inject @ForSqsHandling lateinit var taskQueue: RepeatedTaskQueue @Inject private lateinit var fakeFeatureFlags: FakeFeatureFlags - @Inject private lateinit var fakeLeaseManager: FakeLeaseManager - @Inject private lateinit var queueResolver: QueueResolver private lateinit var queueName: QueueName - private lateinit var deadLetterQueueName: QueueName @BeforeEach fun setUp() { @@ -85,20 +75,19 @@ internal class TaggedLoggerJobQueueTest { return@subscribe } - taggedLogger - .testTag("test123") - .asContext { - messageIdToVerify = it.id - taggedLogger.info("Test log with mdc") - throw SqsJobQueueTestException("Test exception") - } + withSmartTags("testTag" to "test123") { + messageIdToVerify = it.id + logger.info("Test log with mdc") + throw SqsJobQueueTestException("Test exception") + } } queue.enqueue(queueName, "job body") assertThat(allJobsComplete.await(10, TimeUnit.SECONDS)).isTrue() - val serviceLogEvents = logCollector.takeEvents(TaggedLoggerJobQueueTest::class, consumeUnmatchedLogs = false) + val serviceLogEvents = + logCollector.takeEvents(SmartTagsJobQueueTest::class, consumeUnmatchedLogs = false) val sqsLogErrorEvents = logCollector.takeEvents(SqsJobConsumer::class) .filter { it.level == Level.ERROR } @@ -113,7 +102,7 @@ internal class TaggedLoggerJobQueueTest { } @Test - fun shouldLogNormallyWhenNotUsingTaggedLogger() { + fun shouldLogNormallyWhenNotUsingSmartTags() { val allJobsComplete = CountDownLatch(1) var messageIdToVerify: String? = null val jobsReceived = AtomicInteger() @@ -127,7 +116,7 @@ internal class TaggedLoggerJobQueueTest { } messageIdToVerify = it.id - normalLogger.info("Test log without mdc") + logger.info("Test log without mdc") throw SqsJobQueueTestException("Test exception") } @@ -135,7 +124,8 @@ internal class TaggedLoggerJobQueueTest { assertThat(allJobsComplete.await(10, TimeUnit.SECONDS)).isTrue() - val serviceLogEvents = logCollector.takeEvents(TaggedLoggerJobQueueTest::class, consumeUnmatchedLogs = false) + val serviceLogEvents = + logCollector.takeEvents(SmartTagsJobQueueTest::class, consumeUnmatchedLogs = false) val sqsLogErrorEvents = logCollector.takeEvents(SqsJobConsumer::class) .filter { it.level == Level.ERROR } @@ -147,31 +137,22 @@ internal class TaggedLoggerJobQueueTest { assertExistingMdcPropertiesArePresent(sqsLogErrorEvents.single(), messageIdToVerify) } - private fun assertExistingMdcPropertiesArePresent(logEvent: ILoggingEvent, messageIdToVerify: String?) { + private fun assertExistingMdcPropertiesArePresent( + logEvent: ILoggingEvent, + messageIdToVerify: String? + ) { assertThat(logEvent.mdcPropertyMap).containsEntry("sqs_job_id", messageIdToVerify) assertThat(logEvent.mdcPropertyMap).containsEntry("misk.job_queue.job_id", messageIdToVerify) - assertThat(logEvent.mdcPropertyMap).containsEntry("misk.job_queue.queue_name", queueName.value) + assertThat(logEvent.mdcPropertyMap).containsEntry( + "misk.job_queue.queue_name", + queueName.value + ) assertThat(logEvent.mdcPropertyMap).containsEntry("misk.job_queue.queue_type", "aws-sqs") } - class SqsJobQueueTestException(override val message: String): Exception() + class SqsJobQueueTestException(override val message: String) : Exception() companion object { - val taggedLogger = this::class.getTaggedLogger() - val normalLogger = getLogger() - } - - @OptIn(ExperimentalMiskApi::class) - data class SqsJobQueueTestTaggedLogger(val logClass: KClass, val tags: Set = emptySet()): TaggedLogger>(logClass, tags), - Copyable> { - fun testTag(value: String) = tag(Tag("testTag", value)) - - companion object { - fun KClass.getTaggedLogger() = SqsJobQueueTestTaggedLogger(this) - } - - override fun copyWithNewTags(newTags: Set): SqsJobQueueTestTaggedLogger { - return copy(tags = newTags) - } + val logger = getLogger() } } diff --git a/misk/src/main/kotlin/misk/web/exceptions/ExceptionHandlingInterceptor.kt b/misk/src/main/kotlin/misk/web/exceptions/ExceptionHandlingInterceptor.kt index 86e51105c3b..55506ae64d6 100644 --- a/misk/src/main/kotlin/misk/web/exceptions/ExceptionHandlingInterceptor.kt +++ b/misk/src/main/kotlin/misk/web/exceptions/ExceptionHandlingInterceptor.kt @@ -22,8 +22,8 @@ import okhttp3.Headers.Companion.toHeaders import okio.Buffer import okio.BufferedSink import okio.ByteString +import wisp.logging.SmartTagsThreadLocalHandler import wisp.logging.Tag -import wisp.logging.TaggedLogger import wisp.logging.error import wisp.logging.getLogger import wisp.logging.log @@ -52,7 +52,7 @@ class ExceptionHandlingInterceptor( chain.proceed(chain.httpCall) } catch (th: Throwable) { try { - val mdcTags = TaggedLogger.popThreadLocalMdcContext() + val mdcTags = SmartTagsThreadLocalHandler.popThreadLocalSmartTags() if (chain.httpCall.dispatchMechanism == DispatchMechanism.GRPC) { // This response object is only used for determining the status code. toGrpcResponse diff --git a/misk/src/test/kotlin/misk/web/exceptions/TaggedLoggerExceptionHandlingInterceptorTest.kt b/misk/src/test/kotlin/misk/web/exceptions/SmartTagsExceptionHandlingInterceptorTest.kt similarity index 51% rename from misk/src/test/kotlin/misk/web/exceptions/TaggedLoggerExceptionHandlingInterceptorTest.kt rename to misk/src/test/kotlin/misk/web/exceptions/SmartTagsExceptionHandlingInterceptorTest.kt index 6b16813623a..d98543ea635 100644 --- a/misk/src/test/kotlin/misk/web/exceptions/TaggedLoggerExceptionHandlingInterceptorTest.kt +++ b/misk/src/test/kotlin/misk/web/exceptions/SmartTagsExceptionHandlingInterceptorTest.kt @@ -9,10 +9,6 @@ import misk.MiskTestingServiceModule import misk.annotation.ExperimentalMiskApi import misk.inject.KAbstractModule import misk.logging.LogCollectorModule -import misk.web.exceptions.TaggedLoggerExceptionHandlingInterceptorTest.LogMDCContextTestAction.LogMDCContextTestActionLogger.Companion.getTaggedLogger -import misk.web.exceptions.TaggedLoggerExceptionHandlingInterceptorTest.NestedLoggersOuterExceptionHandled.ServiceExtendedTaggedLogger.Companion.getTaggedLoggerNestedOuterExceptionThrown -import misk.web.exceptions.TaggedLoggerExceptionHandlingInterceptorTest.NestedLoggersOuterExceptionHandledNoneThrown.ServiceExtendedTaggedLogger.Companion.getTaggedLoggerNestedOuterExceptionThrownThenNone -import misk.web.exceptions.TaggedLoggerExceptionHandlingInterceptorTest.NestedTaggedLoggersThrowsException.ServiceExtendedTaggedLogger.Companion.getTaggedLoggerNested import misk.security.authz.AccessControlModule import misk.security.authz.FakeCallerAuthenticator import misk.security.authz.MiskCallerAuthenticator @@ -25,29 +21,30 @@ import misk.web.ResponseContentType import misk.web.WebActionModule import misk.web.WebServerTestingModule import misk.web.actions.WebAction -import misk.web.exceptions.TaggedLoggerExceptionHandlingInterceptorTest.NestedTaggedLoggersBothSucceed.ServiceExtendedTaggedLogger.Companion.getTaggedLoggerNestedThreads import misk.web.jetty.JettyService import misk.web.mediatype.MediaTypes import mu.KLogger +import mu.KotlinLogging import okhttp3.Headers import okhttp3.OkHttpClient import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.slf4j.MDC -import wisp.logging.Copyable import wisp.logging.LogCollector +import wisp.logging.SmartTagsThreadLocalHandler import wisp.logging.Tag -import wisp.logging.TaggedLogger import wisp.logging.getLogger import wisp.logging.info +import wisp.logging.withSmartTags +import wisp.logging.withTags import java.util.concurrent.Callable import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import kotlin.reflect.KClass @MiskTest(startService = true) -internal class TaggedLoggerExceptionHandlingInterceptorTest { +internal class SmartTagsExceptionHandlingInterceptorTest { @MiskTestModule val module = object :KAbstractModule() { override fun configure() { @@ -57,13 +54,16 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { install(MiskTestingServiceModule()) multibind().to() - install(WebActionModule.create()) + install(WebActionModule.create()) install(WebActionModule.create()) install(WebActionModule.create()) - install(WebActionModule.create()) + install(WebActionModule.create()) + install(WebActionModule.create()) + install(WebActionModule.create()) install(WebActionModule.create()) install(WebActionModule.create()) - install(WebActionModule.create()) + install(WebActionModule.create()) + install(WebActionModule.create()) } } val httpClient = OkHttpClient() @@ -78,41 +78,55 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { } @Test - fun serviceNotUsingTaggedLoggerShouldLogWithMdcContext() { - val response = invoke(ServiceNotUsingTaggedLoggerShouldLogWithHandRolledMdcContext.URL, "caller") + fun serviceNotUsingSmartTagsShouldLogWithMdcContext() { + val response = invoke(ServiceNotUsingSmartTagsShouldLogWithHandRolledMdcContext.URL, "caller") assertThat(response.code).isEqualTo(200) val logs = - logCollector.takeEvents(ServiceNotUsingTaggedLoggerShouldLogWithHandRolledMdcContext::class) - assertThat(logs).hasSize(1) - assertThat(logs.single().message).isEqualTo("Start Test Process") - assertThat(logs.single().level).isEqualTo(Level.INFO) - assertThat(logs.single().mdcPropertyMap).containsEntry( - "HandRolledLoggerTag", - "handRolledLoggerTagValue" - ) + logCollector.takeEvents(ServiceNotUsingSmartTagsShouldLogWithHandRolledMdcContext::class) + assertThat(logs).hasSize(2) + + assertThat(logs.first().message).isEqualTo("Start Test Process") + assertThat(logs.first().level).isEqualTo(Level.INFO) + assertThat(logs.first().mdcPropertyMap).containsEntry("HandRolledLoggerTag", "handRolledLoggerTagValue") + assertThat(logs.first().mdcPropertyMap).containsEntry("secondHandRolledTag", "secondHandRolledTagValue") + + assertThat(logs.last().message).isEqualTo("Log message using withTags") + assertThat(logs.last().level).isEqualTo(Level.WARN) + assertThat(logs.last().mdcPropertyMap).containsEntry("withTagsTag1", "withTagsValue1") + assertThat(logs.last().mdcPropertyMap).containsEntry("withTagsTag2", "withTagsValue2") } - class ServiceNotUsingTaggedLoggerShouldLogWithHandRolledMdcContext @Inject constructor() : + class ServiceNotUsingSmartTagsShouldLogWithHandRolledMdcContext @Inject constructor() : WebAction { @Get(URL) @Unauthenticated @ResponseContentType(MediaTypes.APPLICATION_JSON) fun call(): String { - logger.info(Tag("HandRolledLoggerTag", "handRolledLoggerTagValue")) { "Start Test Process" } + logger.info( + Tag("HandRolledLoggerTag", "handRolledLoggerTagValue"), + "secondHandRolledTag" to "secondHandRolledTagValue" + ) { "Start Test Process" } + + withTags( + Tag("withTagsTag1", "withTagsValue1"), + "withTagsTag2" to "withTagsValue2" + ) { + logger.warn { "Log message using withTags" } + } return "value" } companion object { - val logger = getLogger() - const val URL = "/log/ServiceNotUsingTaggedLoggerShouldLogWithHandRolledMdcContext/test" + val logger = getLogger() + const val URL = "/log/ServiceNotUsingSmartTagsShouldLogWithHandRolledMdcContext/test" } } // This test verifies the current mdc log tag approach still used in a lot of services works as expected @Test - fun serviceNotUsingTaggedLoggerShouldLogExceptionWithoutMdcContext() { + fun serviceNotUsingSmartTagsShouldLogExceptionWithoutMdcContext() { val response = invoke(ServiceUsingLegacyTaggedLoggerShouldLogExceptionWithoutMdcContext.URL, "caller") assertThat(response.code).isEqualTo(500) @@ -120,13 +134,14 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { ServiceUsingLegacyTaggedLoggerShouldLogExceptionWithoutMdcContext::class, consumeUnmatchedLogs = false) val miskLogs = logCollector.takeEvents(ExceptionHandlingInterceptor::class) - // The service info log had a legacy style TaggedLogger defined within the service using asContext + // The service info log had a legacy style SmartTags defined within the service using asContext // This will log regular messages with the specified mdc tags assertThat(serviceLogs).hasSize(1) with(serviceLogs.single()) { assertThat(message).isEqualTo("Test log with tags") assertThat(level).isEqualTo(Level.INFO) assertThat(mdcPropertyMap).containsEntry("tag123", "value123") + assertThat(mdcPropertyMap).containsEntry("secondTag123", "secondValue123") } // But any exceptions thrown within and handled by misk interceptor do not have the mdc tags @@ -146,6 +161,7 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { fun call(): String { LegacyTaggedLogger(getLogger()) .tag(Tag("tag123", "value123")) + .tag(Tag("secondTag123", "secondValue123")) .asContext { getLogger().info("Test log with tags") throw ServiceUsingLegacyTaggedLoggerException("Test Process Exception without tags") @@ -200,6 +216,7 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { assertThat(message).contains("unexpected error dispatching to") assertThat(level).isEqualTo(Level.ERROR) assertThat(mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(mdcPropertyMap).containsEntry("secondTag", "SecondTagValue123") } } @@ -208,42 +225,28 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { @Unauthenticated @ResponseContentType(MediaTypes.APPLICATION_JSON) fun call(): String { - logger - .testTag("SpecialTagValue123") - .asContext { - logger.info { "Tester" } - throw LogMDCContextTestActionException("Test Exception") - } + withSmartTags( + "testTag" to "SpecialTagValue123", + "secondTag" to "SecondTagValue123" + ) { + logger.info { "Tester" } + throw LogMDCContextTestActionException("Test Exception") + } } - class LogMDCContextTestActionException(message: String) : Throwable(message) + class LogMDCContextTestActionException(message: String) : Exception(message) companion object { - val logger = this::class.getTaggedLogger() + val logger = getLogger() const val URL = "/log/LogMDCContextTestAction/test" } - - data class LogMDCContextTestActionLogger(val logClass: KClass, val tags: Set = emptySet()): TaggedLogger>(logClass, tags), - Copyable> { - fun testTag(value: String) = tag(Tag("testTag", value)) - - companion object { - fun KClass.getTaggedLogger(): LogMDCContextTestActionLogger { - return LogMDCContextTestActionLogger(this) - } - } - - override fun copyWithNewTags(newTags: Set): LogMDCContextTestActionLogger { - return this.copy(tags = newTags) - } - } } @Test - fun shouldHandleConcurrentThreadsUsingTaggedLogger() { + fun shouldHandleConcurrentThreadsUsingSmartTags() { // Start one instance of the service val url = jettyService.httpServerUrl.newBuilder() - .encodedPath(NestedTaggedLoggersBothSucceed.URL) + .encodedPath(NestedSmartTagsBothSucceed.URL) .build() // Setup callable to call that service and get the response @@ -253,7 +256,7 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { val request = okhttp3.Request.Builder() .url(url) .get() - .addHeader(NestedTaggedLoggersBothSucceed.IDENTIFIER_HEADER, identifier) + .addHeader(NestedSmartTagsBothSucceed.IDENTIFIER_HEADER, identifier) val response = OkHttpClient().newCall(request.build()).execute() @@ -281,18 +284,18 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { println("Done. Processed ${results.size} threads") val serviceLogs = logCollector - .takeEvents(NestedTaggedLoggersBothSucceed::class, consumeUnmatchedLogs = false) - .plus(logCollector.takeEvents(NestedTaggedLoggersBothSucceed.AnotherClass::class, consumeUnmatchedLogs = false)) + .takeEvents(NestedSmartTagsBothSucceed::class, consumeUnmatchedLogs = false) + .plus(logCollector.takeEvents(NestedSmartTagsBothSucceed.AnotherClass::class, consumeUnmatchedLogs = false)) // Deterministically order all messages val serviceLogsOrdered = serviceLogs.filter { it.message == "Non nested log message" } .plus(serviceLogs.filter { it.message == "Nested log message with two mdc properties" }) .plus(serviceLogs.filter { it.message == "Non nested log message after nest" }) - .plus(serviceLogs.filter { it.message == "Log message after TaggedLogger" }) + .plus(serviceLogs.filter { it.message == "Log message after SmartTags" }) val miskExceptionLogs = logCollector.takeEvents(ExceptionHandlingInterceptor::class) - serviceLogsOrdered.groupBy { it.mdcPropertyMap[NestedTaggedLoggersBothSucceed.EXECUTION_IDENTIFIER] } + serviceLogsOrdered.groupBy { it.mdcPropertyMap[NestedSmartTagsBothSucceed.EXECUTION_IDENTIFIER] } .forEach { (_, logs) -> assertThat(logs.first().message).isEqualTo("Non nested log message") assertThat(logs.first().mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") @@ -306,7 +309,7 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { assertThat(logs[2].mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") assertThat(logs[2].mdcPropertyMap).doesNotContainKey("testTagNested") - assertThat(logs.last().message).isEqualTo("Log message after TaggedLogger") + assertThat(logs.last().message).isEqualTo("Log message after SmartTags") assertThat(logs.last().mdcPropertyMap).doesNotContainKey("testTag") assertThat(logs.last().mdcPropertyMap).doesNotContainKey("testTagNested") } @@ -315,81 +318,115 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { println("Done") } - class NestedTaggedLoggersBothSucceed @Inject constructor() : WebAction { + class NestedSmartTagsBothSucceed @Inject constructor() : WebAction { @Get(URL) @Unauthenticated @ResponseContentType(MediaTypes.APPLICATION_JSON) fun call(@RequestHeaders headers: Headers): String { - val result = logger - .testTag("SpecialTagValue123") - .tag("executionIdentifier" to headers[IDENTIFIER_HEADER]) - .asContext { - logger.info { "Non nested log message" } - var result = "NO RESULT" - - // Starting a new thread will not inherit the MDC context by default due to MDC being thread local - Thread { - result = AnotherClass().functionWithNestedTaggedLogger(headers[IDENTIFIER_HEADER]) - }.also { it.start() }.join(1000) - - logger.info { "Non nested log message after nest" } - result - } + val result = withSmartTags( + "testTag" to "SpecialTagValue123", + EXECUTION_IDENTIFIER to headers[IDENTIFIER_HEADER] + ) { + logger.info { "Non nested log message" } + var result = "NO RESULT" + + // Starting a new thread will not inherit the MDC context by default due to MDC being thread local + Thread { + result = AnotherClass().functionWithNestedSmartTags(headers[IDENTIFIER_HEADER]) + }.also { it.start() }.join(1000) + + logger.info { "Non nested log message after nest" } + result + } // Manually add this tag to identify the execution for verification - logger.info(EXECUTION_IDENTIFIER to headers[IDENTIFIER_HEADER]) { "Log message after TaggedLogger" } + logger.info(EXECUTION_IDENTIFIER to headers[IDENTIFIER_HEADER]) { "Log message after SmartTags" } return result } companion object { - val logger = this::class.getTaggedLoggerNestedThreads() - const val URL = "/log/NestedTaggedLoggersBothSucceed/test" + val logger = getLogger() + const val URL = "/log/NestedSmartTagsBothSucceed/test" const val IDENTIFIER_HEADER = "IDENTIFIER_HEADER" const val EXECUTION_IDENTIFIER = "executionIdentifier" } - data class ServiceExtendedTaggedLogger( - val logClass: KClass, - val tags: Set = emptySet() - ): TaggedLogger>(logClass, tags), Copyable> { - - fun testTag(value: String) = tag(Tag("testTag", value)) - fun testTagNested(value: String) = tag(Tag("testTagNested", value)) - - companion object { - fun KClass.getTaggedLoggerNestedThreads(): ServiceExtendedTaggedLogger { - return ServiceExtendedTaggedLogger(this) + class AnotherClass() { + fun functionWithNestedSmartTags(parentTag: String?): String { + return withSmartTags( + EXECUTION_IDENTIFIER to parentTag, + "testTagNested" to "NestedTagValue123" + ) { + logger.info { "Nested log message with two mdc properties" } + "SUCCESS" } } - override fun copyWithNewTags(newTags: Set): ServiceExtendedTaggedLogger { - return this.copy(tags = newTags) + companion object { + val logger = getLogger() } } + } - class AnotherClass() { - fun functionWithNestedTaggedLogger(parentTag: String?): String { - return logger - .tag("executionIdentifier" to parentTag) - .testTagNested("NestedTagValue123") - .asContext { - logger.info { "Nested log message with two mdc properties" } - "SUCCESS" - } + @Test + fun shouldHaveLogAndExceptionsFromServiceWithNestedMdcContext() { + val response = invoke(NestedSmartTagsThrowsException.URL, "caller") + assertThat(response.code).isEqualTo(500) + + val serviceLogs = logCollector.takeEvents(NestedSmartTagsThrowsException::class, consumeUnmatchedLogs = false) + val miskExceptionLogs = logCollector.takeEvents(ExceptionHandlingInterceptor::class) + + assertThat(serviceLogs).hasSize(2) + assertThat(serviceLogs.first().message).isEqualTo("Non nested log message") + assertThat(serviceLogs.first().mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(serviceLogs.first().mdcPropertyMap).doesNotContainKey("testTagNested") + + assertThat(serviceLogs.last().message).isEqualTo("Nested log message with two mdc properties") + assertThat(serviceLogs.last().mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(serviceLogs.last().mdcPropertyMap).containsEntry("testTagNested", "NestedTagValue123") + + assertThat(miskExceptionLogs).hasSize(1) + with(miskExceptionLogs.single()) { + assertThat(throwableProxy.message).isEqualTo("Nested logger test exception") + assertThat(message).contains("unexpected error dispatching to") + assertThat(level).isEqualTo(Level.ERROR) + assertThat(mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(mdcPropertyMap).containsEntry("testTagNested", "NestedTagValue123") + } + } + + class NestedSmartTagsThrowsException @Inject constructor() : WebAction { + @Get(URL) + @Unauthenticated + @ResponseContentType(MediaTypes.APPLICATION_JSON) + fun call(): String { + return withSmartTags("testTag" to "SpecialTagValue123") { + logger.info { "Non nested log message" } + functionWithNestedSmartTags() } + } - companion object { - val logger = this::class.getTaggedLoggerNestedThreads() + private fun functionWithNestedSmartTags(): String { + return withSmartTags("testTagNested" to "NestedTagValue123") { + logger.info { "Nested log message with two mdc properties" } + throw NestedSmartTagsException("Nested logger test exception") } } + + class NestedSmartTagsException(message: String) : Exception(message) + + companion object { + val logger = getLogger() + const val URL = "/log/NestedSmartTagsLogger/test" + } } @Test - fun shouldHaveLogAndExceptionsFromServiceWithNestedMdcContext() { - val response = invoke(NestedTaggedLoggersThrowsException.URL, "caller") + fun shouldHandleMixedUseOfWithTagsAndWithSmartTags() { + val response = invoke(NestedWithTagsAndWithSmartTagsThrowsException.URL, "caller") assertThat(response.code).isEqualTo(500) - val serviceLogs = logCollector.takeEvents(NestedTaggedLoggersThrowsException::class, consumeUnmatchedLogs = false) + val serviceLogs = logCollector.takeEvents(NestedWithTagsAndWithSmartTagsThrowsException::class, consumeUnmatchedLogs = false) val miskExceptionLogs = logCollector.takeEvents(ExceptionHandlingInterceptor::class) assertThat(serviceLogs).hasSize(2) @@ -406,57 +443,97 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { assertThat(throwableProxy.message).isEqualTo("Nested logger test exception") assertThat(message).contains("unexpected error dispatching to") assertThat(level).isEqualTo(Level.ERROR) - assertThat(mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(mdcPropertyMap).doesNotContainKey("testTag") assertThat(mdcPropertyMap).containsEntry("testTagNested", "NestedTagValue123") } } - class NestedTaggedLoggersThrowsException @Inject constructor() : WebAction { + /* + When outer nesting is withTags and inner nesting is withSmartTags and the inner nesting throws an exception, + the tags on the exception log should only contain the inner tags and not the outer tags. + */ + class NestedWithTagsAndWithSmartTagsThrowsException @Inject constructor() : WebAction { @Get(URL) @Unauthenticated @ResponseContentType(MediaTypes.APPLICATION_JSON) fun call(): String { - return logger - .testTag("SpecialTagValue123") - .asContext { - logger.info { "Non nested log message" } - functionWithNestedTaggedLogger() - } + return withTags("testTag" to "SpecialTagValue123", includeTagsOnExceptionLogs = false) { + logger.info { "Non nested log message" } + functionWithNestedSmartTags() + } } - private fun functionWithNestedTaggedLogger(): String { - return logger - .testTagNested("NestedTagValue123") - .asContext { - logger.info { "Nested log message with two mdc properties" } - throw NestedTaggedLoggersException("Nested logger test exception") - } + private fun functionWithNestedSmartTags(): String { + return withTags("testTagNested" to "NestedTagValue123", includeTagsOnExceptionLogs = true) { // aka "withSmartTags" + logger.info { "Nested log message with two mdc properties" } + throw NestedSmartTagsException("Nested logger test exception") + } } - class NestedTaggedLoggersException(message: String) : Throwable(message) + class NestedSmartTagsException(message: String) : Exception(message) companion object { - val logger = this::class.getTaggedLoggerNested() - const val URL = "/log/NestedTaggedLoggersLogger/test" + val logger = getLogger() + const val URL = "/log/NestedWithTagsAndWithSmartTagsThrowsException/test" } + } - data class ServiceExtendedTaggedLogger(val logClass: KClass, val tags: Set = emptySet()): TaggedLogger>(logClass, tags), - Copyable> { - fun testTag(value: String) = tag(Tag("testTag", value)) - fun testTagNested(value: String) = tag(Tag("testTagNested", value)) + @Test + fun shouldHandleMixedUseOfWithSmartTagsAndWithTags() { + val response = invoke(NestedWithSmartTagsAndWithTagsThrowsException.URL, "caller") + assertThat(response.code).isEqualTo(500) - companion object { - fun KClass.getTaggedLoggerNested(): ServiceExtendedTaggedLogger { - return ServiceExtendedTaggedLogger(this) - } + val serviceLogs = logCollector.takeEvents(NestedWithSmartTagsAndWithTagsThrowsException::class, consumeUnmatchedLogs = false) + val miskExceptionLogs = logCollector.takeEvents(ExceptionHandlingInterceptor::class) + + assertThat(serviceLogs).hasSize(2) + assertThat(serviceLogs.first().message).isEqualTo("Non nested log message") + assertThat(serviceLogs.first().mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(serviceLogs.first().mdcPropertyMap).doesNotContainKey("testTagNested") + + assertThat(serviceLogs.last().message).isEqualTo("Nested log message with two mdc properties") + assertThat(serviceLogs.last().mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(serviceLogs.last().mdcPropertyMap).containsEntry("testTagNested", "NestedTagValue123") + + assertThat(miskExceptionLogs).hasSize(1) + with(miskExceptionLogs.single()) { + assertThat(throwableProxy.message).isEqualTo("Nested logger test exception") + assertThat(message).contains("unexpected error dispatching to") + assertThat(level).isEqualTo(Level.ERROR) + assertThat(mdcPropertyMap).containsEntry("testTag", "SpecialTagValue123") + assertThat(mdcPropertyMap).doesNotContainKey("testTagNested") + } + } + + /* + When outer nesting is smartTags and inner nesting is withTags and the inner nesting throws an exception, + the tags on the exception log should only contain the outer tags and not the inner tags. + */ + class NestedWithSmartTagsAndWithTagsThrowsException @Inject constructor() : WebAction { + @Get(URL) + @Unauthenticated + @ResponseContentType(MediaTypes.APPLICATION_JSON) + fun call(): String { + return withTags("testTag" to "SpecialTagValue123", includeTagsOnExceptionLogs = true) { // aka "withSmartTags" + logger.info { "Non nested log message" } + functionWithNestedSmartTags() } + } - override fun copyWithNewTags(newTags: Set): ServiceExtendedTaggedLogger { - return this.copy(tags = newTags) + private fun functionWithNestedSmartTags(): String { + return withTags("testTagNested" to "NestedTagValue123", includeTagsOnExceptionLogs = false) { + logger.info { "Nested log message with two mdc properties" } + throw NestedSmartTagsException("Nested logger test exception") } } - } + class NestedSmartTagsException(message: String) : Exception(message) + + companion object { + val logger = getLogger() + const val URL = "/log/NestedWithSmartTagsAndWithTagsThrowsException/test" + } + } @Test fun shouldCorrectlyLogOuterMdcOnlyWhenNestedLoggerExceptionIsCaughtAndAnotherThrown() { @@ -487,50 +564,30 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { @Unauthenticated @ResponseContentType(MediaTypes.APPLICATION_JSON) fun call(): String { - return logger - .testTag("SpecialTagValue123") - .asContext { + return withSmartTags("testTag" to "SpecialTagValue123") { try { - functionWithNestedTaggedLogger() - } catch (e: NestedTaggedLoggerException) { + functionWithNestedSmartTags() + } catch (e: NestedSmartTagsException) { logger.warn { "Exception caught and handled" } } - throw OuterTaggedLoggerException("Should not log MDC from nested tagged logger") + throw OuterSmartTagsException("Should not log MDC from nested tagged logger") } } - private fun functionWithNestedTaggedLogger(): String { - return logger - .testTagNested("NestedTagValue123") - .asContext { - throw NestedTaggedLoggerException("Nested logger test exception") + private fun functionWithNestedSmartTags(): String { + return withSmartTags("testTagNested" to "NestedTagValue123") { + throw NestedSmartTagsException("Nested logger test exception") } } - class NestedTaggedLoggerException(message: String) : Throwable(message) - class OuterTaggedLoggerException(message: String) : Throwable(message) + class NestedSmartTagsException(message: String) : Exception(message) + class OuterSmartTagsException(message: String) : Exception(message) companion object { - val logger = this::class.getTaggedLoggerNestedOuterExceptionThrown() + val logger = getLogger() const val URL = "/log/NestedLoggersOuterExceptionHandled/test" } - - data class ServiceExtendedTaggedLogger(val logClass: KClass, val tags: Set = emptySet()): TaggedLogger>(logClass, tags), - Copyable> { - fun testTag(value: String)= tag("testTag" to value) - fun testTagNested(value: String) = tag("testTagNested" to value) - - companion object { - fun KClass.getTaggedLoggerNestedOuterExceptionThrown(): ServiceExtendedTaggedLogger { - return ServiceExtendedTaggedLogger(this) - } - } - - override fun copyWithNewTags(newTags: Set): ServiceExtendedTaggedLogger { - return this.copy(tags = newTags) - } - } } @@ -556,54 +613,97 @@ internal class TaggedLoggerExceptionHandlingInterceptorTest { @Unauthenticated @ResponseContentType(MediaTypes.APPLICATION_JSON) fun call(): String { - logger - .testTag("SpecialTagValue123") - .asContext { + withSmartTags("testTag" to "SpecialTagValue123") { try { - functionWithNestedTaggedLogger() - } catch (_: NestedTaggedLoggerException) { + functionWithNestedSmartTags() + } catch (_: NestedSmartTagsException) { // Just squash for this test } } - // This is testing the ThreadLocal cleanup function within TaggedLogger when asContext() exits + // This is testing the ThreadLocal cleanup function within SmartTags when asContext() exits // without throwing an exception - val shouldBeEmptySet = TaggedLogger.popThreadLocalMdcContext() + val shouldBeEmptySet = SmartTagsThreadLocalHandler.popThreadLocalSmartTags() logger.info { "Should be zero size and log with no MDC context: ${shouldBeEmptySet.size}" } return "" } - private fun functionWithNestedTaggedLogger(): String { - return logger - .testTagNested("NestedTagValue123") - .asContext { - throw NestedTaggedLoggerException("Nested logger test exception") + private fun functionWithNestedSmartTags(): String { + return withSmartTags("testTag" to "SpecialTagValue123") { + throw NestedSmartTagsException("Nested logger test exception") } } - class NestedTaggedLoggerException(message: String) : Throwable(message) - class OuterTaggedLoggerException(message: String) : Throwable(message) + class NestedSmartTagsException(message: String) : Exception(message) + class OuterSmartTagsException(message: String) : Exception(message) companion object { - val logger = this::class.getTaggedLoggerNestedOuterExceptionThrownThenNone() + val logger = getLogger() const val URL = "/log/NestedLoggersOuterExceptionHandledNoneThrown/test" } + } - data class ServiceExtendedTaggedLogger(val logClass: KClass, val tags: Set = emptySet()): TaggedLogger>(logClass, tags), - Copyable> { - fun testTag(value: String)= tag("testTag" to value) - fun testTagNested(value: String) = tag("testTagNested" to value) + @Test + fun shouldDetectWrappedExceptionsUsingCausedByWhenSettingMdcTags() { + val response = invoke(DetectWrappedExceptionsUsingCausedBy.URL, "caller") + assertThat(response.code).isEqualTo(500) - companion object { - fun KClass.getTaggedLoggerNestedOuterExceptionThrownThenNone(): ServiceExtendedTaggedLogger { - return ServiceExtendedTaggedLogger(this) - } + val serviceLogs = logCollector.takeEvents(DetectWrappedExceptionsUsingCausedBy::class, consumeUnmatchedLogs = false) + val miskExceptionLogs = logCollector.takeEvents(ExceptionHandlingInterceptor::class) + + assertThat(serviceLogs).hasSize(2) + assertThat(serviceLogs.first().message).isEqualTo("Non nested log message") + assertThat(serviceLogs.first().mdcPropertyMap).containsEntry("outerNestedTag", "outerNestedTagValue") + assertThat(serviceLogs.first().mdcPropertyMap).doesNotContainKey("innerNestedTag") + + assertThat(serviceLogs.last().message).isEqualTo("Nested log message with two mdc properties") + assertThat(serviceLogs.last().mdcPropertyMap).containsEntry("outerNestedTag", "outerNestedTagValue") + assertThat(serviceLogs.last().mdcPropertyMap).containsEntry("innerNestedTag", "innerNestedTagValue") + + assertThat(miskExceptionLogs).hasSize(1) + with(miskExceptionLogs.single()) { + assertThat(throwableProxy.message).isEqualTo("Wrapped by another exception") + assertThat(message).contains("unexpected error dispatching to") + assertThat(level).isEqualTo(Level.ERROR) + assertThat(mdcPropertyMap).containsEntry("outerNestedTag", "outerNestedTagValue") + assertThat(mdcPropertyMap).containsEntry("innerNestedTag", "innerNestedTagValue") + } + } + + class DetectWrappedExceptionsUsingCausedBy @Inject constructor() : WebAction { + @Get(URL) + @Unauthenticated + @ResponseContentType(MediaTypes.APPLICATION_JSON) + fun call(): String { + return withSmartTags("outerNestedTag" to "outerNestedTagValue") { + logger.info { "Non nested log message" } + functionWithNestedSmartTagsWrapped() } + } - override fun copyWithNewTags(newTags: Set): ServiceExtendedTaggedLogger { - return this.copy(tags = newTags) + private fun functionWithNestedSmartTagsWrapped(): String { + try { + try { + withSmartTags("innerNestedTag" to "innerNestedTagValue") { + logger.info { "Nested log message with two mdc properties" } + throw ProcessException("Nested logger test exception") + } + } catch (e: Exception) { + throw WrappedByException("Wrapped by exception", e) + } + } catch (e: Exception) { + throw WrappedByAnotherException("Wrapped by another exception", e) } } + + class ProcessException(message: String) : Exception(message) + class WrappedByException(message: String, e: Exception) : Exception(message, e) + class WrappedByAnotherException(message: String, e: Exception) : Exception(message, e) + + companion object { + val logger = getLogger() + const val URL = "/log/DetectWrappedExceptionsUsingCausedBy/test" + } } diff --git a/wisp/wisp-logging/api/wisp-logging.api b/wisp/wisp-logging/api/wisp-logging.api index 32d712d2f1d..f70fe2e5b01 100644 --- a/wisp/wisp-logging/api/wisp-logging.api +++ b/wisp/wisp-logging/api/wisp-logging.api @@ -17,7 +17,10 @@ public final class wisp/logging/LoggingKt { public static final fun trace (Lmu/KLogger;[Lkotlin/Pair;Lkotlin/jvm/functions/Function0;)V public static final fun warn (Lmu/KLogger;Ljava/lang/Throwable;[Lkotlin/Pair;Lkotlin/jvm/functions/Function0;)V public static final fun warn (Lmu/KLogger;[Lkotlin/Pair;Lkotlin/jvm/functions/Function0;)V - public static final fun withTags ([Lkotlin/Pair;Lkotlin/jvm/functions/Function0;)V + public static final fun withSmartTags ([Lkotlin/Pair;Lkotlin/jvm/functions/Function0;)Ljava/lang/Object; + public static final fun withTags ([Lkotlin/Pair;Lkotlin/jvm/functions/Function0;)Ljava/lang/Object; + public static final fun withTags ([Lkotlin/Pair;ZLkotlin/jvm/functions/Function0;)Ljava/lang/Object; + public static synthetic fun withTags$default ([Lkotlin/Pair;ZLkotlin/jvm/functions/Function0;ILjava/lang/Object;)Ljava/lang/Object; } public class wisp/logging/SampledLogger : mu/KLogger { @@ -112,6 +115,11 @@ public class wisp/logging/SampledLogger : mu/KLogger { public fun warn (Lorg/slf4j/Marker;Lkotlin/jvm/functions/Function0;)V } +public final class wisp/logging/SmartTagsThreadLocalHandler { + public static final field INSTANCE Lwisp/logging/SmartTagsThreadLocalHandler; + public final fun popThreadLocalSmartTags ()Ljava/util/Set; +} + public abstract class wisp/logging/TaggedLogger : mu/KLogger, wisp/logging/Copyable { public static final field Companion Lwisp/logging/TaggedLogger$Companion; public fun (Lkotlin/reflect/KClass;Ljava/util/Set;)V @@ -209,6 +217,5 @@ public abstract class wisp/logging/TaggedLogger : mu/KLogger, wisp/logging/Copya } public final class wisp/logging/TaggedLogger$Companion { - public final fun popThreadLocalMdcContext ()Ljava/util/Set; } diff --git a/wisp/wisp-logging/src/main/kotlin/wisp/logging/Logging.kt b/wisp/wisp-logging/src/main/kotlin/wisp/logging/Logging.kt index 41ac78d8635..e6755f3e38b 100644 --- a/wisp/wisp-logging/src/main/kotlin/wisp/logging/Logging.kt +++ b/wisp/wisp-logging/src/main/kotlin/wisp/logging/Logging.kt @@ -1,5 +1,6 @@ package wisp.logging +import misk.annotation.ExperimentalMiskApi import mu.KLogger import mu.KotlinLogging import org.slf4j.MDC @@ -9,7 +10,7 @@ import wisp.sampling.Sampler typealias Tag = Pair inline fun getLogger(): KLogger { - return KotlinLogging.logger(T::class.qualifiedName!!) + return KotlinLogging.logger(T::class.qualifiedName!!) } /** @@ -39,77 +40,152 @@ inline fun getLogger(): KLogger { * @return wrapped logger instance */ fun KLogger.sampled(sampler: Sampler = Sampler.rateLimiting(1L)): KLogger { - return SampledLogger(this, sampler) + return SampledLogger(this, sampler) } fun KLogger.info(vararg tags: Tag, message: () -> Any?) = - log(Level.INFO, message = message, tags = tags) + log(Level.INFO, message = message, tags = tags) fun KLogger.warn(vararg tags: Tag, message: () -> Any?) = - log(Level.WARN, message = message, tags = tags) + log(Level.WARN, message = message, tags = tags) fun KLogger.error(vararg tags: Tag, message: () -> Any?) = - log(Level.ERROR, message = message, tags = tags) + log(Level.ERROR, message = message, tags = tags) fun KLogger.debug(vararg tags: Tag, message: () -> Any?) = - log(Level.DEBUG, message = message, tags = tags) + log(Level.DEBUG, message = message, tags = tags) fun KLogger.trace(vararg tags: Tag, message: () -> Any?) = - log(Level.TRACE, message = message, tags = tags) + log(Level.TRACE, message = message, tags = tags) fun KLogger.info(th: Throwable, vararg tags: Tag, message: () -> Any?) = - log(Level.INFO, th, message = message, tags = tags) + log(Level.INFO, th, message = message, tags = tags) fun KLogger.warn(th: Throwable, vararg tags: Tag, message: () -> Any?) = - log(Level.WARN, th, message = message, tags = tags) + log(Level.WARN, th, message = message, tags = tags) fun KLogger.error(th: Throwable, vararg tags: Tag, message: () -> Any?) = - log(Level.ERROR, th, message = message, tags = tags) + log(Level.ERROR, th, message = message, tags = tags) fun KLogger.debug(th: Throwable, vararg tags: Tag, message: () -> Any?) = - log(Level.DEBUG, th, message = message, tags = tags) + log(Level.DEBUG, th, message = message, tags = tags) fun KLogger.trace(th: Throwable, vararg tags: Tag, message: () -> Any?) = - log(Level.TRACE, th, message = message, tags = tags) + log(Level.TRACE, th, message = message, tags = tags) // This logger takes care of adding the mdc tags and cleaning them up when done fun KLogger.log(level: Level, vararg tags: Tag, message: () -> Any?) { - withTags(*tags) { - when (level) { - Level.ERROR -> error(message) - Level.WARN -> warn(message) - Level.INFO -> info(message) - Level.DEBUG -> debug(message) - Level.TRACE -> trace(message) - } + withTags(*tags) { + when (level) { + Level.ERROR -> error(message) + Level.WARN -> warn(message) + Level.INFO -> info(message) + Level.DEBUG -> debug(message) + Level.TRACE -> trace(message) } + } } // This logger takes care of adding the mdc tags and cleaning them up when done fun KLogger.log(level: Level, th: Throwable, vararg tags: Tag, message: () -> Any?) { - withTags(*tags) { - when (level) { - Level.ERROR -> error(th, message) - Level.INFO -> info(th, message) - Level.WARN -> warn(th, message) - Level.DEBUG -> debug(th, message) - Level.TRACE -> trace(th, message) - } + withTags(*tags) { + when (level) { + Level.ERROR -> error(th, message) + Level.INFO -> info(th, message) + Level.WARN -> warn(th, message) + Level.DEBUG -> debug(th, message) + Level.TRACE -> trace(th, message) } + } } -fun withTags(vararg tags: Tag, f: () -> Unit) { - // Establish MDC, saving prior MDC - val priorMDC = tags.map { (k, v) -> - val priorValue = MDC.get(k) - MDC.put(k, v.toString()) - k to priorValue - } +fun withTags(vararg tags: Tag, block: () -> T): T { + // Establish MDC, saving prior MDC + val priorMDC = tags.map { (key, value) -> + val priorValue = MDC.get(key) + MDC.put(key, value.toString()) + key to priorValue + } + + try { + return block() + } finally { + // Restore or clear prior MDC + priorMDC.forEach { (key, value) -> if (value == null) MDC.remove(key) else MDC.put(key, value) } + } +} - try { - f() - } finally { - // Restore or clear prior MDC - priorMDC.forEach { (k, v) -> if (v == null) MDC.remove(k) else MDC.put(k, v) } +/** + * `includeTagsOnExceptionLogs`: For usage instructions, please see docs below on `withSmartTags` + */ +@OptIn(ExperimentalMiskApi::class) +fun withTags( + vararg tags: Tag, + includeTagsOnExceptionLogs: Boolean = false, + block: () -> T +): T { + return withTags(*tags) { + if (includeTagsOnExceptionLogs) { + SmartTagsThreadLocalHandler.includeTagsOnExceptions(*tags, block = block) + } else { + block() } + } +} + +/** + * Use this function to add tags to the MDC context for the duration of the block. + * + * This is particularly useful (the smart aspect) when an exception is thrown within the block, + * the tags can be retrieved outside that block using `SmartTagsThreadLocalHandler.popThreadLocalSmartTags()` + * and added to the MDC context again when logging the exception. + * + * Within Misk this is already built into both WebAction (`misk.web.exceptions.ExceptionHandlingInterceptor`) + * and `misk.jobqueue.sqs.SqsJobConsumer`. These can be used as an example to extend for any + * other incoming "event" consumers within a service such as Kafka, scheduled tasks, temporal workflows, etc. + * + * Usage: + * ``` + * class ServiceAction (private val webClient: WebClient): WebAction { + * + * @Post("/api/resource") + * fun executeWebAction(@RequestBody request: ServiceActionRequest) { + * logger.info() { "Received request" } + * + * val loadedContext = aClient.load(request.id) + * + * withSmartTags( + * "processValue" to request.process_value, + * "contextToken" to loadedContext.token + * ) { + * logger.info() { "Processing request" } + * doSomething() + * } + * } + * + * private fun doSomething() { + * logger.info() { "Start Process" } + * + * client.someWebRequest() // Client throws exception which is caught and logged by misk framework + * + * logger.info() { "Done" } + * } + * + * companion object { + * val logger = KotlinLogging.logger(ServiceAction::class.java.canonicalName) + * } + * } + * ``` + * + * Logging result: + * ``` + * Log MDC context: [] Log message: "Received request" + * Log MDC context: [processValue: "PV_123", contextToken: "contextTokenValue"] Log message: "Processing request" + * Log MDC context: [processValue: "PV_123", contextToken: "contextTokenValue"] Log message: "Start Process" + * Log MDC context: [processValue: "PV_123", contextToken: "contextTokenValue"] Log message: "unexpected error dispatching to ServiceAction" // This log would not normally include the MDC context + * ``` + */ +@ExperimentalMiskApi +fun withSmartTags(vararg tags: Tag, block: () -> T): T { + return withTags(*tags, includeTagsOnExceptionLogs = true, block = block) } diff --git a/wisp/wisp-logging/src/main/kotlin/wisp/logging/SmartTagsThreadLocalHandler.kt b/wisp/wisp-logging/src/main/kotlin/wisp/logging/SmartTagsThreadLocalHandler.kt new file mode 100644 index 00000000000..be08b8fb0c2 --- /dev/null +++ b/wisp/wisp-logging/src/main/kotlin/wisp/logging/SmartTagsThreadLocalHandler.kt @@ -0,0 +1,67 @@ +package wisp.logging + +import misk.annotation.ExperimentalMiskApi + +@ExperimentalMiskApi +object SmartTagsThreadLocalHandler { + private val threadLocalMdcContext = ThreadLocal() + + /** + * Retrieves all the logging MDC tags that were added to the logger via `withSmartTags()` and + * clears the thread local storage. + * + * Note: the thread local storage is only populated when an exception is thrown within a + * `withSmartTags()` block. + */ + fun popThreadLocalSmartTags() = threadLocalMdcContext + .get() + ?.tags + ?.also { threadLocalMdcContext.remove() } + ?: emptySet() + + internal fun clear() = threadLocalMdcContext.remove() + + private fun setOrAppendTags(exception: Exception, tags: Set) { + val existingContext = threadLocalMdcContext.get() + + if (existingContext == null || !existingContext.wasTriggeredBy(exception)) { + threadLocalMdcContext.set(ThreadLocalSmartTagsMdcContext(exception, tags.toSet())) + } else if (existingContext.wasTriggeredBy(exception)) { + threadLocalMdcContext.set(existingContext.copy(tags = existingContext.tags + tags.toSet())) + } + } + + internal fun includeTagsOnExceptions(vararg tags: Tag, block: () -> T): T { + try { + return block().also { + // Exiting this block gracefully: Lets do some cleanup to keep the ThreadLocal clear. + // The scenario here is that when nested `withSmartTags` threw an exception and it was + // caught and handled by this `withSmartTags`, it should clean up the unused and unneeded context. + SmartTagsThreadLocalHandler.clear() + } + } catch (e: Exception) { + // Calls to `withSmartTags` can be nested - only set if there is not already a context set + // This will be cleared upon logging of the exception within misk or if the thrown exception + // is handled by a higher level `withSmartTags` + SmartTagsThreadLocalHandler.setOrAppendTags(e, tags.toSet()) + + throw e + } + } + + private data class ThreadLocalSmartTagsMdcContext( + val triggeringException: Exception, + val tags: Set + ) { + fun wasTriggeredBy(throwable: Throwable): Boolean { + if (triggeringException == throwable) + return true + + return if (throwable.cause != null) { + wasTriggeredBy(throwable.cause!!) + } else { + false + } + } + } +} diff --git a/wisp/wisp-logging/src/main/kotlin/wisp/logging/TaggedLogger.kt b/wisp/wisp-logging/src/main/kotlin/wisp/logging/TaggedLogger.kt index c9cdcddd5fb..9c779733285 100644 --- a/wisp/wisp-logging/src/main/kotlin/wisp/logging/TaggedLogger.kt +++ b/wisp/wisp-logging/src/main/kotlin/wisp/logging/TaggedLogger.kt @@ -3,7 +3,6 @@ package wisp.logging import misk.annotation.ExperimentalMiskApi import mu.KLogger import mu.KotlinLogging -import org.slf4j.MDC import kotlin.reflect.KClass /** @@ -98,68 +97,10 @@ abstract class TaggedLogger ( // Adds the tags to the Mapped Diagnostic Context for the current thread for the duration of the block. fun asContext(f: () -> T): T { - val priorMDC = MDC.getCopyOfContextMap() ?: emptyMap() - - tags.forEach { (k, v) -> - if (v != null) { - MDC.put(k, v.toString()) - } - } - - try { - return f().also { - // Exiting this TaggedLogger gracefully: Lets do some cleanup to keep the ThreadLocal clear. - // The scenario here is that when nested TaggedLogger threw an exception and it was - // caught and handled by this TaggedLogger, it should clean up the unused and unneeded context. - threadLocalMdcContext.remove() - } - } catch (th: Throwable) { - // TaggedLoggers can be nested - only set if there is not already a context set - // This will be cleared upon logging of the exception within misk or if the thrown exception - // is handled by a higher level TaggedLogger - if (shouldSetThreadLocalContext(th)) { - // Set thread local MDC context for the ExceptionHandlingInterceptor to read - threadLocalMdcContext.set(ThreadLocalTaggedLoggerMdcContext.createWithMdcSnapshot(th)) - } - throw th - } finally { - MDC.setContextMap(priorMDC) - } - } - - private fun shouldSetThreadLocalContext(th: Throwable): Boolean { - // This is the first of any nested TaggedLoggers to catch this exception - if (threadLocalMdcContext.get() == null) { - return true - } - - // A nested TaggedLogger may have caught and handled the exception, and has now thrown something else - return !(threadLocalMdcContext.get()?.wasTriggeredBy(th) ?: false) - } - - private data class ThreadLocalTaggedLoggerMdcContext( - val triggeringThrowable: Throwable, - val tags: Set - ) { - fun wasTriggeredBy(throwable: Throwable): Boolean { - return triggeringThrowable == throwable - } - - companion object { - fun createWithMdcSnapshot(triggeringThrowable: Throwable) = - ThreadLocalTaggedLoggerMdcContext(triggeringThrowable, MDC.getCopyOfContextMap().map { Tag(it.key, it.value) }.toSet()) - } + return withSmartTags(*tags.toTypedArray(), block = f) } companion object { - private val threadLocalMdcContext = ThreadLocal() - - fun popThreadLocalMdcContext() = threadLocalMdcContext - .get() - ?.tags - ?.also { threadLocalMdcContext.remove() } - ?: emptySet() - private fun getLogger(loggerClass: KClass): KLogger { return when { loggerClass.isCompanion -> { From 88efd9eeda5345feaa769f237083ce4c00013c68 Mon Sep 17 00:00:00 2001 From: Lakshmi P <85973417+lakpic@users.noreply.github.com> Date: Mon, 12 Aug 2024 10:08:31 -0700 Subject: [PATCH 13/26] In this approach, we rebuild the url with path and parse it so that query parameters are extracted. Advantage of this approach is that tests can pass the full url with queryparameters (e.g /get?param=value). Disadvantage is that this changes how we were constructing the url and there is a chance for breaking old behavior. GitOrigin-RevId: c5a36bd86576979ac1840f36e6ace3a1c130ed25 --- .../src/main/kotlin/misk/web/WebTestClient.kt | 9 ++++++--- .../test/kotlin/misk/web/WebTestClientTest.kt | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/misk-testing/src/main/kotlin/misk/web/WebTestClient.kt b/misk-testing/src/main/kotlin/misk/web/WebTestClient.kt index a93cb677d94..a4b711d5291 100644 --- a/misk-testing/src/main/kotlin/misk/web/WebTestClient.kt +++ b/misk-testing/src/main/kotlin/misk/web/WebTestClient.kt @@ -9,6 +9,7 @@ import okhttp3.Request import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.Response import jakarta.inject.Inject +import okhttp3.HttpUrl.Companion.toHttpUrl import kotlin.reflect.KClass /** @@ -55,11 +56,13 @@ class WebTestClient @Inject constructor( * Performs a call to the started service. Allows the caller to customize the action before it's * sent through. */ - fun call(path: String, action: Request.Builder.() -> Unit): WebTestResponse = - Request.Builder() - .url(jettyService.httpServerUrl.newBuilder().encodedPath(path).build()) + fun call(path: String, action: Request.Builder.() -> Unit): WebTestResponse { + val fullUrl = jettyService.httpServerUrl.toUrl().toString() + path.trimStart('/') + return Request.Builder() + .url(fullUrl.toHttpUrl()) .apply(action) .let { performRequest(it) } + } private fun performRequest(request: Request.Builder): WebTestResponse { request.header("Accept", MediaTypes.ALL) diff --git a/misk-testing/src/test/kotlin/misk/web/WebTestClientTest.kt b/misk-testing/src/test/kotlin/misk/web/WebTestClientTest.kt index e23e137f41e..fd8c84b08c9 100644 --- a/misk-testing/src/test/kotlin/misk/web/WebTestClientTest.kt +++ b/misk-testing/src/test/kotlin/misk/web/WebTestClientTest.kt @@ -22,6 +22,7 @@ class WebTestClientTest { install(WebServerTestingModule()) install(MiskTestingServiceModule()) install(WebActionModule.create()) + install(WebActionModule.create()) install(WebActionModule.create()) } } @@ -34,6 +35,12 @@ class WebTestClientTest { fun get() = Packet("get") } + class GetActionWithQueryParams @Inject constructor() : WebAction { + @Get("/get/param") + @ResponseContentType(MediaTypes.APPLICATION_JSON) + fun get(@QueryParam paramKey:String) = Packet("get with param value: $paramKey") + } + class PostAction @Inject constructor() : WebAction { @Post("/post") @RequestContentType(MediaTypes.APPLICATION_JSON) @@ -50,6 +57,15 @@ class WebTestClientTest { ).isEqualTo(Packet("get")) } + @Test + fun `performs a GET with query param`() { + assertThat( + webTestClient + .get("/get/param?paramKey=test") + .parseJson() + ).isEqualTo(Packet("get with param value: test")) + } + @Test fun `performs a POST`() { assertThat( From 97d5860b38ce6206bfe55afb761725bf0c98e70c Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 13 Aug 2024 10:11:16 +1000 Subject: [PATCH 14/26] Skip resetting DB if DataSourceService has not started GitOrigin-RevId: efb52773373ea3687903bc3fb2825abc36c595bd --- .../src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt index 66bda0ff305..319eb96b83c 100644 --- a/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt +++ b/misk-jdbc/src/testFixtures/kotlin/misk/jdbc/JdbcTestFixture.kt @@ -17,6 +17,10 @@ class JdbcTestFixture( private val persistentTables = setOf("schema_version") override fun reset() { + if (!dataSourceService.isRunning) { + logger.info { "Skipping truncate tables because data source is not running" } + return + } val stopwatch = Stopwatch.createStarted() val truncatedTableNames = shards(dataSourceService).get().flatMap { shard -> From 82763c80be13ccf2201e0a212be34a2fd635cca8 Mon Sep 17 00:00:00 2001 From: Kevin Kim <95660882+kevink-sq@users.noreply.github.com> Date: Wed, 14 Aug 2024 07:37:49 +0900 Subject: [PATCH 15/26] Update MiskTestExtension to stop Jetty in the shutdown hook GitOrigin-RevId: 2d221e51cf17596ec6849d0f2dd4d015df83fe87 --- .../kotlin/misk/testing/MiskTestExtension.kt | 24 +++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt b/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt index 596cc04e7cd..10c58151cc1 100644 --- a/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt +++ b/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt @@ -104,6 +104,13 @@ internal class MiskTestExtension : BeforeEachCallback, AfterEachCallback { Runtime.getRuntime().addShutdownHook( thread(start = false) { serviceManager.stopAsync().awaitStopped(30, TimeUnit.SECONDS) + /** + * By default, Jetty shutdown is not managed by Guava so needs to be + * done explicitly. This is being done in MiskApplication. + */ + if (serviceManager.jettyIsRunning()) { + context.stopJetty() + } } ) } @@ -134,8 +141,8 @@ internal class MiskTestExtension : BeforeEachCallback, AfterEachCallback { * By default, Jetty shutdown is not managed by Guava so needs to be * done explicitly. This is being done in MiskApplication. */ - if (jettyIsRunning()) { - context.retrieve("injector").getInstance().stop() + if (serviceManager.jettyIsRunning()) { + context.stopJetty() } } } @@ -203,6 +210,19 @@ private fun ExtensionContext.startService(): Boolean { } } + +private fun ServiceManager.jettyIsRunning() : Boolean { + return this + .servicesByState() + .values() + .toList() + .any { it.toString().startsWith("JettyService") } +} + +private fun ExtensionContext.stopJetty() { + this.retrieve("injector").getInstance().stop() +} + // The injector is reused across tests if // 1. The tests module(s) used in the test extend ReusableTestModules, AND // 2. The environment variable MISK_TEST_REUSE_INJECTOR is set to true From 25fb475db60e6ae3aad464fe90613b38d2cfaad2 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Wed, 14 Aug 2024 10:25:17 +1000 Subject: [PATCH 16/26] Injector reuse - support test modules with constructor arguments GitOrigin-RevId: 05141a51cfaa6cb608383b5aa7d163b72347b114 --- misk-inject/api/misk-inject.api | 4 +- .../misk/inject/ReusableTestModuleTest.kt | 42 +++++++++++++++++++ .../kotlin/misk/inject/ReusableTestModule.kt | 36 +++++++++++++++- .../misk/metrics/v2/FakeMetricsModule.kt | 1 - misk-testing/README.md | 2 +- 5 files changed, 80 insertions(+), 5 deletions(-) create mode 100644 misk-inject/misk-inject-guice7-test/src/test/kotlin/misk/inject/ReusableTestModuleTest.kt diff --git a/misk-inject/api/misk-inject.api b/misk-inject/api/misk-inject.api index e806ebac051..b1b5b9994a2 100644 --- a/misk-inject/api/misk-inject.api +++ b/misk-inject/api/misk-inject.api @@ -55,7 +55,9 @@ public abstract class misk/inject/KInstallOnceModule : misk/inject/KAbstractModu public final fun hashCode ()I } -public abstract class misk/inject/ReusableTestModule : misk/inject/KInstallOnceModule { +public abstract class misk/inject/ReusableTestModule : misk/inject/KAbstractModule { public fun ()V + public fun equals (Ljava/lang/Object;)Z + public fun hashCode ()I } diff --git a/misk-inject/misk-inject-guice7-test/src/test/kotlin/misk/inject/ReusableTestModuleTest.kt b/misk-inject/misk-inject-guice7-test/src/test/kotlin/misk/inject/ReusableTestModuleTest.kt new file mode 100644 index 00000000000..c242954c79a --- /dev/null +++ b/misk-inject/misk-inject-guice7-test/src/test/kotlin/misk/inject/ReusableTestModuleTest.kt @@ -0,0 +1,42 @@ +package misk.inject + +import misk.annotation.ExperimentalMiskApi +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNotEquals +import org.junit.jupiter.api.Test + +@OptIn(ExperimentalMiskApi::class) +class ReusableTestModuleTest { + + class NoArgTestModuleOne: ReusableTestModule() + class NoArgTestModuleTwo: ReusableTestModule() + class OneArgTestModuleOne(private val installX: Boolean): ReusableTestModule() + class OneArgTestModuleTwo(private val someValue: Int): ReusableTestModule() + class TwoArgsTestModuleOne(private val installX: Boolean, installY: Boolean): ReusableTestModule() + + @Test + fun testEquality() { + assertEquals(NoArgTestModuleOne(), NoArgTestModuleOne()) + assertNotEquals(NoArgTestModuleOne(), NoArgTestModuleTwo()) + + assertEquals(OneArgTestModuleOne(true), OneArgTestModuleOne(true)) + assertNotEquals(OneArgTestModuleOne(true), OneArgTestModuleOne(false)) + assertNotEquals(OneArgTestModuleTwo(1), OneArgTestModuleTwo(2)) + + assertEquals(TwoArgsTestModuleOne(true, false), TwoArgsTestModuleOne(true, false)) + assertNotEquals(TwoArgsTestModuleOne(true, false), TwoArgsTestModuleOne(false, false)) + } + + @Test + fun testHashcode() { + assertEquals(NoArgTestModuleOne().hashCode(), NoArgTestModuleOne().hashCode()) + assertNotEquals(NoArgTestModuleOne().hashCode(), NoArgTestModuleTwo().hashCode()) + + assertEquals(OneArgTestModuleOne(true).hashCode(), OneArgTestModuleOne(true).hashCode()) + assertNotEquals(OneArgTestModuleOne(true).hashCode(), OneArgTestModuleOne(false).hashCode()) + assertNotEquals(OneArgTestModuleTwo(1).hashCode(), OneArgTestModuleTwo(2).hashCode()) + + assertEquals(TwoArgsTestModuleOne(true, false).hashCode(), TwoArgsTestModuleOne(true, false).hashCode()) + assertNotEquals(TwoArgsTestModuleOne(true, false).hashCode(), TwoArgsTestModuleOne(false, false).hashCode()) + } +} diff --git a/misk-inject/src/main/kotlin/misk/inject/ReusableTestModule.kt b/misk-inject/src/main/kotlin/misk/inject/ReusableTestModule.kt index c78db9c0e32..71e532f77c9 100644 --- a/misk-inject/src/main/kotlin/misk/inject/ReusableTestModule.kt +++ b/misk-inject/src/main/kotlin/misk/inject/ReusableTestModule.kt @@ -1,9 +1,41 @@ package misk.inject import misk.annotation.ExperimentalMiskApi +import kotlin.reflect.KProperty1 +import kotlin.reflect.full.memberProperties +import kotlin.reflect.jvm.isAccessible /** - * This class should be extended by test modules used in tests, for misk to reuse the guice injector across tests for significantly faster test suite performance. + * This class should be extended by test modules used in tests, + * for Misk to reuse the Guice injector across tests for significantly faster test suite performance. */ @ExperimentalMiskApi -abstract class ReusableTestModule: KInstallOnceModule() +abstract class ReusableTestModule: KAbstractModule() { + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other == null || this::class != other::class) return false + + val thisProperties = this::class.memberProperties + val otherProperties = other::class.memberProperties + + if (thisProperties.size != otherProperties.size) return false + + for (property in thisProperties) { + property.isAccessible = true + val thisValue = (property as KProperty1).get(this) + val otherValue = property.get(other as ReusableTestModule) + if (thisValue != otherValue) return false + } + + return true + } + + override fun hashCode(): Int { + var result = javaClass.hashCode() + for (property in this::class.memberProperties) { + property.isAccessible = true + result = 31 * result + ((property as KProperty1).get(this)?.hashCode() ?: 0) + } + return result + } +} diff --git a/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt b/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt index 4b4fe08f4d5..0ca2f500d8c 100644 --- a/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt +++ b/misk-metrics/src/testFixtures/kotlin/misk/metrics/v2/FakeMetricsModule.kt @@ -1,6 +1,5 @@ package misk.metrics.v2 -import io.prometheus.client.CollectorRegistry import misk.inject.KAbstractModule class FakeMetricsModule : KAbstractModule() { diff --git a/misk-testing/README.md b/misk-testing/README.md index 384cb89632b..525ca68f702 100644 --- a/misk-testing/README.md +++ b/misk-testing/README.md @@ -31,7 +31,7 @@ You can check [code samples here](./src/test/kotlin/misk/testing). By default, Misk creates a new injector and starts/stops `Service`s for each test method. This can be slow depending on the size of dependency graph and the type of services involved. To speed up tests, you can reuse the injector instance and service lifecycle across tests by: -1. Extending `misk.inject.ReusableTestModule` in your `@MiskTestModule` annotated modules. Note that this does not currently support test module classes with any constructor arguments. +1. Extending `misk.inject.ReusableTestModule` in your `@MiskTestModule` annotated modules. 2. Setting the `MISK_TEST_REUSE_INJECTOR` environment variable to true for the test task in your build file. ```kotlin tasks.withType().configureEach { From c7eea2cca5448494a6fc71a91f98c075f3325f29 Mon Sep 17 00:00:00 2001 From: David Amar <100435712+damar-block@users.noreply.github.com> Date: Wed, 14 Aug 2024 11:16:21 -0700 Subject: [PATCH 17/26] We are not using DefaultBindingScopingVisitor's visitOther function, so we can just implement the interface directly GitOrigin-RevId: e52510622590f1068de635b97cc73fc4e7a03dac --- misk/src/main/kotlin/misk/web/metadata/guice/GuiceMetadata.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/misk/src/main/kotlin/misk/web/metadata/guice/GuiceMetadata.kt b/misk/src/main/kotlin/misk/web/metadata/guice/GuiceMetadata.kt index 33fd8eabc93..afafa9542b6 100644 --- a/misk/src/main/kotlin/misk/web/metadata/guice/GuiceMetadata.kt +++ b/misk/src/main/kotlin/misk/web/metadata/guice/GuiceMetadata.kt @@ -4,7 +4,7 @@ import com.google.inject.Binding import com.google.inject.Injector import com.google.inject.Key import com.google.inject.Scope -import com.google.inject.spi.DefaultBindingScopingVisitor +import com.google.inject.spi.BindingScopingVisitor import jakarta.inject.Inject import misk.web.metadata.Metadata import misk.web.metadata.MetadataProvider @@ -79,7 +79,7 @@ class GuiceMetadataProvider @Inject constructor() : MetadataProvider() { + private class ScopeVisitor : BindingScopingVisitor { override fun visitEagerSingleton(): String { return "Singleton" } From 1b622b0124d6139e67f0b4438d25463083b0bbba Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Fri, 16 Aug 2024 02:10:56 +1000 Subject: [PATCH 18/26] Fixes some misk test reuse injector bugs GitOrigin-RevId: 5c531499ac0f3028ff74d4efbbfceb1344ea7b46 --- .../kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt | 4 +++- .../src/main/kotlin/misk/testing/MiskTestExtension.kt | 11 +---------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt index 33775f3777c..bedbe237736 100644 --- a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt +++ b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt @@ -14,6 +14,8 @@ class TestDynamoDb @Inject constructor( val service: TestDynamoDbService ) : Service by service, TestFixture { override fun reset() { - service.client.reset() + if (service.server.isRunning) { + service.client.reset() + } } } diff --git a/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt b/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt index 10c58151cc1..887c93423d6 100644 --- a/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt +++ b/misk-testing/src/main/kotlin/misk/testing/MiskTestExtension.kt @@ -44,7 +44,7 @@ internal class MiskTestExtension : BeforeEachCallback, AfterEachCallback { binder().requireAtInjectOnConstructors() multibind().to() - if (context.startService()) { + if (context.startService() || context.reuseInjector()) { multibind().to() if (!context.reuseInjector()) { multibind().to() @@ -132,7 +132,6 @@ internal class MiskTestExtension : BeforeEachCallback, AfterEachCallback { lateinit var serviceManager: ServiceManager override fun afterEach(context: ExtensionContext) { - if (context.startService()) { serviceManager.stopAsync() serviceManager.awaitStopped(30, TimeUnit.SECONDS) @@ -146,14 +145,6 @@ internal class MiskTestExtension : BeforeEachCallback, AfterEachCallback { } } } - - private fun jettyIsRunning() : Boolean { - return serviceManager - .servicesByState() - .values() - .toList() - .any { it.toString().startsWith("JettyService") } - } } /** We inject after starting services and uninject after stopping services. */ From 65fb3532f893c9a25b7a6e0b2615fa111c4b30b2 Mon Sep 17 00:00:00 2001 From: David Amar <100435712+damar-block@users.noreply.github.com> Date: Thu, 15 Aug 2024 09:54:24 -0700 Subject: [PATCH 19/26] Add Mutable AI badge to Misk readme GitOrigin-RevId: 22e9c9a4c3bb23f3874d7a1a8dac1bb37df5443d --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 2bc3e6f3600..7fdb712b686 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ -[](http://search.maven.org/#search%7Cga%7C1%7Ccom.squareup.misk) +[](http://search.maven.org/#search%7Cga%7C1%7Ccom.squareup.misk) [![Mutable.ai Auto Wiki](https://img.shields.io/badge/Auto_Wiki-Mutable.ai-blue)](https://wiki.mutable.ai/cashapp/misk) * Releases * See most recent [public build][snap] From 9f038640de840ac8cfc285cd8ead1ad63a222bce Mon Sep 17 00:00:00 2001 From: Shivani Katukota <54708104+katukota@users.noreply.github.com> Date: Thu, 15 Aug 2024 19:16:53 -0400 Subject: [PATCH 20/26] [Misk Test Flakes] misk.jdbc.CockroachDbRealTransacterTest GitOrigin-RevId: 739c0b99ae9c919d193c972af82e31e23385f04e --- .../kotlin/misk/jdbc/RealTransacterTest.kt | 38 +++++++++++++++---- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/misk-jdbc/src/test/kotlin/misk/jdbc/RealTransacterTest.kt b/misk-jdbc/src/test/kotlin/misk/jdbc/RealTransacterTest.kt index a85b9371cb5..1c7b58b924a 100644 --- a/misk-jdbc/src/test/kotlin/misk/jdbc/RealTransacterTest.kt +++ b/misk-jdbc/src/test/kotlin/misk/jdbc/RealTransacterTest.kt @@ -16,6 +16,7 @@ import org.junit.jupiter.api.Test import wisp.config.Config import wisp.deployment.TESTING import java.sql.Connection +import java.sql.SQLException import java.time.LocalDate import kotlin.test.assertFailsWith import kotlin.test.fail @@ -242,16 +243,35 @@ abstract class RealTransacterTest { @Test fun `a new transaction can be started in session close hook`() { - transacter.transactionWithSession { session -> - session.useConnection { connection -> - session.onSessionClose { - transacter.transactionWithSession { innerSession -> - innerSession.useConnection { innerConnection -> - innerConnection.createStatement().execute("INSERT INTO movies (name) VALUES ('1')") + val maxRetries = 3 + var attempt = 0 + var success = false + + while (attempt < maxRetries && !success) { + try { + transacter.transactionWithSession { session -> + session.useConnection { connection -> + session.onSessionClose { + transacter.transactionWithSession { innerSession -> + innerSession.useConnection { innerConnection -> + innerConnection.createStatement() + .execute("INSERT INTO movies (name) VALUES ('1')") + } + } } + connection.createStatement().execute("INSERT INTO movies (name) VALUES ('2')") } } - connection.createStatement().execute("INSERT INTO movies (name) VALUES ('2')") + success = true // Test passed, exit the loop + } catch (e: SQLException) { + if (isCockroachDbRetryableError(e)) { + attempt++ + if (attempt >= maxRetries) { + throw e // Rethrow after max retries + } + } else { + throw e // Rethrow if it's not a retryable error + } } } @@ -259,6 +279,10 @@ abstract class RealTransacterTest { assertThat(afterCount).isEqualTo(2) } + private fun isCockroachDbRetryableError(e: SQLException): Boolean { + return e.sqlState == "40001" // CockroachDB's serializable transaction error code + } + @Test fun cannotStartTransactionWhileInTransaction() { assertFailsWith { From b1a684cc9e6c2cdb1c696d4dcff4ece5b73f50d6 Mon Sep 17 00:00:00 2001 From: yissachar Date: Fri, 16 Aug 2024 14:47:02 -0400 Subject: [PATCH 21/26] Only run buildMiskWeb on shadowJar GitOrigin-RevId: 509e8755c63c32910d74beab22ecaacd6935d5fa --- misk-admin/build.gradle.kts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/misk-admin/build.gradle.kts b/misk-admin/build.gradle.kts index 4df860330d8..22166d5d43a 100644 --- a/misk-admin/build.gradle.kts +++ b/misk-admin/build.gradle.kts @@ -167,10 +167,19 @@ val buildMiskWeb = tasks.register("buildMiskWeb", MiskWebBuildTask::class.java) outputFiles.setFrom(outputs) } -// buildMiskWeb is expensive and generally not needed locally. Only build it on CI, or if +// buildMiskWeb is expensive and generally not needed locally. Only build it on CI shadowJar, or if // specifically requested. val isCi = System.getenv("CI") == "true" || System.getenv("GITHUB_ACTIONS") != null -if (isCi || System.getProperty("misk.admin.buildMiskWeb") == "true") { +if (isCi) { + tasks.named { it == "explodeCodeSourceMain" || it == "shadowJar" }.configureEach { + dependsOn(buildMiskWeb) + } + + // Needed to properly order since there is an implicit dep + tasks.named { it == "processResources" }.configureEach { + mustRunAfter(buildMiskWeb) + } +} else if (System.getProperty("misk.admin.buildMiskWeb") == "true") { tasks.named { it == "explodeCodeSourceMain" || it == "processResources" }.configureEach { dependsOn(buildMiskWeb) } From d3f720571c5f183967957c26cb8488fce4dcf561 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Sat, 17 Aug 2024 11:14:45 +1000 Subject: [PATCH 22/26] Misk test injector reuse - dynamodb reset fix and mockito test fixture GitOrigin-RevId: 8164ac1d5ed2ce86690a377921b05873aedf4a4e --- .../kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt | 2 +- misk-testing/api/misk-testing.api | 5 +++++ misk-testing/build.gradle.kts | 2 +- .../main/kotlin/misk/mockito/MockitoTestFixture.kt | 11 +++++++++++ 4 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 misk-testing/src/main/kotlin/misk/mockito/MockitoTestFixture.kt diff --git a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt index bedbe237736..efb5047eb0d 100644 --- a/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt +++ b/misk-aws2-dynamodb/src/testFixtures/kotlin/misk/aws2/dynamodb/testing/TestDynamoDb.kt @@ -14,7 +14,7 @@ class TestDynamoDb @Inject constructor( val service: TestDynamoDbService ) : Service by service, TestFixture { override fun reset() { - if (service.server.isRunning) { + if (service.client.isRunning) { service.client.reset() } } diff --git a/misk-testing/api/misk-testing.api b/misk-testing/api/misk-testing.api index 25652b1b88e..c206bc3df31 100644 --- a/misk-testing/api/misk-testing.api +++ b/misk-testing/api/misk-testing.api @@ -102,6 +102,11 @@ public final class misk/mockito/Mockito { public static final field INSTANCE Lmisk/mockito/Mockito; } +public final class misk/mockito/MockitoTestFixture : misk/testing/TestFixture { + public fun (Lcom/google/inject/Provider;)V + public fun reset ()V +} + public final class misk/random/FakeRandom : misk/random/Random { public fun ()V public final fun getNextBoolean ()Ljava/lang/Boolean; diff --git a/misk-testing/build.gradle.kts b/misk-testing/build.gradle.kts index 769c063cdeb..8926af4380b 100644 --- a/misk-testing/build.gradle.kts +++ b/misk-testing/build.gradle.kts @@ -25,6 +25,7 @@ dependencies { api(project(":misk-api")) api(project(":misk-core")) api(project(":misk-inject")) + api(project(":misk-testing-api")) implementation(libs.guavaTestLib) implementation(libs.guiceTestLib) implementation(libs.logbackClassic) @@ -39,7 +40,6 @@ dependencies { implementation(project(":misk-action-scopes")) implementation(project(":misk-config")) implementation(project(":misk-service")) - implementation(project(":misk-testing-api")) implementation(testFixtures(project(":misk-metrics"))) testImplementation(libs.kotlinTest) diff --git a/misk-testing/src/main/kotlin/misk/mockito/MockitoTestFixture.kt b/misk-testing/src/main/kotlin/misk/mockito/MockitoTestFixture.kt new file mode 100644 index 00000000000..e7de55bd1ae --- /dev/null +++ b/misk-testing/src/main/kotlin/misk/mockito/MockitoTestFixture.kt @@ -0,0 +1,11 @@ +package misk.mockito + +import com.google.inject.Provider +import misk.testing.TestFixture +import org.mockito.Mockito + +class MockitoTestFixture(private val mockProvider: Provider) : TestFixture { + override fun reset() { + Mockito.reset(mockProvider.get()) + } +} From da25b2c8e1d6f79eefdb7bd0884d7433cc89bdbd Mon Sep 17 00:00:00 2001 From: yissachar Date: Mon, 19 Aug 2024 11:00:54 -0400 Subject: [PATCH 23/26] Build misk-web on assemble, not just shadowJar GitOrigin-RevId: 489b1ed32a14f269a03f2089c9f577679b46be7c --- misk-admin/build.gradle.kts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/misk-admin/build.gradle.kts b/misk-admin/build.gradle.kts index 22166d5d43a..26ea96d2f19 100644 --- a/misk-admin/build.gradle.kts +++ b/misk-admin/build.gradle.kts @@ -167,11 +167,11 @@ val buildMiskWeb = tasks.register("buildMiskWeb", MiskWebBuildTask::class.java) outputFiles.setFrom(outputs) } -// buildMiskWeb is expensive and generally not needed locally. Only build it on CI shadowJar, or if +// buildMiskWeb is expensive and generally not needed locally. Only build it on CI assemble, or if // specifically requested. val isCi = System.getenv("CI") == "true" || System.getenv("GITHUB_ACTIONS") != null if (isCi) { - tasks.named { it == "explodeCodeSourceMain" || it == "shadowJar" }.configureEach { + tasks.named { it == "explodeCodeSourceMain" || it == "assemble" }.configureEach { dependsOn(buildMiskWeb) } From ca3379f7784e5f0ab9318f6296bd108e6d13d216 Mon Sep 17 00:00:00 2001 From: yissachar Date: Mon, 19 Aug 2024 14:29:00 -0400 Subject: [PATCH 24/26] Revert only building misk-web on assemble GitOrigin-RevId: 23e05e36ea6d3f06c66d25b51b2af5f4eea4c65b --- misk-admin/build.gradle.kts | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/misk-admin/build.gradle.kts b/misk-admin/build.gradle.kts index 26ea96d2f19..4df860330d8 100644 --- a/misk-admin/build.gradle.kts +++ b/misk-admin/build.gradle.kts @@ -167,19 +167,10 @@ val buildMiskWeb = tasks.register("buildMiskWeb", MiskWebBuildTask::class.java) outputFiles.setFrom(outputs) } -// buildMiskWeb is expensive and generally not needed locally. Only build it on CI assemble, or if +// buildMiskWeb is expensive and generally not needed locally. Only build it on CI, or if // specifically requested. val isCi = System.getenv("CI") == "true" || System.getenv("GITHUB_ACTIONS") != null -if (isCi) { - tasks.named { it == "explodeCodeSourceMain" || it == "assemble" }.configureEach { - dependsOn(buildMiskWeb) - } - - // Needed to properly order since there is an implicit dep - tasks.named { it == "processResources" }.configureEach { - mustRunAfter(buildMiskWeb) - } -} else if (System.getProperty("misk.admin.buildMiskWeb") == "true") { +if (isCi || System.getProperty("misk.admin.buildMiskWeb") == "true") { tasks.named { it == "explodeCodeSourceMain" || it == "processResources" }.configureEach { dependsOn(buildMiskWeb) } From 3faf89c78ef1b1dd509e71b46830d410457bd6ef Mon Sep 17 00:00:00 2001 From: Shivani Katukota <54708104+katukota@users.noreply.github.com> Date: Mon, 19 Aug 2024 17:33:09 -0400 Subject: [PATCH 25/26] Fix flaky test GitOrigin-RevId: 034d8309a3b9b7d471f900134a51f077093ef74c --- misk-cron/src/test/kotlin/misk/cron/FakeCronModuleTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/misk-cron/src/test/kotlin/misk/cron/FakeCronModuleTest.kt b/misk-cron/src/test/kotlin/misk/cron/FakeCronModuleTest.kt index 5a51ae3b529..36b61a31171 100644 --- a/misk-cron/src/test/kotlin/misk/cron/FakeCronModuleTest.kt +++ b/misk-cron/src/test/kotlin/misk/cron/FakeCronModuleTest.kt @@ -56,8 +56,8 @@ class FakeCronModuleTest { @Singleton private class DependentService @Inject constructor() : AbstractIdleService() { override fun startUp() { - logger.info { "DependentService started" } sleep(1000) + logger.info { "DependentService started" } } override fun shutDown() {} From 2fd946011cfe6ddc42bd0ece97d9e0ef76a95b16 Mon Sep 17 00:00:00 2001 From: Meghan Stewart <132411442+meghans-cash@users.noreply.github.com> Date: Tue, 20 Aug 2024 09:31:28 -0700 Subject: [PATCH 26/26] Add flush_interval to the LaunchDarklyConfig, default to the default flush interval to allow configuring how often events are sent GitOrigin-RevId: 5e36390bc9fe80da834a7b8baf036591e85536fa --- misk-launchdarkly/api/misk-launchdarkly.api | 9 ++++++--- .../launchdarkly/LaunchDarklyFeatureFlagsModule.kt | 8 +++++++- wisp/wisp-launchdarkly/api/wisp-launchdarkly.api | 9 ++++++--- .../main/kotlin/wisp/launchdarkly/LaunchDarklyClient.kt | 6 +++++- .../main/kotlin/wisp/launchdarkly/LaunchDarklyConfig.kt | 5 ++++- 5 files changed, 28 insertions(+), 9 deletions(-) diff --git a/misk-launchdarkly/api/misk-launchdarkly.api b/misk-launchdarkly/api/misk-launchdarkly.api index 5d697c61a17..432fb4dbf5e 100644 --- a/misk-launchdarkly/api/misk-launchdarkly.api +++ b/misk-launchdarkly/api/misk-launchdarkly.api @@ -3,17 +3,20 @@ public final class misk/feature/launchdarkly/LaunchDarklyConfig : wisp/config/Co public fun (Ljava/lang/String;Ljava/lang/String;Z)V public fun (Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;)V public fun (Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;I)V - public synthetic fun (Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;IILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;ILjava/time/Duration;)V + public synthetic fun (Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;ILjava/time/Duration;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun component1 ()Ljava/lang/String; public final fun component2 ()Ljava/lang/String; public final fun component3 ()Z public final fun component4 ()Lmisk/client/HttpClientSSLConfig; public final fun component5 ()I - public final fun copy (Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;I)Lmisk/feature/launchdarkly/LaunchDarklyConfig; - public static synthetic fun copy$default (Lmisk/feature/launchdarkly/LaunchDarklyConfig;Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;IILjava/lang/Object;)Lmisk/feature/launchdarkly/LaunchDarklyConfig; + public final fun component6 ()Ljava/time/Duration; + public final fun copy (Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;ILjava/time/Duration;)Lmisk/feature/launchdarkly/LaunchDarklyConfig; + public static synthetic fun copy$default (Lmisk/feature/launchdarkly/LaunchDarklyConfig;Ljava/lang/String;Ljava/lang/String;ZLmisk/client/HttpClientSSLConfig;ILjava/time/Duration;ILjava/lang/Object;)Lmisk/feature/launchdarkly/LaunchDarklyConfig; public fun equals (Ljava/lang/Object;)Z public final fun getBase_uri ()Ljava/lang/String; public final fun getEvent_capacity ()I + public final fun getFlush_interval ()Ljava/time/Duration; public final fun getSdk_key ()Ljava/lang/String; public final fun getSsl ()Lmisk/client/HttpClientSSLConfig; public final fun getUse_relay_proxy ()Z diff --git a/misk-launchdarkly/src/main/kotlin/misk/feature/launchdarkly/LaunchDarklyFeatureFlagsModule.kt b/misk-launchdarkly/src/main/kotlin/misk/feature/launchdarkly/LaunchDarklyFeatureFlagsModule.kt index 8143964eaea..c3cecbc6cf8 100644 --- a/misk-launchdarkly/src/main/kotlin/misk/feature/launchdarkly/LaunchDarklyFeatureFlagsModule.kt +++ b/misk-launchdarkly/src/main/kotlin/misk/feature/launchdarkly/LaunchDarklyFeatureFlagsModule.kt @@ -6,6 +6,7 @@ import com.launchdarkly.sdk.server.Components import com.launchdarkly.sdk.server.LDClient import com.launchdarkly.sdk.server.LDConfig import com.launchdarkly.sdk.server.integrations.EventProcessorBuilder.DEFAULT_CAPACITY +import com.launchdarkly.sdk.server.integrations.EventProcessorBuilder.DEFAULT_FLUSH_INTERVAL import com.launchdarkly.sdk.server.interfaces.LDClientInterface import com.squareup.moshi.Moshi import io.micrometer.core.instrument.MeterRegistry @@ -66,7 +67,11 @@ class LaunchDarklyModule @JvmOverloads constructor( // Set wait to 0 to not block here. Block in service initialization instead. .startWait(Duration.ofMillis(0)) .dataSource(Components.streamingDataSource()) - .events(Components.sendEvents().capacity(config.event_capacity)) + .events( + Components.sendEvents() + .capacity(config.event_capacity) + .flushInterval(config.flush_interval) + ) if (config.use_relay_proxy) { ldConfig.serviceEndpoints( @@ -97,4 +102,5 @@ data class LaunchDarklyConfig @JvmOverloads constructor( val use_relay_proxy: Boolean = true, val ssl: HttpClientSSLConfig? = null, val event_capacity: Int = DEFAULT_CAPACITY, + val flush_interval: Duration = DEFAULT_FLUSH_INTERVAL, ) : Config diff --git a/wisp/wisp-launchdarkly/api/wisp-launchdarkly.api b/wisp/wisp-launchdarkly/api/wisp-launchdarkly.api index 2b6a42319d7..93705890922 100644 --- a/wisp/wisp-launchdarkly/api/wisp-launchdarkly.api +++ b/wisp/wisp-launchdarkly/api/wisp-launchdarkly.api @@ -21,17 +21,20 @@ public final class wisp/launchdarkly/LaunchDarklyConfig : wisp/config/Config { public fun (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;)V public fun (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;Z)V public fun (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZI)V - public synthetic fun (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZIILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZILjava/time/Duration;)V + public synthetic fun (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZILjava/time/Duration;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun component1 ()Ljava/lang/String; public final fun component2 ()Ljava/lang/String; public final fun component3 ()Lwisp/client/HttpClientSSLConfig; public final fun component4 ()Z public final fun component5 ()I - public final fun copy (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZI)Lwisp/launchdarkly/LaunchDarklyConfig; - public static synthetic fun copy$default (Lwisp/launchdarkly/LaunchDarklyConfig;Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZIILjava/lang/Object;)Lwisp/launchdarkly/LaunchDarklyConfig; + public final fun component6 ()Ljava/time/Duration; + public final fun copy (Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZILjava/time/Duration;)Lwisp/launchdarkly/LaunchDarklyConfig; + public static synthetic fun copy$default (Lwisp/launchdarkly/LaunchDarklyConfig;Ljava/lang/String;Ljava/lang/String;Lwisp/client/HttpClientSSLConfig;ZILjava/time/Duration;ILjava/lang/Object;)Lwisp/launchdarkly/LaunchDarklyConfig; public fun equals (Ljava/lang/Object;)Z public final fun getBase_uri ()Ljava/lang/String; public final fun getEvent_capacity ()I + public final fun getFlush_interval ()Ljava/time/Duration; public final fun getOffline ()Z public final fun getSdk_key ()Ljava/lang/String; public final fun getSsl ()Lwisp/client/HttpClientSSLConfig; diff --git a/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyClient.kt b/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyClient.kt index 52c8a19dd12..5f53d780470 100644 --- a/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyClient.kt +++ b/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyClient.kt @@ -33,7 +33,11 @@ object LaunchDarklyClient { .streaming(baseUri) .events(baseUri) ) - .events(Components.sendEvents().capacity(config.event_capacity)) + .events( + Components.sendEvents() + .capacity(config.event_capacity) + .flushInterval(config.flush_interval) + ) config.ssl?.let { val trustStore = sslLoader.loadTrustStore(config.ssl.trust_store)!! diff --git a/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyConfig.kt b/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyConfig.kt index 3fc9f705c28..451c774230f 100644 --- a/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyConfig.kt +++ b/wisp/wisp-launchdarkly/src/main/kotlin/wisp/launchdarkly/LaunchDarklyConfig.kt @@ -1,13 +1,16 @@ package wisp.launchdarkly import com.launchdarkly.sdk.server.integrations.EventProcessorBuilder.DEFAULT_CAPACITY +import com.launchdarkly.sdk.server.integrations.EventProcessorBuilder.DEFAULT_FLUSH_INTERVAL import wisp.client.HttpClientSSLConfig import wisp.config.Config +import java.time.Duration data class LaunchDarklyConfig @JvmOverloads constructor( val sdk_key: String, val base_uri: String, val ssl: HttpClientSSLConfig? = null, val offline: Boolean = false, - val event_capacity: Int = DEFAULT_CAPACITY + val event_capacity: Int = DEFAULT_CAPACITY, + val flush_interval: Duration = DEFAULT_FLUSH_INTERVAL ) : Config