pst-122 honoring coverage upload intervals smaller than 1h only once

CHANGELOG.md

@@ -5,6 +5,7 @@ We use [semantic versioning](http://semver.org/):
 - PATCH version when you make backwards compatible bug fixes.
 
 # Next version
+- [fix] _agent_: A configured dump `interval` smaller than 1h is now applied only once and then raised to 1h to prevent excessive uploads that can overwhelm Teamscale. A warning is logged at startup when this happens.
 
 # 37.0.1
 - [fix] _agent_: Changed testwise coverage mode to log a warning and finalize an interrupted test as `SKIPPED` (with a calculated duration) if a new test started before the previous `/test/end` event arrived.

agent/src/main/kotlin/com/teamscale/jacoco/agent/Agent.kt

@@ -1,7 +1,6 @@
 package com.teamscale.jacoco.agent
 
 import com.teamscale.client.FileSystemUtils
-import com.teamscale.client.StringUtils
 import com.teamscale.jacoco.agent.logging.LoggingUtils
 import com.teamscale.jacoco.agent.options.AgentOptions
 import com.teamscale.jacoco.agent.upload.IUploadRetry
@@ -17,8 +16,8 @@ import java.io.File
 import java.io.IOException
 import java.lang.instrument.Instrumentation
 import java.nio.file.Files
-import java.util.Timer
-import kotlin.concurrent.fixedRateTimer
+import java.util.*
+import kotlin.concurrent.schedule
 import kotlin.io.path.deleteIfExists
 import kotlin.io.path.listDirectoryEntries
 import kotlin.time.DurationUnit
@@ -33,6 +32,10 @@ class Agent(options: AgentOptions, instrumentation: Instrumentation?) : AgentBas
 	/** Regular dump task.  */
 	private var timer: Timer? = null
 
+	/** Set once the agent is shutting down so no further dumps are rescheduled.  */
+	@Volatile
+	private var stopped = false
+
 	/** Stores the XML files.  */
 	private val uploader = options.createUploader(instrumentation)
 
@@ -41,17 +44,46 @@ class Agent(options: AgentOptions, instrumentation: Instrumentation?) : AgentBas
 		retryUnsuccessfulUploads(options, uploader)
 
 		if (options.shouldDumpInIntervals()) {
-			val period = options.dumpIntervalInMinutes.toDuration(DurationUnit.MINUTES).inWholeMilliseconds
-			timer = fixedRateTimer("Teamscale-Java-Profiler", true, period, period) {
-				dumpReport()
+			timer = Timer("Teamscale-Java-Profiler", true)
+			// Guard against excessively small intervals (see PST-122): first-time users often set a tiny interval to
+			// quickly get an upload and then forget to raise it, flooding Teamscale. We warn right away, honor the
+			// configured interval once, and then raise it to the minimum for all subsequent dumps.
+			if (options.dumpIntervalInMinutes < AgentOptions.MINIMUM_DUMP_INTERVAL_IN_MINUTES) {
+				logger.warn(
+					"You configured an interval smaller than 1h. This causes excessive amounts of data to be sent" +
+							" to Teamscale. Applying this interval only once, then increasing the interval to 1h."
+				)
+			} else {
+				logger.info("Dumping every ${options.dumpIntervalInMinutes} minutes.")
 			}
-			logger.info("Dumping every ${options.dumpIntervalInMinutes} minutes.")
+			scheduleNextDump()
 		}
 		options.teamscaleServer.partition?.let { partition ->
 			controller.sessionId = partition
 		}
 	}
 
+	/**
+	 * Schedules the next interval dump. The configured interval is applied once; when it is below
+	 * [AgentOptions.MINIMUM_DUMP_INTERVAL_IN_MINUTES] it is raised to that minimum after the first dump to avoid
+	 * flooding Teamscale (see PST-122). The warning about this is logged once at startup.
+	 */
+	private fun scheduleNextDump() {
+		if (stopped) return
+		val delayMillis = options.dumpIntervalInMinutes.toDuration(DurationUnit.MINUTES).inWholeMilliseconds
+		try {
+			timer?.schedule(delayMillis) {
+				dumpReport()
+				if (options.dumpIntervalInMinutes < AgentOptions.MINIMUM_DUMP_INTERVAL_IN_MINUTES) {
+					options.dumpIntervalInMinutes = AgentOptions.MINIMUM_DUMP_INTERVAL_IN_MINUTES
+				}
+				scheduleNextDump()
+			}
+		} catch (_: IllegalStateException) {
+			// The timer was cancelled (agent shutting down) between our check and scheduling; nothing left to do.
+		}
+	}
+
 	/**
 	 * If we have coverage that was leftover because of previously unsuccessful coverage uploads, we retry to upload
 	 * them again with the same configuration as in the previous try.
@@ -107,6 +139,7 @@ class Agent(options: AgentOptions, instrumentation: Instrumentation?) : AgentBas
 	}
 
 	override fun prepareShutdown() {
+		stopped = true
 		timer?.cancel()
 		if (options.shouldDumpOnExit) dumpReport()
 

agent/src/main/kotlin/com/teamscale/jacoco/agent/options/AgentOptions.kt

@@ -16,8 +16,6 @@ import com.teamscale.jacoco.agent.commit_resolution.git_properties.GitProperties
 import com.teamscale.jacoco.agent.commit_resolution.git_properties.GitSingleProjectPropertiesLocator
 import com.teamscale.jacoco.agent.commit_resolution.sapnwdi.NwdiMarkerClassLocatingTransformer
 import com.teamscale.jacoco.agent.configuration.ConfigurationViaTeamscale
-import com.teamscale.jacoco.agent.options.AgentOptions.Companion.GIT_PROPERTIES_COMMIT_DATE_FORMAT_OPTION
-import com.teamscale.jacoco.agent.options.AgentOptions.Companion.GIT_PROPERTIES_JAR_OPTION
 import com.teamscale.jacoco.agent.options.sapnwdi.DelayedSapNwdiMultiUploader
 import com.teamscale.jacoco.agent.options.sapnwdi.SapNwdiApplication
 import com.teamscale.jacoco.agent.upload.IUploader
@@ -673,6 +671,12 @@ open class AgentOptions(private val logger: ILogger) {
 		} else teamscaleProxyOptionsForHttps
 
 	companion object {
+		/**
+		 * The smallest allowed value for [dumpIntervalInMinutes] (see PST-122). If a smaller interval is configured,
+		 * it is honored once and then reset to this minimum.
+		 */
+		const val MINIMUM_DUMP_INTERVAL_IN_MINUTES = 60
+
 		/**
 		 * Can be used to format [java.time.LocalDate] to the format "yyyy-MM-dd-HH-mm-ss.SSS"
 		 */

system-tests/dump-interval-too-small-test/build.gradle.kts

@@ -0,0 +1,8 @@
+plugins {
+	com.teamscale.`kotlin-convention`
+	com.teamscale.`system-test-convention`
+}
+
+tasks.test {
+	teamscaleAgent(mapOf("interval" to "1", "debug" to logFilePath))
+}

system-tests/dump-interval-too-small-test/src/main/kotlin/dump/test/SystemUnderTest.kt

@@ -0,0 +1,12 @@
+package dump.test
+
+/**
+ * A minimal system under test. The agent is attached with a too-small dump `interval`, and we only need the JVM to
+ * start so the agent logs its startup warning.
+ */
+object SystemUnderTest {
+	@JvmStatic
+	fun main(args: Array<String>) {
+		// doesn't need to do anything for this test
+	}
+}

system-tests/dump-interval-too-small-test/src/test/kotlin/com/teamscale/client/DumpIntervalTooSmallTest.kt

@@ -0,0 +1,28 @@
+package com.teamscale.client
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Assertions.assertTrue
+import org.junit.jupiter.api.Test
+import java.nio.file.Paths
+import kotlin.io.path.exists
+import kotlin.io.path.readLines
+
+/**
+ * Ensures that the agent warns at startup when a dump `interval` smaller than 1h is configured (see PST-122), so the
+ * user is told that the interval will be applied only once and then raised to 1h.
+ */
+class DumpIntervalTooSmallTest {
+	@Test
+	@Throws(Exception::class)
+	fun systemTest() {
+		assertTrue(LOG_DIRECTORY.exists())
+		val logContent = LOG_DIRECTORY.resolve("teamscale-jacoco-agent.log")
+			.readLines()
+			.joinToString("\n")
+		assertThat(logContent).containsPattern("WARN.*You configured an interval smaller than 1h")
+	}
+
+	companion object {
+		private val LOG_DIRECTORY = Paths.get("logTest").resolve("logs")
+	}
+}