From fe0c327428ab7aa8d70c1ba938ad637c5e780b09 Mon Sep 17 00:00:00 2001 From: Tyler Gregory Date: Tue, 18 Jun 2024 14:07:49 -0500 Subject: [PATCH 01/33] Acquire a lease per cron task --- .../src/main/kotlin/misk/cron/CronManager.kt | 23 ++++++++++++++ .../src/main/kotlin/misk/cron/CronService.kt | 6 +++- .../src/main/kotlin/misk/cron/CronTask.kt | 15 ++------- .../src/test/kotlin/misk/cron/CronTest.kt | 31 ++++++++++++++++--- 4 files changed, 57 insertions(+), 18 deletions(-) diff --git a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt index 5e323b5e07a..d5e340afd38 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt @@ -7,6 +7,8 @@ import com.cronutils.parser.CronParser import jakarta.inject.Inject import jakarta.inject.Singleton import misk.cron.CronManager.CronEntry.ExecutionTimeMetadata.Companion.toMetadata +import wisp.lease.Lease +import wisp.lease.LeaseManager import wisp.logging.getLogger import java.time.Clock import java.time.Instant @@ -15,12 +17,14 @@ import java.time.ZonedDateTime import java.util.concurrent.CompletableFuture import java.util.concurrent.ExecutorService import kotlin.jvm.optionals.getOrNull +import kotlin.reflect.KClass @Singleton class CronManager @Inject constructor() { @Inject private lateinit var clock: Clock @Inject @ForMiskCron private lateinit var executorService: ExecutorService @Inject @ForMiskCron private lateinit var zoneId: ZoneId + @Inject private lateinit var leaseManager: LeaseManager private val runningCrons = mutableListOf>() @@ -101,19 +105,38 @@ class CronManager @Inject constructor() { } removeCompletedCrons() cronEntries.values.forEach { cronEntry -> + // TODO Support old behavior as well + val taskLease = buildTaskLease(cronEntry.runnable::class) + val holdsTaskLease = if (!taskLease.checkHeld()) { + taskLease.acquire() + } else { + true + } + if (!holdsTaskLease) { + return@forEach + } + val nextExecutionTime = cronEntry.executionTime.nextExecution(previousTime).orElseThrow() .withSecond(0) .withNano(0) + if (nextExecutionTime.toInstant() <= now) { logger.info { "CronJob ${cronEntry.name} was ready at $nextExecutionTime" } runCron(cronEntry) + taskLease.release() } } } + internal fun buildTaskLease(klass: KClass): Lease { + val sanitizedClassName = klass.qualifiedName!!.replace(".", "-") + val leaseName = "misk-cron-${sanitizedClassName}" + return leaseManager.requestLease(leaseName) + } + private fun removeCompletedCrons() { runningCrons.removeIf { it.isDone } } diff --git a/misk-cron/src/main/kotlin/misk/cron/CronService.kt b/misk-cron/src/main/kotlin/misk/cron/CronService.kt index bc9bf64231e..c9f77b47e5b 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronService.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronService.kt @@ -2,9 +2,10 @@ package misk.cron import com.google.common.util.concurrent.AbstractIdleService import com.google.inject.Injector -import wisp.logging.getLogger import jakarta.inject.Inject import jakarta.inject.Singleton +import wisp.lease.LeaseManager +import wisp.logging.getLogger @Singleton internal class CronService @Inject constructor( @@ -12,6 +13,7 @@ internal class CronService @Inject constructor( ) : AbstractIdleService() { @Inject private lateinit var cronManager: CronManager @Inject private lateinit var cronRunnableEntries: List + @Inject private lateinit var leaseManager: LeaseManager override fun startUp() { logger.info { "CronService started" } @@ -24,6 +26,8 @@ internal class CronService @Inject constructor( val runnable = injector.getProvider(cronRunnable.runnableClass.java).get() cronManager.addCron(name, cronPattern.pattern, runnable) + + cronManager.buildTaskLease(cronRunnable.runnableClass) } } diff --git a/misk-cron/src/main/kotlin/misk/cron/CronTask.kt b/misk-cron/src/main/kotlin/misk/cron/CronTask.kt index 72bd4d40d63..5ff92b9ca65 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronTask.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronTask.kt @@ -1,21 +1,19 @@ package misk.cron import com.google.common.util.concurrent.AbstractIdleService +import jakarta.inject.Inject +import jakarta.inject.Singleton import misk.clustering.weights.ClusterWeightProvider import misk.tasks.RepeatedTaskQueue import misk.tasks.Status -import wisp.lease.LeaseManager import wisp.logging.getLogger import java.time.Clock import java.time.Duration -import jakarta.inject.Inject -import jakarta.inject.Singleton @Singleton internal class CronTask @Inject constructor() : AbstractIdleService() { @Inject private lateinit var clock: Clock @Inject private lateinit var cronManager: CronManager - @Inject private lateinit var leaseManager: LeaseManager @Inject @ForMiskCron private lateinit var taskQueue: RepeatedTaskQueue @Inject private lateinit var clusterWeight: ClusterWeightProvider @@ -27,16 +25,9 @@ internal class CronTask @Inject constructor() : AbstractIdleService() { logger.info { "CronTask is running on a passive node. Skipping." } return@scheduleWithBackoff Status.OK } - val lease = leaseManager.requestLease(CRON_CLUSTER_LEASE_NAME) val now = clock.instant() - var leaseHeld = lease.checkHeld() - if (!leaseHeld) { - leaseHeld = lease.acquire() - } - if (leaseHeld) { - cronManager.runReadyCrons(lastRun) - } + cronManager.runReadyCrons(lastRun) lastRun = now Status.OK } diff --git a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt index 894a4ec0eaa..b75dd5fb375 100644 --- a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt +++ b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt @@ -1,7 +1,10 @@ package misk.cron +import jakarta.inject.Inject +import jakarta.inject.Singleton import misk.backoff.FlatBackoff import misk.backoff.retry +import misk.clustering.fake.lease.FakeLeaseManager import misk.clustering.weights.FakeClusterWeight import misk.concurrent.ExplicitReleaseDelayQueue import misk.inject.KAbstractModule @@ -15,8 +18,6 @@ import java.time.Clock import java.time.Duration import java.time.Instant import java.time.ZoneId -import jakarta.inject.Inject -import jakarta.inject.Singleton @MiskTest(startService = true) class CronTest { @@ -34,8 +35,9 @@ class CronTest { @Inject private lateinit var cronManager: CronManager @Inject private lateinit var clock: FakeClock - @Inject lateinit var pendingTasks: ExplicitReleaseDelayQueue - @Inject lateinit var fakeClusterWeight: FakeClusterWeight + @Inject private lateinit var fakeClusterWeight: FakeClusterWeight + @Inject private lateinit var fakeLeaseManager: FakeLeaseManager + @Inject private lateinit var pendingTasks: ExplicitReleaseDelayQueue @Inject private lateinit var minuteCron: MinuteCron @Inject private lateinit var hourCron: HourCron @@ -70,7 +72,26 @@ class CronTest { } @Test - fun leaseDenied() { + fun `should not execute if lease is already held`() { + assertThat(minuteCron.counter).isZero() + + clock.add(Duration.ofMinutes(1)) + waitForNextPendingTask().task() + cronManager.waitForCronsComplete() + assertThat(minuteCron.counter).isEqualTo(1) + + // Mark the lease as held by another process + fakeLeaseManager.markLeaseHeldElsewhere("misk-cron-misk-cron-MinuteCron") + + // The lease is already held, we should not execute + clock.add(Duration.ofMinutes(1)) + waitForNextPendingTask().task() + cronManager.waitForCronsComplete() + assertThat(minuteCron.counter).isEqualTo(1) + } + + @Test + fun zeroClusterWeightPreventsExecution() { assertThat(minuteCron.counter).isEqualTo(0) // Cluster weight is 100 by default, so the cron will run. clock.add(Duration.ofMinutes(1)) From 2a41ba9168e764112f82141f8f956271f15e8f0b Mon Sep 17 00:00:00 2001 From: Din Daniyarbekov Date: Wed, 19 Jun 2024 04:42:48 -0400 Subject: [PATCH 02/33] Add backwards compatibility and update README --- misk-cron/README.md | 8 +++- misk-cron/build.gradle.kts | 1 + .../src/main/kotlin/misk/cron/CronManager.kt | 47 ++++++++++++++++++- .../src/main/kotlin/misk/cron/CronModule.kt | 26 ++++++---- .../src/main/kotlin/misk/cron/CronService.kt | 5 +- .../src/main/kotlin/misk/cron/CronTask.kt | 7 +-- .../src/test/kotlin/misk/cron/CronTest.kt | 30 ++++++++---- 7 files changed, 96 insertions(+), 28 deletions(-) diff --git a/misk-cron/README.md b/misk-cron/README.md index a3a59576fa5..acf5479042e 100644 --- a/misk-cron/README.md +++ b/misk-cron/README.md @@ -1,6 +1,5 @@ ## misk-cron This module gives Misk services a way to run scheduled tasks using [cron](https://en.wikipedia.org/wiki/Cron) syntax. [Leases](https://github.com/cashapp/wisp/tree/main/wisp-lease) are used to ensure that only one instance of the service executes the schedule at a time. - Note that there are **not** strong guarantees on task execution; tasks can be delayed or missed entirely for many reasons, including if the instance currently holding the lease is degraded or if it dies completely while executing the task. This module is not a good choice for highly sensitive, business critical needs. ## Usage @@ -23,3 +22,10 @@ Note that there are **not** strong guarantees on task execution; tasks can be de } } ``` +## Cron Task Distribution +Please keep in mind that the current behavior of the leases is that a single instance of the service runs all the ready cron tasks. + +If you wish to distribute the tasks across the fleet of pods, you would need to do the following: +- Update the `cronLeaseBehavior` within the `CronModule` to be equal to `ONE_LEASE_PER_CRON` + +Note that there are **no** strong guarantees on lease distribution if you take this approach: **all leases for tasks could end up on a single pod**. \ No newline at end of file diff --git a/misk-cron/build.gradle.kts b/misk-cron/build.gradle.kts index dde0d35f8db..5226b5997bb 100644 --- a/misk-cron/build.gradle.kts +++ b/misk-cron/build.gradle.kts @@ -25,6 +25,7 @@ dependencies { testImplementation(libs.assertj) testImplementation(libs.junitApi) + testImplementation(libs.junitParams) testImplementation(libs.logbackClassic) testImplementation(project(":wisp:wisp-logging-testing")) testImplementation(project(":wisp:wisp-time-testing")) diff --git a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt index d5e340afd38..43869725290 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt @@ -24,6 +24,7 @@ class CronManager @Inject constructor() { @Inject private lateinit var clock: Clock @Inject @ForMiskCron private lateinit var executorService: ExecutorService @Inject @ForMiskCron private lateinit var zoneId: ZoneId + @Inject @ForMiskCron private lateinit var cronLeaseBehavior: CronLeaseBehavior @Inject private lateinit var leaseManager: LeaseManager private val runningCrons = mutableListOf>() @@ -96,7 +97,28 @@ class CronManager @Inject constructor() { cronEntries.clear() } - fun runReadyCrons(lastRun: Instant) { + fun tryToRunCrons(lastRun: Instant) { + if (cronLeaseBehavior == CronLeaseBehavior.ONE_LEASE_PER_CLUSTER) { + tryAcquireSingleLeaseAndRunCrons(lastRun) + return + } + + tryAcquireLeasePerCronAndRunCrons(lastRun) + } + + fun tryAcquireSingleLeaseAndRunCrons(lastRun: Instant) { + val singleLease = leaseManager.requestLease(CRON_CLUSTER_LEASE_NAME) + + val holdsTaskLease = if (!singleLease.checkHeld()) { + singleLease.acquire() + } else { + true + } + // if we are not holding the lease -> just return + if (!holdsTaskLease) { + return + } + val now = clock.instant() val previousTime = ZonedDateTime.ofInstant(lastRun, zoneId) @@ -105,7 +127,27 @@ class CronManager @Inject constructor() { } removeCompletedCrons() cronEntries.values.forEach { cronEntry -> - // TODO Support old behavior as well + val nextExecutionTime = cronEntry.executionTime.nextExecution(previousTime).orElseThrow() + .withSecond(0) + .withNano(0) + + if (nextExecutionTime.toInstant() <= now) { + logger.info { + "CronJob ${cronEntry.name} was ready at $nextExecutionTime" + } + runCron(cronEntry) + } + } + } + + private fun tryAcquireLeasePerCronAndRunCrons(lastRun: Instant) { + val now = clock.instant() + val previousTime = ZonedDateTime.ofInstant(lastRun, zoneId) + logger.info { + "Last execution was at $previousTime, now=${ZonedDateTime.ofInstant(now, zoneId)}" + } + removeCompletedCrons() + cronEntries.values.forEach { cronEntry -> val taskLease = buildTaskLease(cronEntry.runnable::class) val holdsTaskLease = if (!taskLease.checkHeld()) { taskLease.acquire() @@ -167,6 +209,7 @@ class CronManager @Inject constructor() { } companion object { + private const val CRON_CLUSTER_LEASE_NAME = "misk.cron.lease" private val logger = getLogger() } } diff --git a/misk-cron/src/main/kotlin/misk/cron/CronModule.kt b/misk-cron/src/main/kotlin/misk/cron/CronModule.kt index 359b51817a5..88c1f49449c 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronModule.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronModule.kt @@ -3,23 +3,24 @@ package misk.cron import com.google.common.util.concurrent.Service import com.google.inject.Key import com.google.inject.Provides +import jakarta.inject.Qualifier import jakarta.inject.Singleton import misk.ReadyService import misk.ServiceModule import misk.concurrent.ExecutorServiceModule +import misk.inject.KAbstractModule +import misk.inject.KInstallOnceModule import misk.inject.toKey import misk.tasks.RepeatedTaskQueue import misk.tasks.RepeatedTaskQueueFactory -import java.time.ZoneId -import jakarta.inject.Qualifier -import misk.inject.KAbstractModule -import misk.inject.KInstallOnceModule import misk.web.metadata.MetadataModule +import java.time.ZoneId class CronModule @JvmOverloads constructor( private val zoneId: ZoneId, private val threadPoolSize: Int = 10, - private val dependencies: List> = listOf() + private val dependencies: List> = listOf(), + private val cronLeaseBehavior: CronLeaseBehavior = CronLeaseBehavior.ONE_LEASE_PER_CRON, ) : KInstallOnceModule() { override fun configure() { install(FakeCronModule(zoneId, threadPoolSize, dependencies)) @@ -30,22 +31,26 @@ class CronModule @JvmOverloads constructor( dependsOn = dependencies, ).dependsOn() ) + bind().annotatedWith().toInstance(cronLeaseBehavior) + install(MetadataModule(CronMetadataProvider())) } - @Provides @ForMiskCron @Singleton fun provideTaskQueue(queueFactory: RepeatedTaskQueueFactory): RepeatedTaskQueue = queueFactory.new("misk.cron.task-queue") + } class FakeCronModule @JvmOverloads constructor( private val zoneId: ZoneId, private val threadPoolSize: Int = 10, - private val dependencies: List> = listOf() -) : KAbstractModule() { + private val dependencies: List> = listOf(), + private val cronLeaseBehavior: CronLeaseBehavior = CronLeaseBehavior.ONE_LEASE_PER_CLUSTER, + ) : KAbstractModule() { override fun configure() { bind().annotatedWith().toInstance(zoneId) + bind().annotatedWith().toInstance(cronLeaseBehavior) install( ExecutorServiceModule.withFixedThreadPool( ForMiskCron::class, @@ -66,3 +71,8 @@ class FakeCronModule @JvmOverloads constructor( @Qualifier @Target(AnnotationTarget.FIELD, AnnotationTarget.FUNCTION, AnnotationTarget.VALUE_PARAMETER) internal annotation class ForMiskCron + +enum class CronLeaseBehavior { + ONE_LEASE_PER_CLUSTER, + ONE_LEASE_PER_CRON, +} diff --git a/misk-cron/src/main/kotlin/misk/cron/CronService.kt b/misk-cron/src/main/kotlin/misk/cron/CronService.kt index c9f77b47e5b..bda642b0474 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronService.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronService.kt @@ -9,11 +9,10 @@ import wisp.logging.getLogger @Singleton internal class CronService @Inject constructor( - private val injector: Injector + private val injector: Injector, ) : AbstractIdleService() { @Inject private lateinit var cronManager: CronManager @Inject private lateinit var cronRunnableEntries: List - @Inject private lateinit var leaseManager: LeaseManager override fun startUp() { logger.info { "CronService started" } @@ -26,7 +25,7 @@ internal class CronService @Inject constructor( val runnable = injector.getProvider(cronRunnable.runnableClass.java).get() cronManager.addCron(name, cronPattern.pattern, runnable) - + // We want to request an interest in holding a lease as soon as the service starts up cronManager.buildTaskLease(cronRunnable.runnableClass) } } diff --git a/misk-cron/src/main/kotlin/misk/cron/CronTask.kt b/misk-cron/src/main/kotlin/misk/cron/CronTask.kt index 5ff92b9ca65..f78fac11e91 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronTask.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronTask.kt @@ -26,9 +26,8 @@ internal class CronTask @Inject constructor() : AbstractIdleService() { return@scheduleWithBackoff Status.OK } - val now = clock.instant() - cronManager.runReadyCrons(lastRun) - lastRun = now + cronManager.tryToRunCrons(lastRun) + lastRun = clock.instant() Status.OK } } @@ -40,8 +39,6 @@ internal class CronTask @Inject constructor() : AbstractIdleService() { companion object { val INTERVAL: Duration = Duration.ofSeconds(60L) - private const val CRON_CLUSTER_LEASE_NAME = "misk.cron.lease" - private val logger = getLogger() } } diff --git a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt index b75dd5fb375..e337dc1a97d 100644 --- a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt +++ b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt @@ -12,7 +12,8 @@ import misk.tasks.DelayedTask import misk.testing.MiskTest import misk.testing.MiskTestModule import org.assertj.core.api.Assertions.assertThat -import org.junit.jupiter.api.Test +import org.junit.jupiter.params.ParameterizedTest +import org.junit.jupiter.params.provider.MethodSource import wisp.time.FakeClock import java.time.Clock import java.time.Duration @@ -46,15 +47,16 @@ class CronTest { private lateinit var lastRun: Instant // This simulates what the automated part of cron does. - private fun runCrons() { + private fun runCrons(cronLeaseBehavior: CronLeaseBehavior) { val now = clock.instant() - cronManager.runReadyCrons(lastRun) + cronManager.tryToRunCrons(lastRun) lastRun = now cronManager.waitForCronsComplete() } - @Test - fun basic() { + @ParameterizedTest + @MethodSource("cronLeaseBehaviorList") + fun basic(cronLeaseBehavior: CronLeaseBehavior) { lastRun = clock.instant() assertThat(minuteCron.counter).isEqualTo(0) @@ -64,15 +66,16 @@ class CronTest { // Advance one hour in one minute intervals. repeat(60) { clock.add(Duration.ofMinutes(1)) - runCrons() + runCrons(cronLeaseBehavior) } assertThat(minuteCron.counter).isEqualTo(60) assertThat(throwsExceptionCron.counter).isEqualTo(4) assertThat(hourCron.counter).isEqualTo(1) } - @Test - fun `should not execute if lease is already held`() { + @ParameterizedTest + @MethodSource("cronLeaseBehaviorList") + fun `should not execute if lease is already held`(cronLeaseBehavior: CronLeaseBehavior) { assertThat(minuteCron.counter).isZero() clock.add(Duration.ofMinutes(1)) @@ -90,7 +93,8 @@ class CronTest { assertThat(minuteCron.counter).isEqualTo(1) } - @Test + @ParameterizedTest + @MethodSource("cronLeaseBehaviorList") fun zeroClusterWeightPreventsExecution() { assertThat(minuteCron.counter).isEqualTo(0) // Cluster weight is 100 by default, so the cron will run. @@ -111,6 +115,14 @@ class CronTest { retry(5, FlatBackoff(Duration.ofMillis(200))) { pendingTasks.peekPending()!! } + + companion object { + @JvmStatic + private fun cronLeaseBehaviorList() = listOf( + CronLeaseBehavior.ONE_LEASE_PER_CRON, + CronLeaseBehavior.ONE_LEASE_PER_CLUSTER, + ) + } } @Singleton From 4b0b4f9aa3a20d90e1f0da76ad9c57d618fc7c5d Mon Sep 17 00:00:00 2001 From: Din Daniyarbekov Date: Mon, 24 Jun 2024 03:32:15 -0400 Subject: [PATCH 03/33] resolve comments --- misk-cron/src/main/kotlin/misk/cron/CronManager.kt | 8 +++----- misk-cron/src/main/kotlin/misk/cron/CronService.kt | 8 ++++++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt index 43869725290..3cec5a61866 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt @@ -98,12 +98,10 @@ class CronManager @Inject constructor() { } fun tryToRunCrons(lastRun: Instant) { - if (cronLeaseBehavior == CronLeaseBehavior.ONE_LEASE_PER_CLUSTER) { - tryAcquireSingleLeaseAndRunCrons(lastRun) - return + when(cronLeaseBehavior) { + CronLeaseBehavior.ONE_LEASE_PER_CRON -> tryAcquireLeasePerCronAndRunCrons(lastRun) + CronLeaseBehavior.ONE_LEASE_PER_CLUSTER -> tryAcquireSingleLeaseAndRunCrons(lastRun) } - - tryAcquireLeasePerCronAndRunCrons(lastRun) } fun tryAcquireSingleLeaseAndRunCrons(lastRun: Instant) { diff --git a/misk-cron/src/main/kotlin/misk/cron/CronService.kt b/misk-cron/src/main/kotlin/misk/cron/CronService.kt index bda642b0474..93eda1c70b7 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronService.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronService.kt @@ -13,6 +13,7 @@ internal class CronService @Inject constructor( ) : AbstractIdleService() { @Inject private lateinit var cronManager: CronManager @Inject private lateinit var cronRunnableEntries: List + @Inject @ForMiskCron private lateinit var cronLeaseBehavior: CronLeaseBehavior override fun startUp() { logger.info { "CronService started" } @@ -25,8 +26,11 @@ internal class CronService @Inject constructor( val runnable = injector.getProvider(cronRunnable.runnableClass.java).get() cronManager.addCron(name, cronPattern.pattern, runnable) - // We want to request an interest in holding a lease as soon as the service starts up - cronManager.buildTaskLease(cronRunnable.runnableClass) + + if (cronLeaseBehavior == CronLeaseBehavior.ONE_LEASE_PER_CRON) { + // We want to request an interest in holding a lease as soon as the service starts up + cronManager.buildTaskLease(cronRunnable.runnableClass) + } } } From 7aa7afedf3b03964fc9738a8dd066d1041deca52 Mon Sep 17 00:00:00 2001 From: Tyler Gregory Date: Mon, 24 Jun 2024 10:05:13 -0500 Subject: [PATCH 04/33] API dump --- misk-cron/api/misk-cron.api | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/misk-cron/api/misk-cron.api b/misk-cron/api/misk-cron.api index 9f031571448..8fe7c01fcf4 100644 --- a/misk-cron/api/misk-cron.api +++ b/misk-cron/api/misk-cron.api @@ -7,10 +7,19 @@ public final class misk/cron/CronEntryModule$Companion { public final fun create (Lkotlin/reflect/KClass;)Lmisk/cron/CronEntryModule; } +public final class misk/cron/CronLeaseBehavior : java/lang/Enum { + public static final field ONE_LEASE_PER_CLUSTER Lmisk/cron/CronLeaseBehavior; + public static final field ONE_LEASE_PER_CRON Lmisk/cron/CronLeaseBehavior; + public static fun getEntries ()Lkotlin/enums/EnumEntries; + public static fun valueOf (Ljava/lang/String;)Lmisk/cron/CronLeaseBehavior; + public static fun values ()[Lmisk/cron/CronLeaseBehavior; +} + public final class misk/cron/CronManager { public static final field Companion Lmisk/cron/CronManager$Companion; public fun ()V - public final fun runReadyCrons (Ljava/time/Instant;)V + public final fun tryAcquireSingleLeaseAndRunCrons (Ljava/time/Instant;)V + public final fun tryToRunCrons (Ljava/time/Instant;)V public final fun waitForCronsComplete ()V } @@ -38,7 +47,8 @@ public final class misk/cron/CronModule : misk/inject/KInstallOnceModule { public fun (Ljava/time/ZoneId;)V public fun (Ljava/time/ZoneId;I)V public fun (Ljava/time/ZoneId;ILjava/util/List;)V - public synthetic fun (Ljava/time/ZoneId;ILjava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Ljava/time/ZoneId;ILjava/util/List;Lmisk/cron/CronLeaseBehavior;)V + public synthetic fun (Ljava/time/ZoneId;ILjava/util/List;Lmisk/cron/CronLeaseBehavior;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun provideTaskQueue (Lmisk/tasks/RepeatedTaskQueueFactory;)Lmisk/tasks/RepeatedTaskQueue; } @@ -50,6 +60,7 @@ public final class misk/cron/FakeCronModule : misk/inject/KAbstractModule { public fun (Ljava/time/ZoneId;)V public fun (Ljava/time/ZoneId;I)V public fun (Ljava/time/ZoneId;ILjava/util/List;)V - public synthetic fun (Ljava/time/ZoneId;ILjava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Ljava/time/ZoneId;ILjava/util/List;Lmisk/cron/CronLeaseBehavior;)V + public synthetic fun (Ljava/time/ZoneId;ILjava/util/List;Lmisk/cron/CronLeaseBehavior;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } From 7b7a0a6b0e17151965bbd691714a4d30c756c407 Mon Sep 17 00:00:00 2001 From: Din Daniyarbekov Date: Tue, 25 Jun 2024 11:56:36 -0400 Subject: [PATCH 05/33] fixes --- .../src/test/kotlin/misk/cron/CronTest.kt | 71 ++++++++++++------- .../kotlin/misk/cron/CronTestingModule.kt | 7 +- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt index e337dc1a97d..b56e564376f 100644 --- a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt +++ b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt @@ -12,6 +12,7 @@ import misk.tasks.DelayedTask import misk.testing.MiskTest import misk.testing.MiskTestModule import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.MethodSource import wisp.time.FakeClock @@ -20,25 +21,13 @@ import java.time.Duration import java.time.Instant import java.time.ZoneId -@MiskTest(startService = true) -class CronTest { - @Suppress("unused") - @MiskTestModule - val module = object : KAbstractModule() { - override fun configure() { - install(CronTestingModule()) - - install(CronEntryModule.create()) - install(CronEntryModule.create()) - install(CronEntryModule.create()) - } - } - +abstract class AbstractCronTest { @Inject private lateinit var cronManager: CronManager @Inject private lateinit var clock: FakeClock @Inject private lateinit var fakeClusterWeight: FakeClusterWeight @Inject private lateinit var fakeLeaseManager: FakeLeaseManager @Inject private lateinit var pendingTasks: ExplicitReleaseDelayQueue + @Inject @ForMiskCron private lateinit var cronLeaseBehavior: CronLeaseBehavior @Inject private lateinit var minuteCron: MinuteCron @Inject private lateinit var hourCron: HourCron @@ -47,16 +36,15 @@ class CronTest { private lateinit var lastRun: Instant // This simulates what the automated part of cron does. - private fun runCrons(cronLeaseBehavior: CronLeaseBehavior) { + private fun runCrons() { val now = clock.instant() cronManager.tryToRunCrons(lastRun) lastRun = now cronManager.waitForCronsComplete() } - @ParameterizedTest - @MethodSource("cronLeaseBehaviorList") - fun basic(cronLeaseBehavior: CronLeaseBehavior) { + @Test + fun basic() { lastRun = clock.instant() assertThat(minuteCron.counter).isEqualTo(0) @@ -66,16 +54,15 @@ class CronTest { // Advance one hour in one minute intervals. repeat(60) { clock.add(Duration.ofMinutes(1)) - runCrons(cronLeaseBehavior) + runCrons() } assertThat(minuteCron.counter).isEqualTo(60) assertThat(throwsExceptionCron.counter).isEqualTo(4) assertThat(hourCron.counter).isEqualTo(1) } - @ParameterizedTest - @MethodSource("cronLeaseBehaviorList") - fun `should not execute if lease is already held`(cronLeaseBehavior: CronLeaseBehavior) { + @Test + fun `should not execute if lease is already held`() { assertThat(minuteCron.counter).isZero() clock.add(Duration.ofMinutes(1)) @@ -84,7 +71,12 @@ class CronTest { assertThat(minuteCron.counter).isEqualTo(1) // Mark the lease as held by another process - fakeLeaseManager.markLeaseHeldElsewhere("misk-cron-misk-cron-MinuteCron") + when (cronLeaseBehavior) { + CronLeaseBehavior.ONE_LEASE_PER_CLUSTER -> + fakeLeaseManager.markLeaseHeldElsewhere("misk.cron.lease") + CronLeaseBehavior.ONE_LEASE_PER_CRON -> + fakeLeaseManager.markLeaseHeldElsewhere("misk-cron-misk-cron-MinuteCron") + } // The lease is already held, we should not execute clock.add(Duration.ofMinutes(1)) @@ -93,9 +85,8 @@ class CronTest { assertThat(minuteCron.counter).isEqualTo(1) } - @ParameterizedTest - @MethodSource("cronLeaseBehaviorList") - fun zeroClusterWeightPreventsExecution() { +@Test +fun zeroClusterWeightPreventsExecution() { assertThat(minuteCron.counter).isEqualTo(0) // Cluster weight is 100 by default, so the cron will run. clock.add(Duration.ofMinutes(1)) @@ -125,6 +116,34 @@ class CronTest { } } +@MiskTest(startService = true) +class CronTestPerCluster: AbstractCronTest() { + @Suppress("unused") + @MiskTestModule + val module = object : KAbstractModule() { + override fun configure() { + install(CronTestingModule(CronLeaseBehavior.ONE_LEASE_PER_CLUSTER)) + install(CronEntryModule.create()) + install(CronEntryModule.create()) + install(CronEntryModule.create()) + } + } +} + +@MiskTest(startService = true) +class CronTestPerLease: AbstractCronTest() { + @Suppress("unused") + @MiskTestModule + val module = object : KAbstractModule() { + override fun configure() { + install(CronTestingModule(CronLeaseBehavior.ONE_LEASE_PER_CRON)) + install(CronEntryModule.create()) + install(CronEntryModule.create()) + install(CronEntryModule.create()) + } + } +} + @Singleton @CronPattern("* * * * *") class MinuteCron @Inject constructor() : Runnable { diff --git a/misk-cron/src/test/kotlin/misk/cron/CronTestingModule.kt b/misk-cron/src/test/kotlin/misk/cron/CronTestingModule.kt index 8888b0c8acd..6e9ceafb54f 100644 --- a/misk-cron/src/test/kotlin/misk/cron/CronTestingModule.kt +++ b/misk-cron/src/test/kotlin/misk/cron/CronTestingModule.kt @@ -14,7 +14,7 @@ import misk.tasks.RepeatedTaskQueue import misk.tasks.RepeatedTaskQueueFactory import java.time.ZoneId -class CronTestingModule : KAbstractModule() { +class CronTestingModule @JvmOverloads constructor(private val cronLeaseBehavior: CronLeaseBehavior) : KAbstractModule() { override fun configure() { val applicationModules: List = listOf( FakeLeaseModule(), @@ -23,7 +23,10 @@ class CronTestingModule : KAbstractModule() { MiskTestingServiceModule(), // Cron support requires registering the CronJobHandler and the CronRunnerModule. - FakeCronModule(ZoneId.of("America/Toronto")), + FakeCronModule( + zoneId = ZoneId.of("America/Toronto"), + cronLeaseBehavior = cronLeaseBehavior, + ), ServiceModule() .dependsOn(keyOf(ForMiskCron::class)), ) From cdf5d0743aef4e76750fbcaba3ceb32f12bbc2b5 Mon Sep 17 00:00:00 2001 From: Tyler Gregory Date: Tue, 25 Jun 2024 11:04:06 -0500 Subject: [PATCH 06/33] Remove unnecessary class name sanitization --- misk-cron/src/main/kotlin/misk/cron/CronManager.kt | 4 ++-- misk-cron/src/test/kotlin/misk/cron/CronTest.kt | 12 +----------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt index 3cec5a61866..7eff2e8107b 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt @@ -172,8 +172,8 @@ class CronManager @Inject constructor() { } internal fun buildTaskLease(klass: KClass): Lease { - val sanitizedClassName = klass.qualifiedName!!.replace(".", "-") - val leaseName = "misk-cron-${sanitizedClassName}" + val className = klass.qualifiedName!! + val leaseName = "misk.cron.task.lease.${className}" return leaseManager.requestLease(leaseName) } diff --git a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt index b56e564376f..81613d02e2d 100644 --- a/misk-cron/src/test/kotlin/misk/cron/CronTest.kt +++ b/misk-cron/src/test/kotlin/misk/cron/CronTest.kt @@ -13,8 +13,6 @@ import misk.testing.MiskTest import misk.testing.MiskTestModule import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.Test -import org.junit.jupiter.params.ParameterizedTest -import org.junit.jupiter.params.provider.MethodSource import wisp.time.FakeClock import java.time.Clock import java.time.Duration @@ -75,7 +73,7 @@ abstract class AbstractCronTest { CronLeaseBehavior.ONE_LEASE_PER_CLUSTER -> fakeLeaseManager.markLeaseHeldElsewhere("misk.cron.lease") CronLeaseBehavior.ONE_LEASE_PER_CRON -> - fakeLeaseManager.markLeaseHeldElsewhere("misk-cron-misk-cron-MinuteCron") + fakeLeaseManager.markLeaseHeldElsewhere("misk.cron.task.lease.misk.cron.MinuteCron") } // The lease is already held, we should not execute @@ -106,14 +104,6 @@ fun zeroClusterWeightPreventsExecution() { retry(5, FlatBackoff(Duration.ofMillis(200))) { pendingTasks.peekPending()!! } - - companion object { - @JvmStatic - private fun cronLeaseBehaviorList() = listOf( - CronLeaseBehavior.ONE_LEASE_PER_CRON, - CronLeaseBehavior.ONE_LEASE_PER_CLUSTER, - ) - } } @MiskTest(startService = true) From 4c8d5470e09877921f3a289ecdec941880061196 Mon Sep 17 00:00:00 2001 From: Tyler Gregory Date: Tue, 20 Aug 2024 13:05:26 -0500 Subject: [PATCH 07/33] Lease held elsewhere WIP --- .../main/kotlin/misk/clustering/lease/ClusterAwareLease.kt | 4 ++++ misk-cron/src/main/kotlin/misk/cron/CronManager.kt | 5 +++++ .../src/main/kotlin/wisp/lease/FakeLease.kt | 2 ++ .../src/main/kotlin/wisp/lease/FakeLeaseManager.kt | 2 ++ wisp/wisp-lease/src/main/kotlin/wisp/lease/Lease.kt | 5 +++++ .../src/test/kotlin/wisp/lease/pool/PoolLeaseTest.kt | 6 +++++- 6 files changed, 23 insertions(+), 1 deletion(-) diff --git a/misk-clustering/src/main/kotlin/misk/clustering/lease/ClusterAwareLease.kt b/misk-clustering/src/main/kotlin/misk/clustering/lease/ClusterAwareLease.kt index 9662af1783f..36b97aa64c9 100644 --- a/misk-clustering/src/main/kotlin/misk/clustering/lease/ClusterAwareLease.kt +++ b/misk-clustering/src/main/kotlin/misk/clustering/lease/ClusterAwareLease.kt @@ -25,6 +25,10 @@ class ClusterAwareLease( return (clusterWeightProvider.get() != 0) } + override fun checkHeldElsewhere(): Boolean { + return (clusterWeightProvider.get() == 0) + } + override fun release(): Boolean { return true } diff --git a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt index 7eff2e8107b..c4f3136554d 100644 --- a/misk-cron/src/main/kotlin/misk/cron/CronManager.kt +++ b/misk-cron/src/main/kotlin/misk/cron/CronManager.kt @@ -145,6 +145,11 @@ class CronManager @Inject constructor() { "Last execution was at $previousTime, now=${ZonedDateTime.ofInstant(now, zoneId)}" } removeCompletedCrons() + val clusterLease = leaseManager.requestLease(CRON_CLUSTER_LEASE_NAME) + if (clusterLease.checkHeldElsewhere()) { + logger.info { "Cluster lease is held, skipping cron execution" } + return + } cronEntries.values.forEach { cronEntry -> val taskLease = buildTaskLease(cronEntry.runnable::class) val holdsTaskLease = if (!taskLease.checkHeld()) { diff --git a/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLease.kt b/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLease.kt index 30560f97439..3550aa92fb3 100644 --- a/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLease.kt +++ b/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLease.kt @@ -8,6 +8,8 @@ class FakeLease( override fun checkHeld() = manager.isLeaseHeld(name) + override fun checkHeldElsewhere() = manager.isLeaseHeldElsewhere(name) + /** * @return true if this process acquires the lease. */ diff --git a/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLeaseManager.kt b/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLeaseManager.kt index ada4bfe88ab..0f642e118ee 100644 --- a/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLeaseManager.kt +++ b/wisp/wisp-lease-testing/src/main/kotlin/wisp/lease/FakeLeaseManager.kt @@ -24,6 +24,8 @@ open class FakeLeaseManager : LeaseManager { fun isLeaseHeld(name: String) = !leasesHeldElsewhere.containsKey(name) + fun isLeaseHeldElsewhere(name: String) = leasesHeldElsewhere.containsKey(name) + fun markLeaseHeld(name: String) { leasesHeldElsewhere.remove(name) (requestLease(name) as FakeLease).acquire() diff --git a/wisp/wisp-lease/src/main/kotlin/wisp/lease/Lease.kt b/wisp/wisp-lease/src/main/kotlin/wisp/lease/Lease.kt index ed70c1bba43..df46abecf12 100644 --- a/wisp/wisp-lease/src/main/kotlin/wisp/lease/Lease.kt +++ b/wisp/wisp-lease/src/main/kotlin/wisp/lease/Lease.kt @@ -16,6 +16,11 @@ interface Lease { */ fun checkHeld(): Boolean + /** + * @return true if the lease is owned by another process instance. + */ + fun checkHeldElsewhere(): Boolean + /** * Attempts to acquire the lock on the lease. If the lock was not already held and the lock * was successfully obtained, listeners should be notified. diff --git a/wisp/wisp-lease/src/test/kotlin/wisp/lease/pool/PoolLeaseTest.kt b/wisp/wisp-lease/src/test/kotlin/wisp/lease/pool/PoolLeaseTest.kt index 29a22cf6064..a6b63399b4e 100644 --- a/wisp/wisp-lease/src/test/kotlin/wisp/lease/pool/PoolLeaseTest.kt +++ b/wisp/wisp-lease/src/test/kotlin/wisp/lease/pool/PoolLeaseTest.kt @@ -184,7 +184,11 @@ internal class TestLease( return release } + override fun checkHeldElsewhere(): Boolean { + return !checkHeld + } + override fun addListener(listener: Lease.StateChangeListener) { TODO("Not yet implemented") } -} \ No newline at end of file +} From 7c32ffd509921d932bfa762955fa435e8ffd5c5b Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 6 Aug 2024 09:31:43 +1000 Subject: [PATCH 08/33] 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 8f5b2403ff8204d4d9498b58226393fe80feba84 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 6 Aug 2024 09:44:45 +1000 Subject: [PATCH 09/33] 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 7c600c8ba82569ad52c731fd5b771cf3a02b6205 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 6 Aug 2024 12:11:20 +1000 Subject: [PATCH 10/33] 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 93c6b216c96400fce39c8bb0d1eac68feea9534c 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 11/33] 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 3cdb3e45d2387d5880993b88bb3bcc24ba194e11 Mon Sep 17 00:00:00 2001 From: Giancarlo Salamanca Date: Tue, 6 Aug 2024 19:42:51 -0400 Subject: [PATCH 12/33] 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 f95301e80555f3979b425fd2ae7b5e8d818b02a6 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Wed, 7 Aug 2024 11:58:07 +1000 Subject: [PATCH 13/33] 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 f49a286514ba547966461ef7993afdd98689cbe4 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 14/33] 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 30a98f93f2db47340f1e0041fd750e9a146a10b8 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 15/33] 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 3003e10b789451d25f1d72d60a1d6a10b238baab Mon Sep 17 00:00:00 2001 From: Evan Nelson Date: Wed, 7 Aug 2024 17:00:21 -0700 Subject: [PATCH 16/33] 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 2bdbd1af23dd79a2905e258682f895fd85344752 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 17/33] 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 c8bafb6567b0ae7986e10d59fcd14536cb6bac4b 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 18/33] 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 0b6ac89d547737bfa69d6aec9fdefab14fbbee6f 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 19/33] 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 6b157232334b86abc98f8929a8f8865454cec544 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 20/33] 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 988a5763243119b1362e62302715152f1f9d96e2 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Tue, 13 Aug 2024 10:11:16 +1000 Subject: [PATCH 21/33] 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 2dd10bb99edab2d85c9fb409dd2a8edc32ee2097 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 22/33] 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 c10e0fec1cc1e05e4c11256003aafa4316a30eb4 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Wed, 14 Aug 2024 10:25:17 +1000 Subject: [PATCH 23/33] 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 9cd88a9491d4031a8e01198917d1fdc0625fec64 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 24/33] 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 75ef2b027854c1ef68dc14a3ef5e751fa8ca3e8d Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Fri, 16 Aug 2024 02:10:56 +1000 Subject: [PATCH 25/33] 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 7adc9721b2ca9e55049b578225b5b0b8c8f81b21 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 26/33] 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 9ac94ab29522b60e7ccaec45ae5376a0008e0fec 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 27/33] [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 a722cfbb5545f0b1881c1310235a1b0f0c8c2c36 Mon Sep 17 00:00:00 2001 From: yissachar Date: Fri, 16 Aug 2024 14:47:02 -0400 Subject: [PATCH 28/33] 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 7744ee3420eaa07bf41a52b3cc6335c31f74b190 Mon Sep 17 00:00:00 2001 From: Mehdi Mollaverdi Date: Sat, 17 Aug 2024 11:14:45 +1000 Subject: [PATCH 29/33] 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 d705db145aa4bf463a6db8cc8be0567bbd68c199 Mon Sep 17 00:00:00 2001 From: yissachar Date: Mon, 19 Aug 2024 11:00:54 -0400 Subject: [PATCH 30/33] 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 2d2ec8b5777dc2c3425e3374c99b6d593e668f6c Mon Sep 17 00:00:00 2001 From: yissachar Date: Mon, 19 Aug 2024 14:29:00 -0400 Subject: [PATCH 31/33] 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 99f639a15bbe259753750ef7da9c80090e7e0c60 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 32/33] 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 6d426286f3c1e1365474c7938a810ec4f95e1f31 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 33/33] 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