From bbf5c35af0c12b5982fafdc68220137705b6c8c5 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 00:21:37 -0700 Subject: [PATCH 01/10] ADFA-5531: record when each sample was taken The watchers kept values and no times. The chart does not need them -- it reads a sample's age from its position in the buffer -- but the exported metrics file states a time per row, and inferring one from an index would be wrong three ways: the newest sample was taken up to an interval before the export, the sampling loop delays *after* doing its work so its true period runs long, and a series can stop and restart without its buffer being cleared. Not a small error, either. Measured on a Pixel 6 Pro at a nominal 1s rate, the real gap between samples averaged 1.108s and reached 1.758s. Over a full ten-thousand-sample buffer that is eighteen minutes of skew at the old end of the file. One ring buffer per watcher, appended where the values are appended and cleared alongside them, so a zero at an index means nothing was ever sampled there -- which is what will tell an empty cell apart from a measured zero. A per-process "watched since" goes with it: a process can start being watched long after the others, and the Gradle daemon will (ADFA-5514), so its buffer reaches back to the start of the session however late it appeared. MetricsAnnotationStore gains allAnnotations(), since the file carries the whole retained history and recentAnnotations() has no argument meaning "all of it" that does not overflow its cutoff arithmetic. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/utils/MemoryUsageWatcher.kt | 42 +++++++++++++++++++ .../utils/MetricsAnnotationStore.kt | 10 +++++ .../androidide/utils/NetworkUsageWatcher.kt | 30 +++++++++++++ .../androidide/utils/PowerUsageWatcher.kt | 29 +++++++++++++ 4 files changed, 111 insertions(+) diff --git a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt index 229a103e27..498b8451de 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt @@ -58,6 +58,7 @@ class MemoryUsageWatcher updateInterval: Long = DEFAULT_UPDATE_INTERVAL, private val coroutineDispatcher: CoroutineContext = newSingleThreadContext("MemoryUsageWatcher"), private val mainDispatcher: CoroutineContext = Dispatchers.Main.immediate, + private val nowMillis: () -> Long = System::currentTimeMillis, ) { /** * Milliseconds between samples. Changing it clears the history: the chart reads a sample's @@ -84,6 +85,21 @@ class MemoryUsageWatcher private var samplingJob: Job? = null private val memoryUsage = ConcurrentHashMap() + /** + * When each sample was taken, in the same order and at the same indices as the values. + * + * Recorded rather than reconstructed. The chart infers a sample's age from its position, + * which is close enough for placing a marker on a plot, but the exported metrics file states + * a time per row (ADFA-5531) and inference would be wrong three ways: the newest sample was + * taken up to an interval before the export, the loop delays *after* doing its work so the + * true period drifts past the nominal one, and sampling can stop and restart without the + * buffer being cleared. + * + * A zero means no sample was ever recorded at that index, which is what tells a blank cell + * apart from a measured zero. + */ + private val sampleTimes = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + /** * Guards the per-process ring buffers, matching [NetworkUsageWatcher] and * [PowerUsageWatcher]. The sampler appends to them; [clearHistory] wipes them from whatever @@ -196,6 +212,13 @@ class MemoryUsageWatcher return } + // Once per sample, not once per process: every process is read in this one pass, so + // they share a time, and that is what makes a row of the exported file a single moment. + synchronized(historyLock) { + sampleTimes[0] = nowMillis() + sampleTimes.shift(1) + } + val pids = memoryUsage.keys.toIntArray() pids.forEach { pid -> @@ -263,6 +286,11 @@ class MemoryUsageWatcher pid, pname, MutableShiftedLongArray(MAX_USAGE_ENTRIES), + // A process can start being watched long after the others -- the Gradle daemon + // appears when a build does -- and its buffer is zero-filled back to the start + // of the session. Without this, the exported file could not tell those zeros + // from a process that really was using no memory (ADFA-5531). + watchedSinceMillis = nowMillis(), ) } @@ -277,9 +305,21 @@ class MemoryUsageWatcher // so this is reachable, not theoretical. synchronized(historyLock) { memoryUsage.values.forEach { it._history.clear() } + sampleTimes.clear() } } + /** + * When each retained sample was taken, oldest first, as milliseconds since the epoch. + * + * A zero at an index means nothing was ever sampled there -- the buffer is fixed-length and + * starts, and is cleared, full of them. A copy, for the same reason the values are copied. + */ + fun sampleTimes(): LongArray = + synchronized(historyLock) { + sampleTimes.toLongArray() + } + /** * Returns the memory usage of all the registered processes. */ @@ -377,6 +417,8 @@ class MemoryUsageWatcher val pid: Int, val pname: String, internal val _history: MutableShiftedLongArray, + /** When this process started being watched, as milliseconds since the epoch. */ + val watchedSinceMillis: Long = 0L, ) { internal val memInfo: MemoryInfo = MemoryInfo() diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt index fe70af6b2a..0aba44c264 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsAnnotationStore.kt @@ -181,6 +181,16 @@ class MetricsAnnotationStore( return annotations.filter { it.atMillis >= cutoff } } + /** + * Every annotation the store holds, oldest first. + * + * The exported metrics file carries the whole retained history rather than a window of it, so + * it cannot go through [recentAnnotations] -- there is no "within" that means "all of it" + * without the cutoff arithmetic overflowing (ADFA-5531). + */ + @Synchronized + fun allAnnotations(): List = annotations.toList() + @Synchronized fun clear() { annotations.clear() diff --git a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt index 784f13d286..95aa4e1fb3 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt @@ -68,6 +68,7 @@ class NetworkUsageWatcher // touching Dispatchers.Main at construction throws in a plain JVM test, and most of these // tests never start the sampling loop at all. private val mainDispatcher: CoroutineContext? = null, + private val nowMillis: () -> Long = System::currentTimeMillis, ) { private val coroutineScope = CoroutineScope(SupervisorJob() + coroutineDispatcher) private val watching = AtomicBoolean(false) @@ -103,6 +104,21 @@ class NetworkUsageWatcher /** Guards the two ring buffers: the sampler writes them, the UI thread snapshots them. */ private val historyLock = Any() + /** + * When each sample was taken, in the same order and at the same indices as the values. + * + * Recorded rather than reconstructed. The chart infers a sample's age from its position, + * which is close enough for placing a marker on a plot, but the exported metrics file states + * a time per row (ADFA-5531) and inference would be wrong three ways: the newest sample was + * taken up to an interval before the export, the loop delays *after* doing its work so the + * true period drifts past the nominal one, and sampling can stop and restart without the + * buffer being cleared. + * + * A zero means no sample was ever recorded at that index, which is what tells a blank cell + * apart from a measured zero. + */ + private val sampleTimes = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + private val received = MutableShiftedLongArray(MAX_USAGE_ENTRIES) private val transmitted = MutableShiftedLongArray(MAX_USAGE_ENTRIES) @@ -143,6 +159,17 @@ class NetworkUsageWatcher NetworkUsage(received.toLongArray(), transmitted.toLongArray()) } + /** + * When each retained sample was taken, oldest first, as milliseconds since the epoch. + * + * A zero at an index means nothing was ever sampled there -- the buffer is fixed-length and + * starts, and is cleared, full of them. A copy, for the same reason the values are copied. + */ + fun sampleTimes(): LongArray = + synchronized(historyLock) { + sampleTimes.toLongArray() + } + /** * Discards every recorded sample and drops the cumulative baseline, so the next sample * re-establishes it rather than reporting everything since the last one as one huge delta. @@ -151,6 +178,7 @@ class NetworkUsageWatcher synchronized(historyLock) { received.clear() transmitted.clear() + sampleTimes.clear() lastRx = null lastTx = null } @@ -265,6 +293,8 @@ class NetworkUsageWatcher // and the second block then put the pre-reset values straight back, so the next // sample counted traffic from before the change. synchronized(historyLock) { + sampleTimes[0] = nowMillis() + sampleTimes.shift(1) record(received, previous = lastRx, current = rx) record(transmitted, previous = lastTx, current = tx) lastRx = rx diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index 4753e9e730..212b16511b 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -58,6 +58,7 @@ class PowerUsageWatcher private val source: PowerSource, private val coroutineDispatcher: CoroutineContext = newSingleThreadContext("PowerUsageWatcher"), private val mainDispatcher: CoroutineContext = Dispatchers.Main.immediate, + private val nowMillis: () -> Long = System::currentTimeMillis, ) { private val coroutineScope = CoroutineScope(SupervisorJob() + coroutineDispatcher) private val watching = AtomicBoolean(false) @@ -75,6 +76,21 @@ class PowerUsageWatcher /** Guards the ring buffers: the sampler writes them, the UI thread snapshots them. */ private val historyLock = Any() + /** + * When each sample was taken, in the same order and at the same indices as the values. + * + * Recorded rather than reconstructed. The chart infers a sample's age from its position, + * which is close enough for placing a marker on a plot, but the exported metrics file states + * a time per row (ADFA-5531) and inference would be wrong three ways: the newest sample was + * taken up to an interval before the export, the loop delays *after* doing its work so the + * true period drifts past the nominal one, and sampling can stop and restart without the + * buffer being cleared. + * + * A zero means no sample was ever recorded at that index, which is what tells a blank cell + * apart from a measured zero. + */ + private val sampleTimes = MutableShiftedLongArray(MAX_USAGE_ENTRIES) + private val temperature = MutableShiftedLongArray(MAX_USAGE_ENTRIES) private val power = MutableShiftedLongArray(MAX_USAGE_ENTRIES) @@ -128,8 +144,20 @@ class PowerUsageWatcher PowerUsage(temperature.toLongArray(), power.toLongArray(), thermal.toLongArray()) } + /** + * When each retained sample was taken, oldest first, as milliseconds since the epoch. + * + * A zero at an index means nothing was ever sampled there -- the buffer is fixed-length and + * starts, and is cleared, full of them. A copy, for the same reason the values are copied. + */ + fun sampleTimes(): LongArray = + synchronized(historyLock) { + sampleTimes.toLongArray() + } + fun clearHistory() { synchronized(historyLock) { + sampleTimes.clear() temperature.clear() power.clear() thermal.clear() @@ -195,6 +223,7 @@ class PowerUsageWatcher latestBattery = reading.battery synchronized(historyLock) { + append(sampleTimes, nowMillis()) append(temperature, reading.temperatureMilliCelsius) append(power, reading.powerMicroWatts) append(thermal, reading.thermalStatus.toLong()) From 73930561c99d56a5b04eccda5c5b3f30ba28db7e Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 00:21:58 -0700 Subject: [PATCH 02/10] ADFA-5531: define the canonical metrics file, and name every export the same way The format section of the ticket is the single definition of this file, and ADFA-5494, ADFA-5526 and ADFA-5534 each produce or consume it -- they differ only in compressing it. So it lives in one place, with no Android types, and the whole format is tested without a device. A row is a sampling tick. Its stated time is the memory watcher's, recorded when it sampled rather than reconstructed, and the network and power values on that row are the samples at the same index, taken within one interval of it. The three watchers share an interval, are started together and are cleared together, which is what makes the index mean the same thing in all three; they do not read their sources at the same instant, and the file says so rather than implying otherwise. Decisions the ticket left open: - The row timestamp is ISO 8601 with the device's offset, which round-trips exactly for ADFA-5494 and stays legible to someone triaging a report from another timezone. Written with an explicit three-digit fraction rather than ISO_OFFSET_DATE_TIME, which drops trailing zeros and gives a column of varying width -- ".38" on one row and ".123" on the next. - The filename keeps its own format, deliberately different: rule (c) is built from what a filesystem allows and what sorts lexicographically. - An export always produces a file. With nothing sampled it is a header and no rows, which a consumer can handle more easily than a file that may or may not exist -- and the empty case is not exotic, since changing the sampling rate clears every buffer. - The memory columns are fixed at the three the chart can plot rather than taken from what is being watched, because that set changes during a session and a header that followed it would describe a different file each time. Item (2) of the ticket is the same rule applied to the image: the PNG used to lead with the chart's title, which sorted a pair exported at the same moment apart. Both writers now take an injectable clock, which is also what lets a test write distinguishable files without racing the millisecond the name is built from. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../com/itsaky/androidide/utils/MetricsCsv.kt | 251 +++++++++++++++ .../itsaky/androidide/utils/MetricsCsvFile.kt | 79 +++++ .../androidide/utils/MetricsFileName.kt | 50 +++ .../androidide/utils/MetricsSnapshot.kt | 31 +- .../itsaky/androidide/utils/MetricsCsvTest.kt | 299 ++++++++++++++++++ .../androidide/utils/MetricsFileNameTest.kt | 65 ++++ .../androidide/utils/MetricsSnapshotTest.kt | 55 ++-- 7 files changed, 778 insertions(+), 52 deletions(-) create mode 100644 app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt create mode 100644 app/src/main/java/com/itsaky/androidide/utils/MetricsCsvFile.kt create mode 100644 app/src/main/java/com/itsaky/androidide/utils/MetricsFileName.kt create mode 100644 app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt create mode 100644 app/src/test/java/com/itsaky/androidide/utils/MetricsFileNameTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt new file mode 100644 index 0000000000..c52891e086 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt @@ -0,0 +1,251 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import java.time.Instant +import java.time.ZoneId +import java.time.format.DateTimeFormatter +import java.util.Locale +import kotlin.math.abs + +/** + * The canonical metrics file: everything the carousel has sampled, as CSV (ADFA-5531). + * + * One definition, because the file has several producers and consumers -- the export button here, + * ADFA-5494's restore across process death, and the copies ADFA-5526 and ADFA-5534 attach to crash + * reports and to feedback. Those differ from this only in compressing it. + * + * A row is a sampling tick, and its columns come from three watchers that each keep their own ring + * buffer and their own coroutine. They are started together and share one interval, and every one + * of them is cleared when that interval changes, so the tick at index *i* is the same tick in all + * three -- but they do not read their sources at the same instant. The row's stated time is the + * memory watcher's, recorded when it sampled; the network and power values on that row were taken + * within one interval of it. Nothing here reconstructs a time from an index. + * + * Formatting only, with no Android types, so the whole format can be tested without a device. + */ +object MetricsCsv { + /** Media type for the written file, for the sharing intent. */ + const val MIME_TYPE = "text/csv" + + /** + * A sample time of zero means no sample was taken at that index: the ring buffers are + * fixed-length and start, and are cleared, full of zeros. + */ + const val NO_SAMPLE = 0L + + /** + * A row's time, ISO 8601 with the offset the device was on. + * + * Deliberately not the filename's format, which is built from what a filesystem allows and what + * sorts lexicographically. This one has to round-trip exactly for ADFA-5494 and be read by a + * person triaging a report from another timezone, which is what the offset is for. + * + * Spelled out rather than [DateTimeFormatter.ISO_OFFSET_DATE_TIME], which drops trailing zeros + * from the fraction and so writes a column of varying width -- ".38" for one row and ".123" for + * the next. Both parse, but a fixed three digits matches the millisecond the value is recorded + * at and the three the filename carries. + */ + private val TIMESTAMP_FORMAT: DateTimeFormatter = + DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX", Locale.ROOT) + + /** + * The memory series, in column order. + * + * Fixed rather than taken from whatever is being watched at export time. The set changes during + * a session -- the Gradle daemon appears when a build starts and goes when it exits (ADFA-5514) + * -- and a header that depended on it would describe a different file each time. A process that + * is not being watched leaves its column empty. + */ + val MEMORY_COLUMNS = listOf("IDE", "Gradle Tooling", "Gradle Daemon") + + @JvmStatic + val HEADER: List = + listOf("timestamp") + + MEMORY_COLUMNS.map { "${it.lowercase().replace(' ', '_')}_pss_bytes" } + + listOf( + "net_rx_bytes", + "net_tx_bytes", + "battery_temp_millicelsius", + "power_microwatts", + "thermal_status", + "annotation", + "annotation_kind", + ) + + /** + * One series of samples and the times they were recorded at. + * + * @property times When each value was sampled, oldest first and parallel to [values]. A + * [NO_SAMPLE] entry marks an index nothing was ever recorded at, which is what tells an empty + * cell apart from a measured zero. + * @property values The samples themselves. + * @property since When this series started being recorded. Samples timed before it belong to + * the buffer's zero-filled past rather than to this series -- the Gradle daemon's buffer reaches + * back to the start of the session however late in it the daemon appeared. + */ + class Series( + private val times: LongArray, + private val values: LongArray, + private val since: Long = 0L, + ) { + /** The value at index [i], or `null` if this series has nothing to say there. */ + fun at(i: Int): Long? { + if (i < 0 || i >= times.size || i >= values.size) { + return null + } + val time = times[i] + return if (time == NO_SAMPLE || time < since) null else values[i] + } + + companion object { + val EMPTY = Series(LongArray(0), LongArray(0)) + } + } + + /** + * @property atMillis When the event happened. + * @property label Its text, already resolved. + * @property kind The sort of event, as the annotation store names it. + */ + data class Marker( + val atMillis: Long, + val label: String, + val kind: String, + ) + + /** + * Everything one export writes. + * + * @property rowTimes The memory watcher's sample times, oldest first. They are the rows, because + * memory is the one series always being recorded. + */ + class Snapshot( + val rowTimes: LongArray, + val memory: Map, + val networkReceived: Series = Series.EMPTY, + val networkTransmitted: Series = Series.EMPTY, + val temperature: Series = Series.EMPTY, + val power: Series = Series.EMPTY, + val thermal: Series = Series.EMPTY, + val annotations: List = emptyList(), + ) + + /** + * Writes [snapshot] to [out], timestamps in [zone]. + * + * The header is always written, even when nothing has been sampled. A file that exists and + * reports no rows is easier for a consumer to handle than one that may or may not be there, and + * the empty case is not exotic: changing the sampling rate clears every buffer. + */ + fun write( + snapshot: Snapshot, + zone: ZoneId, + out: Appendable, + ) { + out.append(HEADER.joinToString(",", transform = ::quote)).append('\n') + + val markerRows = markerRows(snapshot) + val row = StringBuilder() + snapshot.rowTimes.forEachIndexed { i, at -> + if (at == NO_SAMPLE) { + return@forEachIndexed + } + + row.setLength(0) + row.append(quote(formatTime(at, zone))) + MEMORY_COLUMNS.forEach { name -> row.append(',').append(number(snapshot.memory[name]?.at(i))) } + row.append(',').append(number(snapshot.networkReceived.at(i))) + row.append(',').append(number(snapshot.networkTransmitted.at(i))) + row.append(',').append(number(snapshot.temperature.at(i))) + row.append(',').append(number(snapshot.power.at(i))) + row.append(',').append(number(snapshot.thermal.at(i))) + val marker = markerRows[i] + row.append(',').append(marker?.let { quote(it.label) } ?: "") + row.append(',').append(marker?.let { quote(it.kind) } ?: "") + out.append(row).append('\n') + } + } + + /** + * An annotation's time, moved onto the clock the samples are stamped with. + * + * [MetricsAnnotationStore] records on [android.os.SystemClock.elapsedRealtime], which is + * monotonic and immune to the wall clock being set, and is what the chart wants: it only ever + * asks how long ago something happened. A file has to say *when*, so the samples carry epoch + * milliseconds, and the two cannot be compared without this. + * + * Mixing them is not a small error. A monotonic time is a few hours since boot and an epoch time + * is decades, so every row looks about equally far from the marker and the nearest-row search + * lands on whichever row has the smallest number -- the oldest one in the buffer, every time. + * + * @param monotonicAtMillis The time the store recorded. + * @param nowEpochMillis Now, on the samples' clock. + * @param nowMonotonicMillis Now, on the store's clock. Read as close together as possible. + */ + fun epochFor( + monotonicAtMillis: Long, + nowEpochMillis: Long, + nowMonotonicMillis: Long, + ): Long = monotonicAtMillis + (nowEpochMillis - nowMonotonicMillis) + + /** [atMillis] as ISO 8601 in [zone]. */ + fun formatTime( + atMillis: Long, + zone: ZoneId, + ): String = TIMESTAMP_FORMAT.format(Instant.ofEpochMilli(atMillis).atZone(zone)) + + /** + * The row each marker belongs on, resolved once for the whole file. + * + * A marker goes on the row whose sample is nearest it in time. An annotation is recorded when + * something happened, not when a sample was taken, so requiring an exact match would drop + * almost all of them; and doing this per row rather than once would walk every marker against + * every row, which at ten thousand of each is not a cost worth paying for a button. + * + * Where two markers land on one row the earlier wins, and the later is dropped rather than + * silently overwriting it -- the file has one annotation column per row by definition. + */ + private fun markerRows(snapshot: Snapshot): Map { + if (snapshot.annotations.isEmpty()) { + return emptyMap() + } + + val sampled = snapshot.rowTimes.withIndex().filter { it.value != NO_SAMPLE } + if (sampled.isEmpty()) { + return emptyMap() + } + + val rows = mutableMapOf() + snapshot.annotations.sortedBy { it.atMillis }.forEach { marker -> + val nearest = sampled.minByOrNull { abs(it.value - marker.atMillis) } ?: return@forEach + rows.putIfAbsent(nearest.index, marker) + } + return rows + } + + private fun number(value: Long?): String = value?.toString() ?: "" + + /** + * A CSV string cell. + * + * Quoted per the format's rule (b), with any quote inside it doubled -- a task name is text the + * IDE was given, and nothing guarantees it has no quotes in it. + */ + private fun quote(value: String): String = "\"" + value.replace("\"", "\"\"") + "\"" +} diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsvFile.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsvFile.kt new file mode 100644 index 0000000000..81310f3182 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsvFile.kt @@ -0,0 +1,79 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import android.content.Context +import androidx.annotation.VisibleForTesting +import org.slf4j.LoggerFactory +import java.io.File +import java.io.IOException +import java.time.ZoneId + +/** + * Writes a [MetricsCsv.Snapshot] to a file the IDE can share (ADFA-5531). + * + * The same scratch-directory arrangement as [MetricsSnapshot], and for the same reason: exports go + * under the cache so the platform can reclaim them, and the sharing intent grants the recipient a + * read on the file before that matters. + */ +object MetricsCsvFile { + private val log = LoggerFactory.getLogger(MetricsCsvFile::class.java) + + private const val DIRECTORY = "metrics-exports" + + /** + * How many exports to keep. + * + * A share hands the recipient a URI and returns long before the recipient reads it, so the + * previous file cannot be deleted on the next export. Fewer than the images are kept: a full + * buffer is around a megabyte of text, against a few hundred kilobytes for a PNG. + */ + @VisibleForTesting + internal const val KEEP_RECENT = 3 + + /** + * Writes [snapshot] and returns the file, or `null` if it could not be written. + */ + fun write( + context: Context, + snapshot: MetricsCsv.Snapshot, + nowMillis: Long = System.currentTimeMillis(), + zone: ZoneId = ZoneId.systemDefault(), + ): File? { + val directory = File(context.cacheDir, DIRECTORY) + return try { + if (!directory.exists() && !directory.mkdirs()) { + log.error("Could not create the metrics export directory at {}", directory) + return null + } + + val file = File(directory, MetricsFileName.forTime(nowMillis, "csv", zone)) + // Buffered and streamed rather than built into a string: a full buffer is ten thousand + // rows, and holding the whole file in memory to write it is a megabyte of char array + // the export does not need. + file.bufferedWriter().use { writer -> + MetricsCsv.write(snapshot, zone, writer) + } + MetricsSnapshot.pruneTo(directory, KEEP_RECENT, file) + file + } catch (io: IOException) { + log.error("Could not write the metrics export", io) + null + } + } +} diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsFileName.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsFileName.kt new file mode 100644 index 0000000000..97a13b3419 --- /dev/null +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsFileName.kt @@ -0,0 +1,50 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import java.time.Instant +import java.time.ZoneId +import java.time.format.DateTimeFormatter +import java.util.Locale + +/** + * What every exported metrics file is called (ADFA-5531). + * + * One rule for the CSV and the chart image alike, differing only in extension, so a pair exported + * together sorts together and a consumer can tell when a file was written without opening it. + * + * Underscores and no offset, which is what makes it a filename rather than a timestamp: it has to + * survive every filesystem the IDE can write to and sort lexicographically in a directory listing. + * The times *inside* the file are ISO 8601 -- see [MetricsCsv]. + */ +object MetricsFileName { + /** `YYYY_MM_DD_HH_MM_SS_SSS`, as the format section of ADFA-5531 specifies it. */ + private val PATTERN: DateTimeFormatter = + DateTimeFormatter.ofPattern("yyyy_MM_dd_HH_mm_ss_SSS", Locale.ROOT) + + /** + * The name for a file written at [atMillis], with [extension] and no leading dot. + * + * Local time, because this is the name a person reads in a share sheet or a file manager. + */ + fun forTime( + atMillis: Long, + extension: String, + zone: ZoneId = ZoneId.systemDefault(), + ): String = "${PATTERN.format(Instant.ofEpochMilli(atMillis).atZone(zone))}.$extension" +} diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt index 8f1f07d0b5..7159482843 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt @@ -23,9 +23,6 @@ import androidx.annotation.VisibleForTesting import org.slf4j.LoggerFactory import java.io.File import java.io.IOException -import java.text.SimpleDateFormat -import java.util.Date -import java.util.Locale /** * Writes a metrics chart image to a file the IDE can share (ADFA-5486). @@ -47,13 +44,16 @@ object MetricsSnapshot { */ @VisibleForTesting internal const val KEEP_RECENT = 5 - private const val TIMESTAMP_PATTERN = "yyyyMMdd-HHmmss" /** Media type for the written file, for the sharing intent. */ const val MIME_TYPE = "image/png" /** - * Writes [bitmap] as a PNG named after [label] and the current time. + * Writes [bitmap] as a PNG, named by [MetricsFileName] like every other exported metrics file. + * + * The name used to lead with the chart's title. ADFA-5531 made one naming rule for the image and + * the CSV so that a pair exported together sorts together, and a title in front of the timestamp + * would have sorted them apart. * * A few recent snapshots are kept rather than only the newest. This is a scratch directory for * handing an image to another app, not a gallery, so it stays bounded -- but a share hands the @@ -66,7 +66,7 @@ object MetricsSnapshot { fun write( context: Context, bitmap: Bitmap, - label: String, + nowMillis: Long = System.currentTimeMillis(), ): File? { val directory = File(context.cacheDir, DIRECTORY) return try { @@ -75,7 +75,7 @@ object MetricsSnapshot { return null } - val file = File(directory, "${fileNameFor(label)}.png") + val file = File(directory, MetricsFileName.forTime(nowMillis, "png")) file.outputStream().use { output -> if (!bitmap.compress(Bitmap.CompressFormat.PNG, QUALITY, output)) { log.error("Could not encode the chart snapshot") @@ -97,7 +97,7 @@ object MetricsSnapshot { * trusted to sort newest: two exports in the same second share a timestamp, and the filename * carries only whole seconds. */ - private fun pruneTo( + internal fun pruneTo( directory: File, limit: Int, newest: File, @@ -112,19 +112,4 @@ object MetricsSnapshot { } } } - - /** - * A filename from [label] and the current time, with anything that is not safe in a filename - * replaced. Chart titles are translated, so they can contain spaces and non-ASCII. - */ - private fun fileNameFor(label: String): String { - val stamp = SimpleDateFormat(TIMESTAMP_PATTERN, Locale.US).format(Date()) - val safeLabel = - label - .lowercase(Locale.US) - .replace(Regex("[^a-z0-9]+"), "-") - .trim('-') - .ifEmpty { "metrics" } - return "$safeLabel-$stamp" - } } diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt new file mode 100644 index 0000000000..a546d023cd --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt @@ -0,0 +1,299 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 +import java.time.ZoneId + +/** + * The canonical metrics file format (ADFA-5531). + * + * Pinned closely because it is not this ticket's file alone: ADFA-5494 reads it back to restore the + * chart history, and ADFA-5526 and ADFA-5534 attach copies to crash reports and to feedback. A + * column that quietly changes shape breaks a consumer that is not in front of you. + */ +@RunWith(JUnit4::class) +class MetricsCsvTest { + private val zone: ZoneId = ZoneId.of("America/Los_Angeles") + + private fun render(snapshot: MetricsCsv.Snapshot): List = + StringBuilder() + .also { MetricsCsv.write(snapshot, zone, it) } + .toString() + .trimEnd('\n') + .split('\n') + + private fun snapshot( + rowTimes: LongArray = longArrayOf(T0, T0 + 1_000L), + memory: Map = emptyMap(), + networkReceived: MetricsCsv.Series = MetricsCsv.Series.EMPTY, + networkTransmitted: MetricsCsv.Series = MetricsCsv.Series.EMPTY, + temperature: MetricsCsv.Series = MetricsCsv.Series.EMPTY, + power: MetricsCsv.Series = MetricsCsv.Series.EMPTY, + thermal: MetricsCsv.Series = MetricsCsv.Series.EMPTY, + annotations: List = emptyList(), + ) = MetricsCsv.Snapshot( + rowTimes = rowTimes, + memory = memory, + networkReceived = networkReceived, + networkTransmitted = networkTransmitted, + temperature = temperature, + power = power, + thermal = thermal, + annotations = annotations, + ) + + private fun series( + times: LongArray, + values: LongArray, + since: Long = 0L, + ) = MetricsCsv.Series(times, values, since) + + @Test + fun `an export with nothing sampled is a header and no rows`() { + // The empty case is not exotic: changing the sampling rate clears every buffer, so the very + // next export has nothing to say. It still produces a file. + val lines = render(snapshot(rowTimes = LongArray(4))) + + assertThat(lines).hasSize(1) + assertThat(lines.single()).isEqualTo(MetricsCsv.HEADER.joinToString(",") { "\"$it\"" }) + } + + @Test + fun `a row's time is the one recorded for that sample`() { + val lines = render(snapshot(rowTimes = longArrayOf(T0))) + + // Read back, not reconstructed from an index and an interval: 2026-09-06T22:33:40.123 in + // Los Angeles, with the offset that says which 22:33 it was. + assertThat(lines[1]).startsWith("\"2026-09-06T22:33:40.123-07:00\"") + } + + @Test + fun `the fraction is always three digits, even when it ends in zero`() { + // ISO_OFFSET_DATE_TIME drops trailing zeros and would write ".38" here, giving a column of + // varying width. Both parse; only one lines up with the milliseconds the filename carries. + val lines = render(snapshot(rowTimes = longArrayOf(T0 - 43L))) + + assertThat(lines[1]).startsWith("\"2026-09-06T22:33:40.080-07:00\"") + } + + @Test + fun `an index nothing was sampled at is not a row`() { + // The buffers are fixed-length and start full of zeros, so most of a young session's buffer + // has never been written. Those are absent rows, not rows of zeros. + val lines = render(snapshot(rowTimes = longArrayOf(0L, 0L, T0, 0L, T0 + 1_000L))) + + assertThat(lines).hasSize(3) + assertThat(lines[1]).contains("22:33:40.123") + assertThat(lines[2]).contains("22:33:41.123") + } + + @Test + fun `every row has as many cells as the header`() { + val times = longArrayOf(T0, T0 + 1_000L) + val lines = + render( + snapshot( + rowTimes = times, + memory = mapOf("IDE" to series(times, longArrayOf(1L, 2L))), + networkReceived = series(times, longArrayOf(3L, 4L)), + annotations = listOf(MetricsCsv.Marker(T0, "assemble", "TASK")), + ), + ) + + lines.forEach { line -> + assertThat(cellsIn(line)).hasSize(MetricsCsv.HEADER.size) + } + } + + @Test + fun `a process that was not being watched yet leaves the cell empty, not zero`() { + val times = longArrayOf(T0, T0 + 1_000L) + val lines = + render( + snapshot( + rowTimes = times, + // The Gradle daemon appears when a build starts, and its buffer is zero-filled + // back to the beginning of the session (ADFA-5514). Reporting those zeros as + // measurements would say the daemon was running and using nothing. + memory = mapOf("Gradle Daemon" to series(times, longArrayOf(0L, 900L), since = T0 + 1_000L)), + ), + ) + + val daemon = MetricsCsv.HEADER.indexOf("gradle_daemon_pss_bytes") + assertThat(cellsIn(lines[1])[daemon]).isEmpty() + assertThat(cellsIn(lines[2])[daemon]).isEqualTo("900") + } + + @Test + fun `a series that never recorded leaves its columns empty`() { + val times = longArrayOf(T0, T0 + 1_000L) + val lines = + render( + snapshot( + rowTimes = times, + memory = mapOf("IDE" to series(times, longArrayOf(5L, 6L))), + // A device whose traffic counters are unsupported never records a sample, and + // zero bytes transferred is a different statement from no measurement. + networkReceived = MetricsCsv.Series.EMPTY, + ), + ) + + val rx = MetricsCsv.HEADER.indexOf("net_rx_bytes") + assertThat(cellsIn(lines[1])[rx]).isEmpty() + } + + @Test + fun `strings are quoted, numbers are not, and a quote inside one is doubled`() { + val times = longArrayOf(T0) + val lines = + render( + snapshot( + rowTimes = times, + memory = mapOf("IDE" to series(times, longArrayOf(7L))), + annotations = listOf(MetricsCsv.Marker(T0, ":app:say \"hi\"", "TASK")), + ), + ) + + val cells = cellsIn(lines[1]) + assertThat(cells[MetricsCsv.HEADER.indexOf("ide_pss_bytes")]).isEqualTo("7") + assertThat(cells[MetricsCsv.HEADER.indexOf("annotation")]).isEqualTo("\":app:say \"\"hi\"\"\"") + assertThat(cells[MetricsCsv.HEADER.indexOf("annotation_kind")]).isEqualTo("\"TASK\"") + } + + @Test + fun `an annotation lands on the sample nearest in time, not only an exact match`() { + val times = longArrayOf(T0, T0 + 1_000L, T0 + 2_000L) + val lines = + render( + snapshot( + rowTimes = times, + // Recorded when the build started, which is between two samples. Requiring an + // exact match would drop nearly every marker in the file. + annotations = listOf(MetricsCsv.Marker(T0 + 1_600L, "Build started", "BUILD_STARTED")), + ), + ) + + val column = MetricsCsv.HEADER.indexOf("annotation") + assertThat(cellsIn(lines[1])[column]).isEmpty() + assertThat(cellsIn(lines[2])[column]).isEmpty() + assertThat(cellsIn(lines[3])[column]).isEqualTo("\"Build started\"") + } + + @Test + fun `two annotations falling on one sample keep the earlier one`() { + val times = longArrayOf(T0) + val lines = + render( + snapshot( + rowTimes = times, + annotations = + listOf( + MetricsCsv.Marker(T0 + 40L, "second", "TASK"), + MetricsCsv.Marker(T0 + 10L, "first", "TASK"), + ), + ), + ) + + // One annotation column per row by definition, so the loser is dropped rather than + // overwriting the winner or being appended into the same cell. + assertThat(cellsIn(lines[1])[MetricsCsv.HEADER.indexOf("annotation")]).isEqualTo("\"first\"") + } + + @Test + fun `an annotation recorded on the monotonic clock lands on the right row`() { + // The store stamps annotations with elapsedRealtime and the samples carry epoch millis. + // Recorded three seconds ago, on a device up for two hours. + val upFor = 2 * 60 * 60 * 1000L + val recordedAt = upFor - 3_000L + val times = longArrayOf(T0, T0 + 1_000L, T0 + 2_000L, T0 + 3_000L) + val onEpoch = MetricsCsv.epochFor(recordedAt, nowEpochMillis = T0 + 3_000L, nowMonotonicMillis = upFor) + + val lines = + render( + snapshot( + rowTimes = times, + annotations = listOf(MetricsCsv.Marker(onEpoch, "Build started", "BUILD_STARTED")), + ), + ) + + val column = MetricsCsv.HEADER.indexOf("annotation") + assertThat(cellsIn(lines[1])[column]).isEqualTo("\"Build started\"") + } + + @Test + fun `an unconverted monotonic time would land on the oldest row`() { + // What the mix-up looked like on a device: a monotonic time is a few hours and an epoch time + // is decades, so every row is about equally far away and the nearest-row search picks + // whichever number is smallest -- the oldest sample, whenever the event really happened. + val times = longArrayOf(T0, T0 + 1_000L, T0 + 2_000L) + val lines = + render( + snapshot( + rowTimes = times, + annotations = listOf(MetricsCsv.Marker(2 * 60 * 60 * 1000L, "Build started", "BUILD_STARTED")), + ), + ) + + val column = MetricsCsv.HEADER.indexOf("annotation") + assertThat(cellsIn(lines[1])[column]).isEqualTo("\"Build started\"") + assertThat(cellsIn(lines[3])[column]).isEmpty() + } + + @Test + fun `the memory columns are the three the chart can plot`() { + // Fixed, not derived from what is being watched: the set changes mid-session, and a header + // that followed it would describe a different file each time. + assertThat(MetricsCsv.MEMORY_COLUMNS).containsExactly("IDE", "Gradle Tooling", "Gradle Daemon").inOrder() + assertThat(MetricsCsv.HEADER).containsAtLeast("ide_pss_bytes", "gradle_tooling_pss_bytes", "gradle_daemon_pss_bytes") + } + + /** Splits a row on commas that are not inside a quoted cell. */ + private fun cellsIn(line: String): List { + val cells = mutableListOf() + val cell = StringBuilder() + var quoted = false + line.forEach { c -> + when { + c == '"' -> { + quoted = !quoted + cell.append(c) + } + + c == ',' && !quoted -> { + cells += cell.toString() + cell.setLength(0) + } + + else -> { + cell.append(c) + } + } + } + cells += cell.toString() + return cells + } + + private companion object { + /** 2026-09-06T22:33:40.123 in America/Los_Angeles, which is UTC-7 at that date. */ + const val T0 = 1_788_759_220_123L + } +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsFileNameTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsFileNameTest.kt new file mode 100644 index 0000000000..fd98e4ffbf --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsFileNameTest.kt @@ -0,0 +1,65 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.junit.runners.JUnit4 +import java.time.ZoneId + +/** The one name every exported metrics file gets (ADFA-5531's rule (c)). */ +@RunWith(JUnit4::class) +class MetricsFileNameTest { + private val zone: ZoneId = ZoneId.of("America/Los_Angeles") + + @Test + fun `the name is the local time to the millisecond, and the extension`() { + val name = MetricsFileName.forTime(T0, "csv", zone) + + assertThat(name).isEqualTo("2026_09_06_22_33_40_123.csv") + } + + @Test + fun `the image and the data exported at one moment differ only in extension`() { + // Which is the point of rule (c): a pair exported together sorts together, and neither + // leads with a chart title that would sort them apart. + val csv = MetricsFileName.forTime(T0, "csv", zone) + val png = MetricsFileName.forTime(T0, "png", zone) + + assertThat(csv.removeSuffix(".csv")).isEqualTo(png.removeSuffix(".png")) + } + + @Test + fun `names sort in the order the files were written`() { + val earlier = MetricsFileName.forTime(T0, "csv", zone) + val later = MetricsFileName.forTime(T0 + 1L, "csv", zone) + val muchLater = MetricsFileName.forTime(T0 + 86_400_000L, "csv", zone) + + // A directory listing is sorted lexicographically, so the format has to be too -- which is + // why it is fixed-width and big-endian rather than anything friendlier to read. + assertThat(listOf(muchLater, later, earlier).sorted()) + .containsExactly(earlier, later, muchLater) + .inOrder() + } + + private companion object { + /** 2026-09-06T22:33:40.123 in America/Los_Angeles. */ + const val T0 = 1_788_759_220_123L + } +} diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsSnapshotTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsSnapshotTest.kt index 53ca31b780..613d699915 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MetricsSnapshotTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsSnapshotTest.kt @@ -27,8 +27,10 @@ import org.robolectric.RobolectricTestRunner import java.io.File /** - * Pins ADFA-5486's snapshot export: a chart becomes a PNG in the cache, named after the chart, with - * only the newest one kept. + * Pins ADFA-5486's snapshot export: a chart becomes a PNG in the cache, with a few recent ones kept. + * + * The name used to lead with the chart's title. ADFA-5531 gave the image and the CSV one naming rule + * so a pair exported together sorts together, which is what the naming tests here now pin. */ @RunWith(RobolectricTestRunner::class) class MetricsSnapshotTest { @@ -38,7 +40,7 @@ class MetricsSnapshotTest { @Test fun `writes a png into the cache`() { - val file = MetricsSnapshot.write(context, bitmap(), "Memory usage") + val file = MetricsSnapshot.write(context, bitmap()) assertThat(file).isNotNull() assertThat(file!!.exists()).isTrue() @@ -49,54 +51,39 @@ class MetricsSnapshotTest { } @Test - fun `names the file after the chart`() { - val file = MetricsSnapshot.write(context, bitmap(), "Network traffic") + fun `the name is the shared metrics naming rule`() { + val file = MetricsSnapshot.write(context, bitmap(), AT) - assertThat(file!!.name).startsWith("network-traffic-") - } - - @Test - fun `a title with punctuation or non-ascii still makes a usable filename`() { - // Chart titles are translated, so they are not guaranteed to be filename-safe. - val file = MetricsSnapshot.write(context, bitmap(), "Mémoire / usage (MB)") - - assertThat(file).isNotNull() - assertThat(file!!.name).matches("[a-z0-9-]+\\.png") - } - - @Test - fun `a title with nothing usable still produces a file`() { - val file = MetricsSnapshot.write(context, bitmap(), "***") - - assertThat(file).isNotNull() - assertThat(file!!.name).startsWith("metrics-") + // The same name the CSV exported at that moment would get, differing only in extension -- + // no chart title in front of it to sort the pair apart (ADFA-5531). + assertThat(file!!.name).isEqualTo(MetricsFileName.forTime(AT, "png")) } @Test fun `a shared snapshot survives the next few exports`() { - val shared = MetricsSnapshot.write(context, bitmap(), "Memory usage")!! + val shared = MetricsSnapshot.write(context, bitmap(), AT)!! // A share hands the recipient a FileProvider URI and the chooser returns long before the // recipient opens it. Deleting the previous file on the next export pulled the image out // from under an app that had not read it yet. - repeat(3) { index -> MetricsSnapshot.write(context, bitmap(), "Chart $index") } + repeat(3) { index -> MetricsSnapshot.write(context, bitmap(), AT + index + 1L) } assertThat(shared.exists()).isTrue() } @Test fun `the directory stays bounded across many exports`() { - repeat(20) { index -> MetricsSnapshot.write(context, bitmap(), "Chart $index") } + repeat(20) { index -> MetricsSnapshot.write(context, bitmap(), AT + index) } // Bounded, not unbounded: this is a scratch directory, not a gallery. - val directory = MetricsSnapshot.write(context, bitmap(), "Last")!!.parentFile!! + val directory = MetricsSnapshot.write(context, bitmap(), AT + 100L)!!.parentFile!! assertThat(directory.listFiles()!!.size).isAtMost(MetricsSnapshot.KEEP_RECENT) } @Test fun `the newest snapshot is the one handed back, and it is on disk`() { - MetricsSnapshot.write(context, bitmap(), "Memory usage") - val newest = MetricsSnapshot.write(context, bitmap(), "Network traffic") + MetricsSnapshot.write(context, bitmap(), AT) + val newest = MetricsSnapshot.write(context, bitmap(), AT + 1L) // This used to assert that the previous file was gone. It is not, deliberately: a share // can still be reading it. What has to hold is that the file returned exists and is in @@ -105,4 +92,14 @@ class MetricsSnapshotTest { assertThat(newest!!.exists()).isTrue() assertThat(newest.parentFile).isEqualTo(File(context.cacheDir, "metrics-snapshots")) } + + private companion object { + /** + * A fixed export time. + * + * The name carries milliseconds, so two writes in the same millisecond would be one file. + * Real exports are a tap apart; a test loop is not. + */ + const val AT = 1_788_759_220_123L + } } From 3ba8bbcc8ae41a23f66f77d0fc086846dab6ca13 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 00:22:17 -0700 Subject: [PATCH 03/10] ADFA-5531: add the export button to the carousel A spreadsheet icon to the left of the camera, sharing its corner and its touch target. Both are exports, and every other gesture over the chart is already spoken for -- paging, the two-finger tap that undocks, pinch to zoom, and a tap on the x axis for the sampling rate. It exports the whole buffer, not the visible window and not the page on screen: this is the file the format is defined around, and its other consumers want everything there is. Assembly happens on the UI thread because it is snapshots of the watchers' buffers; formatting and writing happen off it, because ten thousand rows is not a click listener's work. The annotations needed converting, not just copying. MetricsAnnotationStore records on SystemClock.elapsedRealtime, which is monotonic and is what the chart wants -- it only ever asks how long ago something happened -- while the samples carry epoch milliseconds. Comparing the two directly is not a small error: a monotonic time is a few hours since boot and an epoch time is decades, so every row looks about equally far from a marker and the nearest-row search lands on whichever number is smallest, which is the oldest row in the buffer every time. On a device the build marker appeared four minutes before the build. The conversion is a pure function with the failure mode pinned by a test of its own. The button carries a long-press help tag like every other control in the carousel, and its Tier 1 and Tier 2 text is written into the documentation database. ADFA-5513 still owes it a Tier 3 destination, as it does the other twelve; that ticket has been told. Verified on a Pixel 6 Pro. A tap writes 2026_09_07_00_20_09_926.csv and opens the share sheet; the image exported beside it differs only in extension. Against a real build: sixty-five rows, timestamps a second apart and irregular as recorded, "Build started" on the row 1.3s after the Run tap, a task marker carrying the Gradle task's own name, and "Build finished" nineteen seconds later. The Gradle daemon column is present and empty, which is correct until ADFA-5514 lands. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../ui/MetricsCarouselController.kt | 119 +++++++++++++++++- app/src/main/res/drawable/ic_spreadsheet.xml | 25 ++++ app/src/main/res/layout/layout_mem_usage.xml | 14 +++ .../androidide/idetooltips/TooltipTag.kt | 1 + resources/src/main/res/values/strings.xml | 2 + 5 files changed, 160 insertions(+), 1 deletion(-) create mode 100644 app/src/main/res/drawable/ic_spreadsheet.xml diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index c948fb6216..7f3d4db9c7 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -22,6 +22,7 @@ import android.content.Context import android.content.ContextWrapper import android.content.Intent import android.content.res.ColorStateList +import android.os.SystemClock import android.util.TypedValue import android.view.View import android.view.ViewGroup @@ -46,6 +47,8 @@ import com.itsaky.androidide.utils.DialogUtils import com.itsaky.androidide.utils.IntentUtils import com.itsaky.androidide.utils.MemoryUsageWatcher import com.itsaky.androidide.utils.MetricsAnnotationStore +import com.itsaky.androidide.utils.MetricsCsv +import com.itsaky.androidide.utils.MetricsCsvFile import com.itsaky.androidide.utils.MetricsSamplingRates import com.itsaky.androidide.utils.MetricsSnapshot import com.itsaky.androidide.utils.NetworkUsageWatcher @@ -253,6 +256,7 @@ class MetricsCarouselController( // A camera button in the graph's bottom-right corner exports the chart. The gestures over // the chart are all spoken for, so this is a control rather than another gesture. binding.metricsSnapshot.setOnClickListener { exportSnapshot() } + binding.metricsExport.setOnClickListener { exportCsv() } // Arrows are the dependable way to move between pages: a swipe has to share the gesture // with panning a zoomed chart and with the editor's drawer, and loses often enough to be @@ -308,6 +312,7 @@ class MetricsCarouselController( binding.metricsPrevious to TooltipTag.CAROUSEL_PREVIOUS, binding.metricsNext to TooltipTag.CAROUSEL_NEXT, binding.metricsSnapshot to TooltipTag.CAROUSEL_SNAPSHOT, + binding.metricsExport to TooltipTag.CAROUSEL_EXPORT, binding.metricsBattery to TooltipTag.CAROUSEL_BATTERY, // Wired even though it is only visible while undocked: the message is the one control // that outlives unbind(), so its help must not be torn down with the rest. @@ -334,6 +339,7 @@ class MetricsCarouselController( networkRenderer.onXAxisTap = null powerRenderer.onXAxisTap = null binding?.metricsSnapshot?.setOnClickListener(null) + binding?.metricsExport?.setOnClickListener(null) binding?.let { bound -> helpTargets(bound) // All but the undocked message: that view becomes visible *because* the carousel @@ -617,7 +623,7 @@ class MetricsCarouselController( // fresh full-size ARGB_8888 copy of the plot on every tap, which is // megabytes that would otherwise sit around until the collector noticed. try { - MetricsSnapshot.write(appContext, bitmap, label) + MetricsSnapshot.write(appContext, bitmap) } finally { bitmap.recycle() } @@ -651,6 +657,117 @@ class MetricsCarouselController( return true } + /** + * Writes every retained sample to a CSV file and offers it to another app (ADFA-5531). + * + * The whole buffer, not the visible window and not the current page: this is the file the + * metrics format is defined around, and ADFA-5494, ADFA-5526 and ADFA-5534 all want everything + * there is. Assembled on the UI thread because it is snapshots of the watchers' buffers, then + * formatted and written off it -- ten thousand rows is not a click listener's work. + * + * @return whether an export could be started. The write itself completes later. + */ + @UiThread + fun exportCsv(): Boolean { + val binding = this.binding ?: return false + if (exportInFlight) { + log.debug("Ignoring an export request while one is already being written") + return false + } + + val context = binding.root.context + val appContext = context.applicationContext + val snapshot = snapshot() + exportInFlight = true + scope.launch { + // Guarded for the same reason exportSnapshot is: the scope has no exception handler, so + // anything escaping here is filed as a crash. + runCatching { + val file = withContext(Dispatchers.IO) { MetricsCsvFile.write(appContext, snapshot) } + val host = this@MetricsCarouselController.binding?.root?.context + if (file == null || host == null) { + Toast.makeText(appContext, string.msg_metrics_export_failed, Toast.LENGTH_SHORT).show() + return@runCatching + } + val extraFlags = + if (host.findActivityOrNull() == null) Intent.FLAG_ACTIVITY_NEW_TASK else 0 + IntentUtils.shareFile(host, file, MetricsCsv.MIME_TYPE, extraFlags) + }.onFailure { failure -> + if (failure is CancellationException) { + exportInFlight = false + throw failure + } + log.error("Could not share the metrics export", failure) + Toast.makeText(appContext, string.msg_metrics_export_failed, Toast.LENGTH_SHORT).show() + } + exportInFlight = false + } + return true + } + + /** + * The watchers' buffers, as the export format's view of them. + * + * Rows come from the memory watcher: it is the only one always recording, the network watcher + * stops for good on a device whose counters are unsupported, and a power source can be missing. + * The other series are read at the same index -- the watchers share an interval, are started + * together and are cleared together -- and each carries its own sample times, so a series that + * was not recording leaves empty cells rather than zeros. + */ + @UiThread + @VisibleForTesting + internal fun snapshot(): MetricsCsv.Snapshot { + val memoryTimes = memoryUsageWatcher.sampleTimes() + val networkTimes = networkUsageWatcher.sampleTimes() + val powerTimes = powerUsageWatcher.sampleTimes() + val network = networkUsageWatcher.getUsage() + val power = powerUsageWatcher.getUsage() + + return MetricsCsv.Snapshot( + rowTimes = memoryTimes, + memory = + memoryUsageWatcher.getMemoryUsages().associate { process -> + process.pname to + MetricsCsv.Series( + times = memoryTimes, + values = process.usageHistory.toLongArray(), + since = process.watchedSinceMillis, + ) + }, + networkReceived = MetricsCsv.Series(networkTimes, network.received), + networkTransmitted = MetricsCsv.Series(networkTimes, network.transmitted), + temperature = MetricsCsv.Series(powerTimes, power.temperatureMilliCelsius), + power = MetricsCsv.Series(powerTimes, power.powerMicroWatts), + thermal = MetricsCsv.Series(powerTimes, power.thermalStatus), + annotations = markers(), + ) + } + + /** + * The annotations, with their times moved onto the clock the samples carry. + * + * The store records on the monotonic clock and the samples on the wall clock, and the two are + * read here as close together as they can be so the offset between them is the right one. + */ + private fun markers(): List { + val store = annotations ?: return emptyList() + val nowEpoch = System.currentTimeMillis() + val nowMonotonic = SystemClock.elapsedRealtime() + return store.allAnnotations().map { annotation -> + MetricsCsv.Marker( + atMillis = MetricsCsv.epochFor(annotation.atMillis, nowEpoch, nowMonotonic), + label = labelFor(annotation), + kind = annotation.kind.name, + ) + } + } + + /** An annotation's text: a build outcome carries a string id, a task carries its own name. */ + private fun labelFor(annotation: MetricsAnnotationStore.Annotation): String { + val context = binding?.root?.context ?: return annotation.label + return annotation.kind.labelRes?.let(context::getString) ?: annotation.label + } + /** * Releases the controller for good. Distinct from [unbind], which runs on every dock, undock * and recreation; this is the terminal teardown and cancels any snapshot still being written. diff --git a/app/src/main/res/drawable/ic_spreadsheet.xml b/app/src/main/res/drawable/ic_spreadsheet.xml new file mode 100644 index 0000000000..db16b3dd48 --- /dev/null +++ b/app/src/main/res/drawable/ic_spreadsheet.xml @@ -0,0 +1,25 @@ + + + + + + + + + diff --git a/app/src/main/res/layout/layout_mem_usage.xml b/app/src/main/res/layout/layout_mem_usage.xml index f267ae6466..aee15c1e87 100644 --- a/app/src/main/res/layout/layout_mem_usage.xml +++ b/app/src/main/res/layout/layout_mem_usage.xml @@ -88,6 +88,20 @@ app:layout_constraintBottom_toBottomOf="@id/metrics_pager" app:layout_constraintEnd_toEndOf="@id/metrics_pager" /> + + + Next metric Save chart image Couldn\'t save the chart image. + Save metrics data + Couldn\'t save the metrics data. Received Sent From 3e3869b8839632722373055ccf9b15f1d0ed86a7 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 05:20:44 -0700 Subject: [PATCH 04/10] ADFA-5531: read a row's time and its values as one moment The export asked each watcher for its sample times and then for its values, in two separate locked calls. A sample landing between them shifts every value one index against the times, so each row of the file carries its neighbour's timestamp -- for the whole buffer, not just the newest row. That is the one property a row of this file has. Two halves, because both sides had a window: The memory sampler appended the time in one critical section and each process's value in another, so a reader could catch a buffer holding one more timestamp than values. It now reads every process first, off the lock, and appends the time and all the values together. The reflective PSS read becomes an injectable reader, matching the other watchers' readRxBytes/readTxBytes, which is also what lets a test read from inside a sample. Readers get one call per watcher. MemoryUsageWatcher.history() returns times and per-process values from one critical section; the network and power sample times ride on the NetworkUsage and PowerUsage they were recorded with. The times-only accessors are gone, so the window cannot be reintroduced by asking for the halves separately. The test reads from inside a sample and fails without the fix with one timestamp against zero values. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../ui/MetricsCarouselController.kt | 25 ++-- .../androidide/utils/MemoryUsageWatcher.kt | 120 +++++++++++------ .../androidide/utils/NetworkUsageWatcher.kt | 30 +++-- .../androidide/utils/PowerUsageWatcher.kt | 29 ++-- .../MemoryUsageWatcherSampleAlignmentTest.kt | 124 ++++++++++++++++++ 5 files changed, 250 insertions(+), 78 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index 7f3d4db9c7..d2e130ad02 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -717,28 +717,29 @@ class MetricsCarouselController( @UiThread @VisibleForTesting internal fun snapshot(): MetricsCsv.Snapshot { - val memoryTimes = memoryUsageWatcher.sampleTimes() - val networkTimes = networkUsageWatcher.sampleTimes() - val powerTimes = powerUsageWatcher.sampleTimes() + // One call per watcher, not one per array. Each returns its times and its values from a + // single critical section, which is what keeps a row of the file a single moment: two calls + // let the sampler append between them and every value came out one row off its timestamp. + val memory = memoryUsageWatcher.history() val network = networkUsageWatcher.getUsage() val power = powerUsageWatcher.getUsage() return MetricsCsv.Snapshot( - rowTimes = memoryTimes, + rowTimes = memory.times, memory = - memoryUsageWatcher.getMemoryUsages().associate { process -> + memory.processes.associate { process -> process.pname to MetricsCsv.Series( - times = memoryTimes, - values = process.usageHistory.toLongArray(), + times = memory.times, + values = process.usage, since = process.watchedSinceMillis, ) }, - networkReceived = MetricsCsv.Series(networkTimes, network.received), - networkTransmitted = MetricsCsv.Series(networkTimes, network.transmitted), - temperature = MetricsCsv.Series(powerTimes, power.temperatureMilliCelsius), - power = MetricsCsv.Series(powerTimes, power.powerMicroWatts), - thermal = MetricsCsv.Series(powerTimes, power.thermalStatus), + networkReceived = MetricsCsv.Series(network.sampleTimes, network.received), + networkTransmitted = MetricsCsv.Series(network.sampleTimes, network.transmitted), + temperature = MetricsCsv.Series(power.sampleTimes, power.temperatureMilliCelsius), + power = MetricsCsv.Series(power.sampleTimes, power.powerMicroWatts), + thermal = MetricsCsv.Series(power.sampleTimes, power.thermalStatus), annotations = markers(), ) } diff --git a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt index 498b8451de..eeb47d03ce 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt @@ -20,6 +20,7 @@ package com.itsaky.androidide.utils import android.app.ActivityManager import android.os.Debug import android.os.Debug.MemoryInfo +import androidx.annotation.VisibleForTesting import androidx.collection.IntObjectMap import androidx.collection.MutableIntObjectMap import androidx.core.content.getSystemService @@ -59,6 +60,17 @@ class MemoryUsageWatcher private val coroutineDispatcher: CoroutineContext = newSingleThreadContext("MemoryUsageWatcher"), private val mainDispatcher: CoroutineContext = Dispatchers.Main.immediate, private val nowMillis: () -> Long = System::currentTimeMillis, + // Injectable for the same reason the other watchers' readers are: it is the one part of a + // sample that needs a device. ActivityManager.getProcessMemoryInfo is rate-limited and + // internally uses Debug.getMemoryInfo, so the reflective call goes around the limit. + private val readTotalPssKb: (Int, MemoryInfo) -> Int = { pid, into -> + ReflectionUtils.invokeMethod(android_os_Debug_getMemoryInfo, null, pid, into) + + // From https://developer.android.com/tools/dumpsys#meminfo + // "PSS is a good measure for the actual RAM weight of a process and for comparison + // against the RAM use of other processes and the total available RAM." + into.totalPss + }, ) { /** * Milliseconds between samples. Changing it clears the history: the chart reads a sample's @@ -199,7 +211,8 @@ class MemoryUsageWatcher } } - private fun readUsages() { + @VisibleForTesting + internal fun readUsages() { if (memoryUsage.isEmpty()) { // Nothing to sample. Returning before the service lookup keeps an idle watcher off // BaseApplication, which a unit test does not have. @@ -212,49 +225,35 @@ class MemoryUsageWatcher return } - // Once per sample, not once per process: every process is read in this one pass, so - // they share a time, and that is what makes a row of the exported file a single moment. - synchronized(historyLock) { - sampleTimes[0] = nowMillis() - sampleTimes.shift(1) - } - + // Read every process first, append nothing yet. The reading is the slow part and must + // not hold the lock; the append is the part a reader can see, and all of it -- the time + // and every process's value -- has to land in one critical section. A reader that + // caught the time appended but not the values got a file whose every row sat on its + // neighbour's timestamp, which is the one thing a row of this file is for (ADFA-5531). + val at = nowMillis() val pids = memoryUsage.keys.toIntArray() + val sampled = ArrayList>(pids.size) pids.forEach { pid -> - - // ActivityManager.getProcessMemoryInfo is rate-limited - // but it internally uses Debug.getMemoryInfo to get the memory info - // we use it directly using reflection to bypass the rate limit val proc = memoryUsage[pid] ?: run { log.warn("Process {} is not being watched, but readUsages() was called for the process", pid) return@forEach } - ReflectionUtils.invokeMethod(android_os_Debug_getMemoryInfo, null, pid, proc.memInfo) - - // From https://developer.android.com/tools/dumpsys#meminfo - // "PSS is a good measure for the actual RAM weight of a process and for comparison against - // the RAM use of other processes and the total available RAM." - val usage = proc.memInfo.totalPss - // values are in kB, convert to bytes - val usageBytes = usage * 1024L - memoryUsage[pid]!!.apply { - // we insert the usage entry at the start of the array, then increment the shift amount by 1 - // this makes the newly inserted usage entry the last element in the array - // and the oldest usage entry the first element in the array - - // this means that _history[_history.size - 1] will be the newest usage entry - - // the "shift" amount basically indicates what is the start index of the array - // for example, if shift is 1, then _history[0] will actually return _history[1] (index shifted by 1 to the right) - // when the shift amount exceeds the size of the array, it will be reset to 0 (wrapped around) + sampled += proc to readTotalPssKb(pid, proc.memInfo) * 1024L + } - synchronized(historyLock) { - _history[0] = usageBytes - _history.shift(1) - } + synchronized(historyLock) { + // The entry goes in at the start of the array and the shift amount goes up by one, + // which makes it the last element and the oldest the first -- so + // _history[_history.size - 1] is always the newest. The shift is the array's start + // index, wrapping back to 0 once it passes the end. + sampleTimes[0] = at + sampleTimes.shift(1) + sampled.forEach { (proc, usageBytes) -> + proc._history[0] = usageBytes + proc._history.shift(1) } } } @@ -310,14 +309,31 @@ class MemoryUsageWatcher } /** - * When each retained sample was taken, oldest first, as milliseconds since the epoch. + * Every retained sample, with the times the samples were taken at. * - * A zero at an index means nothing was ever sampled there -- the buffer is fixed-length and - * starts, and is cleared, full of them. A copy, for the same reason the values are copied. + * One lock around the whole read, and it has to be: asking for the times and the values + * separately let the sampler append between the two calls, which shifts every value one + * index against its timestamp and puts each row of the exported file on its neighbour's + * time (ADFA-5531). There is no accessor for the times alone, deliberately. + * + * A zero time at an index means nothing was ever sampled there -- the buffers are + * fixed-length and start, and are cleared, full of them. Copies, for the same reason the + * values have always been copied. */ - fun sampleTimes(): LongArray = + fun history(): MemoryHistory = synchronized(historyLock) { - sampleTimes.toLongArray() + MemoryHistory( + times = sampleTimes.toLongArray(), + processes = + memoryUsage.values.map { proc -> + ProcessHistory( + pid = proc.pid, + pname = proc.pname, + usage = proc._history.toLongArray(), + watchedSinceMillis = proc.watchedSinceMillis, + ) + }, + ) } /** @@ -393,6 +409,32 @@ class MemoryUsageWatcher (coroutineDispatcher as? ExecutorCoroutineDispatcher)?.close() } + /** + * One process's retained samples, detached from the watcher. + * + * @property usage The samples, oldest first, in bytes. + * @property watchedSinceMillis When this process started being watched. Its buffer reaches + * back to the start of the session however late in it the process appeared, and this is what + * tells those zeros from a measurement. + */ + class ProcessHistory( + val pid: Int, + val pname: String, + val usage: LongArray, + val watchedSinceMillis: Long, + ) + + /** + * Every watched process's samples and the times they were taken at, read together. + * + * @property times When each sample was taken, oldest first, as milliseconds since the epoch, + * parallel to every entry in [processes]. + */ + class MemoryHistory( + val times: LongArray, + val processes: List, + ) + /** * Registers a listener to be notified when the memory usage of a process changes. */ diff --git a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt index 95aa4e1fb3..2166a87d74 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt @@ -156,18 +156,7 @@ class NetworkUsageWatcher */ fun getUsage(): NetworkUsage = synchronized(historyLock) { - NetworkUsage(received.toLongArray(), transmitted.toLongArray()) - } - - /** - * When each retained sample was taken, oldest first, as milliseconds since the epoch. - * - * A zero at an index means nothing was ever sampled there -- the buffer is fixed-length and - * starts, and is cleared, full of them. A copy, for the same reason the values are copied. - */ - fun sampleTimes(): LongArray = - synchronized(historyLock) { - sampleTimes.toLongArray() + NetworkUsage(received.toLongArray(), transmitted.toLongArray(), sampleTimes.toLongArray()) } /** @@ -332,20 +321,33 @@ class NetworkUsageWatcher * * @property received Bytes received during each interval. * @property transmitted Bytes transmitted during each interval. + * @property sampleTimes When each sample was taken, oldest first, as milliseconds since the + * epoch, parallel to the values. Read in the same critical section as them, because reading + * the two separately let the sampler append between the calls and shifted every value one + * index against its timestamp (ADFA-5531). A zero means nothing was ever sampled at that + * index -- the buffers are fixed-length and start, and are cleared, full of them. Defaulted + * empty for the chart, which asks only how long ago a sample was and never when. */ data class NetworkUsage( val received: LongArray, val transmitted: LongArray, + val sampleTimes: LongArray = LongArray(0), ) { override fun equals(other: Any?): Boolean = this === other || ( other is NetworkUsage && received.contentEquals(other.received) && - transmitted.contentEquals(other.transmitted) + transmitted.contentEquals(other.transmitted) && + sampleTimes.contentEquals(other.sampleTimes) ) - override fun hashCode(): Int = 31 * received.contentHashCode() + transmitted.contentHashCode() + override fun hashCode(): Int { + var result = received.contentHashCode() + result = 31 * result + transmitted.contentHashCode() + result = 31 * result + sampleTimes.contentHashCode() + return result + } } fun interface NetworkUsageListener { diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index 212b16511b..3c38817837 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -141,18 +141,12 @@ class PowerUsageWatcher */ fun getUsage(): PowerUsage = synchronized(historyLock) { - PowerUsage(temperature.toLongArray(), power.toLongArray(), thermal.toLongArray()) - } - - /** - * When each retained sample was taken, oldest first, as milliseconds since the epoch. - * - * A zero at an index means nothing was ever sampled there -- the buffer is fixed-length and - * starts, and is cleared, full of them. A copy, for the same reason the values are copied. - */ - fun sampleTimes(): LongArray = - synchronized(historyLock) { - sampleTimes.toLongArray() + PowerUsage( + temperature.toLongArray(), + power.toLongArray(), + thermal.toLongArray(), + sampleTimes.toLongArray(), + ) } fun clearHistory() { @@ -285,11 +279,18 @@ class PowerUsageWatcher * @property temperatureMilliCelsius Battery temperature per sample. * @property powerMicroWatts Instantaneous draw per sample. * @property thermalStatus Throttling level per sample, for the chart's shading. + * @property sampleTimes When each sample was taken, oldest first, as milliseconds since the + * epoch, parallel to the values. Read in the same critical section as them, because reading + * the two separately let the sampler append between the calls and shifted every value one + * index against its timestamp (ADFA-5531). A zero means nothing was ever sampled at that + * index -- the buffers are fixed-length and start, and are cleared, full of them. Defaulted + * empty for the chart, which asks only how long ago a sample was and never when. */ data class PowerUsage( val temperatureMilliCelsius: LongArray, val powerMicroWatts: LongArray, val thermalStatus: LongArray, + val sampleTimes: LongArray = LongArray(0), ) { override fun equals(other: Any?): Boolean = this === other || @@ -297,13 +298,15 @@ class PowerUsageWatcher other is PowerUsage && temperatureMilliCelsius.contentEquals(other.temperatureMilliCelsius) && powerMicroWatts.contentEquals(other.powerMicroWatts) && - thermalStatus.contentEquals(other.thermalStatus) + thermalStatus.contentEquals(other.thermalStatus) && + sampleTimes.contentEquals(other.sampleTimes) ) override fun hashCode(): Int { var result = temperatureMilliCelsius.contentHashCode() result = 31 * result + powerMicroWatts.contentHashCode() result = 31 * result + thermalStatus.contentHashCode() + result = 31 * result + sampleTimes.contentHashCode() return result } } diff --git a/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt new file mode 100644 index 0000000000..035e57cf68 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt @@ -0,0 +1,124 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * That a row of the exported metrics file is one moment (ADFA-5531). + * + * The exported file states a time per row, and every value on that row has to be the one recorded + * at it. The sampler runs on its own thread and the export reads from the UI thread, so the only + * thing making that true is where the sampler's appends happen and how many calls the reader makes. + */ +@RunWith(RobolectricTestRunner::class) +class MemoryUsageWatcherSampleAlignmentTest { + private var clock = 1_700_000_000_000L + + private fun watcher(readPssKb: (Int, android.os.Debug.MemoryInfo) -> Int = { _, _ -> PSS_KB }) = + MemoryUsageWatcher( + nowMillis = { + clock += TICK_MILLIS + clock + }, + readTotalPssKb = readPssKb, + ) + + @Test + fun `a read taken during a sample sees times and values that agree`() { + lateinit var watcher: MemoryUsageWatcher + var midSample: MemoryUsageWatcher.MemoryHistory? = null + + // Read from inside the sample, which is the interleaving the sampling thread and the UI + // thread can produce for real. Appending the time and the values in separate critical + // sections left this window: the reader caught a buffer with one more timestamp in it than + // values, so every value in the exported file sat on the row below its own timestamp. + watcher = + watcher { _, _ -> + if (midSample == null) { + midSample = watcher.history() + } + PSS_KB + } + watcher.watchProcess(PID, "IDE") + + watcher.readUsages() + watcher.readUsages() + + val history = checkNotNull(midSample) + val stamped = history.times.count { it != MetricsCsv.NO_SAMPLE } + val measured = + history.processes + .single() + .usage + .count { it != 0L } + assertThat(measured).isEqualTo(stamped) + } + + @Test + fun `a completed sample stamps every process with the same time`() { + val watcher = watcher() + watcher.watchProcess(PID, "IDE") + watcher.watchProcess(OTHER_PID, "Gradle Daemon") + + watcher.readUsages() + + // The two processes are read one after the other, but they belong to one row, so they share + // its time -- and each has exactly one value against it. + val history = watcher.history() + assertThat(history.times.count { it != MetricsCsv.NO_SAMPLE }).isEqualTo(1) + history.processes.forEach { process -> + assertThat(process.usage.count { it != 0L }).isEqualTo(1) + } + } + + @Test + fun `the times come back with the values, not from a call of their own`() { + val watcher = watcher() + watcher.watchProcess(PID, "IDE") + watcher.readUsages() + + // The guard on the fix above: one accessor, so a caller cannot reintroduce the window by + // asking for the halves separately. There is deliberately no times-only accessor. + val history = watcher.history() + assertThat(history.times).hasLength(MemoryUsageWatcher.MAX_USAGE_ENTRIES) + assertThat(history.processes.single().usage).hasLength(MemoryUsageWatcher.MAX_USAGE_ENTRIES) + assertThat(history.times.last()).isNotEqualTo(MetricsCsv.NO_SAMPLE) + assertThat( + history.processes + .single() + .usage + .last(), + ).isEqualTo(PSS_KB * 1024L) + } + + private companion object { + const val PID = 4242 + + const val OTHER_PID = 4243 + + /** Any non-zero reading; the test counts measured samples rather than reading values. */ + const val PSS_KB = 512 + + /** Enough that no two sample times collide. */ + const val TICK_MILLIS = 1_000L + } +} From 53bdaa8cf40c13fabaab8806c0afbdc515cbb098 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 07:00:14 -0700 Subject: [PATCH 05/10] ADFA-5531: spell the header out, instead of deriving it from the code The header test built its expectation with MetricsCsv.HEADER.joinToString, from the same constant MetricsCsv.write builds the file from -- so it asserted only that the code agrees with itself. Renaming a column, reordering one, or changing how cells are quoted would have renamed the expectation too and stayed green. It is one literal line now. ADFA-5494 reads this format back and ADFA-5526 and ADFA-5534 ship it inside reports, so a schema change should have to come and edit it on purpose. Verified: renaming a memory column now fails this test, where before it did not. Agreed with review on #1799 and not carried out at the time. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../com/itsaky/androidide/utils/MetricsCsvTest.kt | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt index a546d023cd..f44619461a 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt @@ -74,7 +74,7 @@ class MetricsCsvTest { val lines = render(snapshot(rowTimes = LongArray(4))) assertThat(lines).hasSize(1) - assertThat(lines.single()).isEqualTo(MetricsCsv.HEADER.joinToString(",") { "\"$it\"" }) + assertThat(lines.single()).isEqualTo(EXPECTED_HEADER) } @Test @@ -293,6 +293,18 @@ class MetricsCsvTest { } private companion object { + /** + * The header line, spelled out rather than derived from [MetricsCsv.HEADER]. + * + * This is the file's contract, and a test that builds its expectation from the same constant + * the code builds the file from asserts only that the code is self-consistent -- a renamed + * column or a change in how cells are quoted would rename it here too and stay green. + * ADFA-5494 reads this format back, and ADFA-5526 and ADFA-5534 ship it inside reports, so a + * schema change should have to come and edit this line on purpose. + */ + const val EXPECTED_HEADER = + "\"timestamp\",\"ide_pss_bytes\",\"gradle_tooling_pss_bytes\",\"gradle_daemon_pss_bytes\",\"net_rx_bytes\",\"net_tx_bytes\",\"battery_temp_millicelsius\",\"power_microwatts\",\"thermal_status\",\"annotation\",\"annotation_kind\"" + /** 2026-09-06T22:33:40.123 in America/Los_Angeles, which is UTC-7 at that date. */ const val T0 = 1_788_759_220_123L } From 31e10df3a1ea33c70e6998de7ad9761ec633029b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 07:31:40 -0700 Subject: [PATCH 06/10] ADFA-5531: stop the file reporting sentinels and a lost watch time Two review findings, both making the exported file say something untrue while staying perfectly well-formed. ProcessMemoryInfo.snapshot dropped watchedSinceMillis, and getMemoryUsages hands out snapshots. Defaulted to 0 it reads as "watched since the epoch", so the guard that blanks a process's zero-filled past never fired and the Gradle daemon exported measured zeros for the whole session before the daemon existed. Latent as of the row-alignment commit, which moved the export onto history(); the trap is removed rather than left for the next getMemoryUsages consumer. PowerUsageWatcher stores Long.MIN_VALUE for a reading the platform will not give, and the writer stringified it -- so battery_temp_millicelsius and power_microwatts carried -9223372036854775808. The chart maps that to zero; the file did not. Series gains an optional `absent` sentinel written as an empty cell, which is what the format already means by "nothing to say here". The caller names the value, because MetricsCsv deliberately has no Android types in it. thermal_status gets the same treatment for THERMAL_UNKNOWN. Both tests fail against the unfixed code. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../ui/MetricsCarouselController.kt | 23 +++++++++++++--- .../androidide/utils/MemoryUsageWatcher.kt | 7 ++++- .../com/itsaky/androidide/utils/MetricsCsv.kt | 14 +++++++++- .../MemoryUsageWatcherSampleAlignmentTest.kt | 15 +++++++++++ .../itsaky/androidide/utils/MetricsCsvTest.kt | 27 +++++++++++++++++++ 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index d2e130ad02..8709b5a8f9 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -737,9 +737,26 @@ class MetricsCarouselController( }, networkReceived = MetricsCsv.Series(network.sampleTimes, network.received), networkTransmitted = MetricsCsv.Series(network.sampleTimes, network.transmitted), - temperature = MetricsCsv.Series(power.sampleTimes, power.temperatureMilliCelsius), - power = MetricsCsv.Series(power.sampleTimes, power.powerMicroWatts), - thermal = MetricsCsv.Series(power.sampleTimes, power.thermalStatus), + // The power series carry in-band sentinels for a reading the device does not provide. + // Named here rather than in MetricsCsv, which has no Android types in it. + temperature = + MetricsCsv.Series( + power.sampleTimes, + power.temperatureMilliCelsius, + absent = PowerUsageWatcher.UNAVAILABLE, + ), + power = + MetricsCsv.Series( + power.sampleTimes, + power.powerMicroWatts, + absent = PowerUsageWatcher.UNAVAILABLE, + ), + thermal = + MetricsCsv.Series( + power.sampleTimes, + power.thermalStatus, + absent = PowerUsageWatcher.THERMAL_UNKNOWN.toLong(), + ), annotations = markers(), ) } diff --git a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt index eeb47d03ce..6d1387274e 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt @@ -473,7 +473,12 @@ class MemoryUsageWatcher * The MemoryInfo instance is shared deliberately: it is the sampler's scratch buffer * for the next reading and no reader looks at it. */ - internal fun snapshot(): ProcessMemoryInfo = ProcessMemoryInfo(pid, pname, _history.copy()) + internal fun snapshot(): ProcessMemoryInfo = + // Every field, including watchedSinceMillis. Dropping it let it default to 0, which + // reads as "watched since the epoch" -- so the guard that blanks a process's + // zero-filled past never fired, and the Gradle daemon's buffer exported as + // measured zeros from before it existed (ADFA-5531). + ProcessMemoryInfo(pid, pname, _history.copy(), watchedSinceMillis) override fun equals(other: Any?): Boolean { if (this === other) return true diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt index c52891e086..0fb2721720 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt @@ -95,6 +95,13 @@ object MetricsCsv { * [NO_SAMPLE] entry marks an index nothing was ever recorded at, which is what tells an empty * cell apart from a measured zero. * @property values The samples themselves. + * @property absent The in-band value this series uses for "the device did not provide a + * reading", or `null` if it has none. Written as an empty cell, the same as an unsampled index. + * A watcher that stores a sentinel -- `PowerUsageWatcher.UNAVAILABLE` is [Long.MIN_VALUE] -- + * would otherwise put `-9223372036854775808` in a numeric column, and every consumer that + * averages or plots that column gets an answer that is not merely wrong but spectacular. The + * sentinel is named by the caller rather than known here, because this file deliberately has no + * Android types in it. * @property since When this series started being recorded. Samples timed before it belong to * the buffer's zero-filled past rather than to this series -- the Gradle daemon's buffer reaches * back to the start of the session however late in it the daemon appeared. @@ -103,6 +110,7 @@ object MetricsCsv { private val times: LongArray, private val values: LongArray, private val since: Long = 0L, + private val absent: Long? = null, ) { /** The value at index [i], or `null` if this series has nothing to say there. */ fun at(i: Int): Long? { @@ -110,7 +118,11 @@ object MetricsCsv { return null } val time = times[i] - return if (time == NO_SAMPLE || time < since) null else values[i] + if (time == NO_SAMPLE || time < since) { + return null + } + val value = values[i] + return if (value == absent) null else value } companion object { diff --git a/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt index 035e57cf68..5b7e2a21aa 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MemoryUsageWatcherSampleAlignmentTest.kt @@ -110,6 +110,21 @@ class MemoryUsageWatcherSampleAlignmentTest { ).isEqualTo(PSS_KB * 1024L) } + @Test + fun `a copied process still says when it started being watched`() { + val watcher = watcher() + watcher.watchProcess(PID, "Gradle Daemon") + watcher.readUsages() + + // getMemoryUsages hands out copies, and the copy used to drop watchedSinceMillis -- which + // defaults to 0, i.e. "watched since the epoch". The export's guard for a process's + // zero-filled past then never fired, so the daemon's buffer from before the daemon existed + // came out as measured zeros rather than empty cells (ADFA-5531). + val copied = watcher.getMemoryUsages().single() + assertThat(copied.watchedSinceMillis).isNotEqualTo(0L) + assertThat(copied.watchedSinceMillis).isEqualTo(watcher.getMemoryUsage(PID)!!.watchedSinceMillis) + } + private companion object { const val PID = 4242 diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt index f44619461a..cb9cceba0f 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt @@ -292,7 +292,34 @@ class MetricsCsvTest { return cells } + @Test + fun `a reading the device does not provide is an empty cell, not a sentinel`() { + val times = longArrayOf(T0, T0 + 1_000L) + val lines = + render( + snapshot( + rowTimes = times, + temperature = + MetricsCsv.Series( + times, + longArrayOf(Long.MIN_VALUE, 31_500L), + absent = Long.MIN_VALUE, + ), + ), + ) + + // PowerUsageWatcher stores Long.MIN_VALUE for a reading the platform will not give. Written + // straight out, a numeric column gets -9223372036854775808, and anything that averages or + // plots it -- ADFA-5494 reads this format back -- gets an answer that is not merely wrong + // but spectacular. Empty is what the format already means by "nothing to say here". + assertThat(cellsIn(lines[1])[TEMPERATURE_COLUMN]).isEmpty() + assertThat(cellsIn(lines[2])[TEMPERATURE_COLUMN]).isEqualTo("31500") + } + private companion object { + /** Index of `battery_temp_millicelsius`, from the header contract above. */ + val TEMPERATURE_COLUMN = EXPECTED_HEADER.split(",").indexOf("\"battery_temp_millicelsius\"") + /** * The header line, spelled out rather than derived from [MetricsCsv.HEADER]. * From 757dec1729c12e0258d15e632f3dceca63a59970 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Mon, 7 Sep 2026 18:32:49 -0700 Subject: [PATCH 07/10] ADFA-5531: hide the export button when the carousel undocks This ticket added the export button to the strip and did not extend setUndocked's list, so undocking left a live CSV export button sitting over the "tap to bring them back" message -- the same omission the battery readout made one ticket earlier, and for the same reason. Found by the test rather than by reading: the case that ADFA-5499's fix rewrote enumerates the strip's children instead of naming five ids, so merging that branch up turned this into a failure here, naming it -- "expected to be empty but was: [metrics_export]". That is what the rewrite was for. Straight into carouselIds, with no second term: unlike the battery readout, the export button has no other reason to be hidden, so docking can put it back unconditionally. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../com/itsaky/androidide/ui/MetricsCarouselLayout.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt index b568754857..f5a6ddc746 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselLayout.kt @@ -73,9 +73,10 @@ class MetricsCarouselLayout /** * Shows either the carousel or the "it is in a floating window" message, never a mix. * - * The whole strip switches, not just the pager. The arrows and the snapshot button are - * chrome for a chart that is not here: left behind they sit over the message, and the - * camera is inert anyway because undocking unbinds the controller that listens to it. + * The whole strip switches, not just the pager. The arrows, the snapshot button and the + * export button are chrome for a chart that is not here: left behind they sit over the + * message, and each is inert anyway because undocking unbinds the controller that listens + * to them. * * Keeping the set here was supposed to stop a control added later from being forgotten. * It did not: the battery readout arrived afterwards and was missed, so the readout sat @@ -92,6 +93,7 @@ class MetricsCarouselLayout R.id.metrics_previous, R.id.metrics_next, R.id.metrics_snapshot, + R.id.metrics_export, ) carouselIds.forEach { id -> findViewById(id)?.isVisible = !undocked From fdcb241963f11bb92073eaf69d238aef8e55af54 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 09:51:19 -0700 Subject: [PATCH 08/10] ADFA-5531: fix the review's export findings Four of the second round's findings, and the KDoc claim that was not true. A cell the export writes can be a formula. quote() doubled an embedded quote and stopped there, so a task name beginning = + - @ tab or CR went into the file as itself. The annotation columns carry names read from the user's own build script, and ADFA-5526 and ADFA-5534 attach this file to crash reports and feedback that someone opens. Such a cell now gets a leading apostrophe. Quoting alone does not prevent the evaluation. The numeric columns are written by number() and are left alone, because a negative reading has to stay a number. markerRows had no distance cap, so an annotation older than the ring buffer reaches -- the ordinary case in a long session -- was pulled onto whichever row was nearest, which for every one of them is row 0. putIfAbsent then kept the first and dropped the rest, so the file carried one arbitrary ancient marker on its oldest row and silently lost both the others and the real annotation that belonged there. A marker further than one sampling interval from its nearest row is dropped now. The chart already had both guards; only the export was missing them. Snapshot gained sampleIntervalMillis to say what that bound is, required rather than defaulted, because a default would pick it for a caller who never considered it. Three defaults that read as lies are gone: sampleTimes on NetworkUsage and on PowerUsage, and watchedSinceMillis on ProcessMemoryInfo, which this PR had already removed at its other site. An omitted sampleTimes produced a history whose every sample read as never-taken -- invisible to the chart, which asks only how long ago a sample was, and silently empty in every one of that watcher's CSV columns. The chart tests that relied on the default now state it, which is the point. exportCsv reused the camera button's in-flight flag. The two write different files into different directories and cannot race, so the single flag only meant that exporting ten thousand rows made the camera button dead for as long as it ran, and dead without saying so. And the file's own KDoc claimed the network and power values on a row "were taken within one interval of it". Nothing enforces that. The series are paired to a row by array index, each watcher runs its own loop, and the drift accumulates backwards through the buffer. Every column is still a real reading with a real time behind it, so nothing in the file is invented, but a consumer must not read one row as three simultaneous measurements. The comment says that now. Merging the series on time instead is its own change and its own PR. Each fix has a test that fails without it: the formula prefix, the distance cap, the stale marker that stole a real one's row, and the export that refused the camera. The monotonic-clock test changed its expectation rather than its subject -- an unconverted time now reaches no row at all instead of landing on the oldest one, which is still the outcome epochFor exists to prevent. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../ui/MetricsCarouselController.kt | 34 +++++-- .../androidide/utils/MemoryUsageWatcher.kt | 2 +- .../com/itsaky/androidide/utils/MetricsCsv.kt | 53 ++++++++++- .../androidide/utils/NetworkUsageWatcher.kt | 9 +- .../androidide/utils/PowerUsageWatcher.kt | 9 +- .../editor/MemUsageLineColorTest.kt | 1 + .../ui/MemoryUsageChartRendererTest.kt | 2 + .../ui/MetricsAnnotationSpanTest.kt | 1 + .../androidide/ui/MetricsCarouselHelpTest.kt | 6 +- .../ui/MetricsCarouselRebindTest.kt | 25 +++++ .../androidide/ui/MetricsChartAxisTapTest.kt | 1 + .../ui/MetricsChartNewestWindowTest.kt | 6 +- .../ui/MetricsChartTextScaleTest.kt | 18 +++- .../ui/NetworkUsageChartRendererTest.kt | 5 +- .../ui/PowerUsageChartRendererTest.kt | 4 +- .../itsaky/androidide/utils/MetricsCsvTest.kt | 91 +++++++++++++++++-- 16 files changed, 232 insertions(+), 35 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt index e383b3179f..4c77e71308 100644 --- a/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt +++ b/app/src/main/java/com/itsaky/androidide/ui/MetricsCarouselController.kt @@ -173,14 +173,26 @@ class MetricsCarouselController( private var currentPage = 0 /** - * Whether an export is already running. + * Whether a PNG snapshot is already being written. * * One at a time. The camera button is not debounced and each tap launched its own coroutine, * so two quick taps raced over the same scratch directory -- and, within the same second, over * the same filename, since the name is the chart label and a whole-second timestamp. Touched * only on the main thread, which is where both the tap and the coroutine's continuations run. + * + * A rapid second tap of the same button is refused silently: it is the double tap this exists + * to swallow, and a message for it would be noise on the gesture a user did not mean to make. + */ + private var snapshotInFlight = false + + /** + * Whether a CSV export is already being written, tracked apart from [snapshotInFlight]. + * + * The two write different files into different directories and cannot race each other, so one + * flag for both only meant that starting a ten-thousand-row export refused the camera button + * for as long as it ran -- and refused it silently, which reads as a dead control. */ - private var exportInFlight = false + private var csvExportInFlight = false /** * The pager of the bound carousel, or `null` when nothing is bound. Exposed so a host can apply @@ -589,7 +601,7 @@ class MetricsCarouselController( @UiThread fun exportSnapshot(): Boolean { val binding = this.binding ?: return false - if (exportInFlight) { + if (snapshotInFlight) { log.debug("Ignoring a snapshot request while one is already being written") return false } @@ -613,7 +625,7 @@ class MetricsCarouselController( // it ends in startActivity, which throws from a context with no task of its own unless it is // given FLAG_ACTIVITY_NEW_TASK, so it keeps the context the carousel is hosted in. val appContext = context.applicationContext - exportInFlight = true + snapshotInFlight = true scope.launch { // Everything here is guarded: the scope has no exception handler, so anything escaping // reaches the global crash reporter and is filed as a crash. MetricsSnapshot.write @@ -649,13 +661,13 @@ class MetricsCarouselController( // Cleared before rethrowing: a cancelled export is finished either way, and // leaving the flag set would refuse every later one for the life of the // carousel. - exportInFlight = false + snapshotInFlight = false throw failure } log.error("Could not share the chart snapshot", failure) Toast.makeText(appContext, string.msg_metrics_snapshot_failed, Toast.LENGTH_SHORT).show() } - exportInFlight = false + snapshotInFlight = false } return true } @@ -673,7 +685,7 @@ class MetricsCarouselController( @UiThread fun exportCsv(): Boolean { val binding = this.binding ?: return false - if (exportInFlight) { + if (csvExportInFlight) { log.debug("Ignoring an export request while one is already being written") return false } @@ -681,7 +693,7 @@ class MetricsCarouselController( val context = binding.root.context val appContext = context.applicationContext val snapshot = snapshot() - exportInFlight = true + csvExportInFlight = true scope.launch { // Guarded for the same reason exportSnapshot is: the scope has no exception handler, so // anything escaping here is filed as a crash. @@ -697,13 +709,13 @@ class MetricsCarouselController( IntentUtils.shareFile(host, file, MetricsCsv.MIME_TYPE, extraFlags) }.onFailure { failure -> if (failure is CancellationException) { - exportInFlight = false + csvExportInFlight = false throw failure } log.error("Could not share the metrics export", failure) Toast.makeText(appContext, string.msg_metrics_export_failed, Toast.LENGTH_SHORT).show() } - exportInFlight = false + csvExportInFlight = false } return true } @@ -729,6 +741,8 @@ class MetricsCarouselController( return MetricsCsv.Snapshot( rowTimes = memory.times, + // The memory watcher's, because its times are the rows. + sampleIntervalMillis = memoryUsageWatcher.updateInterval, memory = memory.processes.associate { process -> process.pname to diff --git a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt index 6d1387274e..99c477dd0b 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MemoryUsageWatcher.kt @@ -460,7 +460,7 @@ class MemoryUsageWatcher val pname: String, internal val _history: MutableShiftedLongArray, /** When this process started being watched, as milliseconds since the epoch. */ - val watchedSinceMillis: Long = 0L, + val watchedSinceMillis: Long, ) { internal val memInfo: MemoryInfo = MemoryInfo() diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt index 0fb2721720..0ae7657fd0 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt @@ -32,10 +32,16 @@ import kotlin.math.abs * * A row is a sampling tick, and its columns come from three watchers that each keep their own ring * buffer and their own coroutine. They are started together and share one interval, and every one - * of them is cleared when that interval changes, so the tick at index *i* is the same tick in all - * three -- but they do not read their sources at the same instant. The row's stated time is the - * memory watcher's, recorded when it sampled; the network and power values on that row were taken - * within one interval of it. Nothing here reconstructs a time from an index. + * of them is cleared when that interval changes. The row's stated time is the memory watcher's, + * recorded when it sampled, and nothing here reconstructs a time from an index. + * + * The other series are paired to that row by array index rather than by time, which is not the same + * thing. Each watcher runs its own loop and does its own per-tick work, so their ticks drift apart, + * and because the buffers are filled oldest-first the drift accumulates backwards: the further back + * a row is, the further its network and power values can sit from its stated time. Every column is + * a real reading with a real time behind it, so nothing in the file is invented -- but a consumer + * must not read one row as three simultaneous measurements. Merging the series on time instead is + * the subject of its own change; this comment says what is true until then. * * Formatting only, with no Android types, so the whole format can be tested without a device. */ @@ -146,9 +152,13 @@ object MetricsCsv { * * @property rowTimes The memory watcher's sample times, oldest first. They are the rows, because * memory is the one series always being recorded. + * @property sampleIntervalMillis How often the watchers sample. Required rather than defaulted, + * because it decides which annotations are near enough to a row to be written on it and a + * default would pick that bound for a caller who never considered it. */ class Snapshot( val rowTimes: LongArray, + val sampleIntervalMillis: Long, val memory: Map, val networkReceived: Series = Series.EMPTY, val networkTransmitted: Series = Series.EMPTY, @@ -230,6 +240,16 @@ object MetricsCsv { * almost all of them; and doing this per row rather than once would walk every marker against * every row, which at ten thousand of each is not a cost worth paying for a button. * + * A marker further than one sampling interval from its nearest row is dropped rather than + * pulled onto it. Sampling at a fixed interval leaves every marker that happened while the + * buffer was filling within half an interval of some sample, so a greater distance means the + * marker falls outside the sampled window -- an annotation older than the buffer reaches, which + * is the ordinary case in a long session. Without the cap every one of those lands on row 0, + * where [MutableMap.putIfAbsent] keeps the first and drops the rest: the file would carry one + * arbitrary ancient marker on its oldest row and lose the others silently. The chart has both + * guards already -- it asks the store only for the annotations in the visible span, and drops + * any whose x falls before the first sample. + * * Where two markers land on one row the earlier wins, and the later is dropped rather than * silently overwriting it -- the file has one annotation column per row by definition. */ @@ -246,6 +266,9 @@ object MetricsCsv { val rows = mutableMapOf() snapshot.annotations.sortedBy { it.atMillis }.forEach { marker -> val nearest = sampled.minByOrNull { abs(it.value - marker.atMillis) } ?: return@forEach + if (abs(nearest.value - marker.atMillis) > snapshot.sampleIntervalMillis) { + return@forEach + } rows.putIfAbsent(nearest.index, marker) } return rows @@ -253,11 +276,31 @@ object MetricsCsv { private fun number(value: Long?): String = value?.toString() ?: "" + /** + * The characters that make a spreadsheet read a cell as a formula rather than as text. + * + * Tab and carriage return are here because a leading one of either is stripped on import, which + * exposes whatever follows it: a cell of "\t=cmd" is a formula too. + */ + private val FORMULA_LEAD = charArrayOf('=', '+', '-', '@', '\t', '\r') + /** * A CSV string cell. * * Quoted per the format's rule (b), with any quote inside it doubled -- a task name is text the * IDE was given, and nothing guarantees it has no quotes in it. + * + * A cell beginning with one of [FORMULA_LEAD] additionally gets a leading apostrophe. Quoting + * alone does not stop a spreadsheet evaluating the cell on import, and the annotation columns + * carry Gradle task names taken from the user's own build script -- into a file ADFA-5526 and + * ADFA-5534 attach to crash reports and to feedback, which a support engineer then opens. The + * apostrophe is part of the cell as written, so a reader parsing this file back has to strip it. + * + * The numeric columns do not come through here. They are written by [number], where a negative + * value has to stay a number rather than become text with a quote in front of it. */ - private fun quote(value: String): String = "\"" + value.replace("\"", "\"\"") + "\"" + private fun quote(value: String): String { + val guarded = if (value.isNotEmpty() && value[0] in FORMULA_LEAD) "'" + value else value + return "\"" + guarded.replace("\"", "\"\"") + "\"" + } } diff --git a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt index 2166a87d74..3be34da8f1 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/NetworkUsageWatcher.kt @@ -325,13 +325,16 @@ class NetworkUsageWatcher * epoch, parallel to the values. Read in the same critical section as them, because reading * the two separately let the sampler append between the calls and shifted every value one * index against its timestamp (ADFA-5531). A zero means nothing was ever sampled at that - * index -- the buffers are fixed-length and start, and are cleared, full of them. Defaulted - * empty for the chart, which asks only how long ago a sample was and never when. + * index -- the buffers are fixed-length and start, and are cleared, full of them. + * + * Required, with no empty default. A caller that omitted it produced a history whose every + * sample read as never-taken, which the chart cannot see -- it asks only how long ago a + * sample was -- but which silently emptied both of this watcher's columns in the CSV. */ data class NetworkUsage( val received: LongArray, val transmitted: LongArray, - val sampleTimes: LongArray = LongArray(0), + val sampleTimes: LongArray, ) { override fun equals(other: Any?): Boolean = this === other || diff --git a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt index 5cb23f3788..d6fd12025f 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/PowerUsageWatcher.kt @@ -288,14 +288,17 @@ class PowerUsageWatcher * epoch, parallel to the values. Read in the same critical section as them, because reading * the two separately let the sampler append between the calls and shifted every value one * index against its timestamp (ADFA-5531). A zero means nothing was ever sampled at that - * index -- the buffers are fixed-length and start, and are cleared, full of them. Defaulted - * empty for the chart, which asks only how long ago a sample was and never when. + * index -- the buffers are fixed-length and start, and are cleared, full of them. + * + * Required, with no empty default. A caller that omitted it produced a history whose every + * sample read as never-taken, which the chart cannot see -- it asks only how long ago a + * sample was -- but which silently emptied every one of this watcher's columns in the CSV. */ data class PowerUsage( val temperatureMilliCelsius: LongArray, val powerMicroWatts: LongArray, val thermalStatus: LongArray, - val sampleTimes: LongArray = LongArray(0), + val sampleTimes: LongArray, ) { override fun equals(other: Any?): Boolean = this === other || diff --git a/app/src/test/java/com/itsaky/androidide/activities/editor/MemUsageLineColorTest.kt b/app/src/test/java/com/itsaky/androidide/activities/editor/MemUsageLineColorTest.kt index d82bb0b9aa..8bb19fa47f 100644 --- a/app/src/test/java/com/itsaky/androidide/activities/editor/MemUsageLineColorTest.kt +++ b/app/src/test/java/com/itsaky/androidide/activities/editor/MemUsageLineColorTest.kt @@ -39,6 +39,7 @@ class MemUsageLineColorTest { pid = 1234, pname = name, _history = MutableShiftedLongArray(4), + watchedSinceMillis = 0L, ) @Test diff --git a/app/src/test/java/com/itsaky/androidide/ui/MemoryUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MemoryUsageChartRendererTest.kt index f47a5561d2..20c57604df 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MemoryUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MemoryUsageChartRendererTest.kt @@ -58,6 +58,7 @@ class MemoryUsageChartRendererTest { PID_IDE, "IDE", MutableShiftedLongArray(LongArray(history.size) { history[it] }), + watchedSinceMillis = 0L, ) renderer { arrayOf(process) }.attach(chart) chart.layOutAndDraw() @@ -73,6 +74,7 @@ class MemoryUsageChartRendererTest { pid, pname, MutableShiftedLongArray(MemoryUsageWatcher.MAX_USAGE_ENTRIES) { (firstMegabytes + it) * BYTES_PER_MB }, + watchedSinceMillis = 0L, ) private fun datasetFor( diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt index 23e24a84d1..ab36922fa2 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsAnnotationSpanTest.kt @@ -54,6 +54,7 @@ class MetricsAnnotationSpanTest { NetworkUsageWatcher.NetworkUsage( LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), ) }, annotations = store, diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt index e2bf17635c..44c35a3d3c 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselHelpTest.kt @@ -210,7 +210,11 @@ class MetricsCarouselHelpTest { val renderer = NetworkUsageChartRenderer( usageProvider = { - NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + NetworkUsageWatcher.NetworkUsage( + LongArray(SAMPLES) { 1_000L }, + LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), + ) }, ) renderer.attach(chart) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselRebindTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselRebindTest.kt index b563a51c9b..79d6e02646 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselRebindTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsCarouselRebindTest.kt @@ -128,6 +128,31 @@ class MetricsCarouselRebindTest { assertThat(controller.exportSnapshot()).isFalse() } + @Test + fun `a running CSV export does not refuse the camera button`() { + val controller = controller() + val binding = strip() + controller.bind(binding) + laidOut(binding) + + // The two write different files into different directories and cannot race each other. One + // flag for both meant that starting an export of ten thousand rows made the camera button + // dead for as long as it ran, and dead silently -- the tap returned false and said nothing. + assertThat(controller.exportCsv()).isTrue() + assertThat(controller.exportSnapshot()).isTrue() + } + + @Test + fun `a second CSV export is refused while the first is still being written`() { + val controller = controller() + val binding = strip() + controller.bind(binding) + laidOut(binding) + + assertThat(controller.exportCsv()).isTrue() + assertThat(controller.exportCsv()).isFalse() + } + @Test fun `both arrows are tinted, whatever inflated them`() { val binding = strip() diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt index c9713c73f9..3396443bb7 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartAxisTapTest.kt @@ -57,6 +57,7 @@ class MetricsChartAxisTapTest { NetworkUsageWatcher.NetworkUsage( LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), ) }, ) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt index d9636409b2..a8443a7894 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartNewestWindowTest.kt @@ -42,7 +42,11 @@ class MetricsChartNewestWindowTest { private fun renderer() = NetworkUsageChartRenderer( usageProvider = { - NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + NetworkUsageWatcher.NetworkUsage( + LongArray(SAMPLES) { 1_000L }, + LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), + ) }, ) diff --git a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartTextScaleTest.kt b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartTextScaleTest.kt index 29ace075c5..b131aee423 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/MetricsChartTextScaleTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/MetricsChartTextScaleTest.kt @@ -49,7 +49,11 @@ class MetricsChartTextScaleTest { val chart = SafeLineChart(context) NetworkUsageChartRenderer( usageProvider = { - NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + NetworkUsageWatcher.NetworkUsage( + LongArray(SAMPLES) { 1_000L }, + LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), + ) }, ).attach(chart) return chart @@ -114,7 +118,11 @@ class MetricsChartTextScaleTest { val chart = SafeLineChart(context) NetworkUsageChartRenderer( usageProvider = { - NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + NetworkUsageWatcher.NetworkUsage( + LongArray(SAMPLES) { 1_000L }, + LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), + ) }, annotations = store, sampleInterval = { 1_000L }, @@ -176,7 +184,11 @@ class MetricsChartTextScaleTest { .inflate(R.layout.item_metrics_chart, strip.metricsPager, false) as SafeLineChart NetworkUsageChartRenderer( usageProvider = { - NetworkUsageWatcher.NetworkUsage(LongArray(SAMPLES) { 1_000L }, LongArray(SAMPLES) { 500L }) + NetworkUsageWatcher.NetworkUsage( + LongArray(SAMPLES) { 1_000L }, + LongArray(SAMPLES) { 500L }, + LongArray(SAMPLES), + ) }, ).attach(page) page.layOutAndDraw(width = strip.metricsPager.width, height = strip.metricsPager.height) diff --git a/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt index fa9394ce11..b104b450be 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/NetworkUsageChartRendererTest.kt @@ -44,10 +44,13 @@ class NetworkUsageChartRendererTest { private val context = ApplicationProvider.getApplicationContext() + // No sample times: these tests are about what the chart draws, and the chart asks only how long + // ago a sample was. Stated rather than defaulted, because the same emptiness in production + // silently blanks the CSV's network columns. private fun usage( received: LongArray, transmitted: LongArray = received, - ) = NetworkUsageWatcher.NetworkUsage(received, transmitted) + ) = NetworkUsageWatcher.NetworkUsage(received, transmitted, LongArray(received.size)) private fun rendererFor(usage: NetworkUsageWatcher.NetworkUsage): Pair { val chart = SafeLineChart(context) diff --git a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt index 8e03637b38..f1738a164e 100644 --- a/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt +++ b/app/src/test/java/com/itsaky/androidide/ui/PowerUsageChartRendererTest.kt @@ -40,11 +40,13 @@ import org.robolectric.RobolectricTestRunner class PowerUsageChartRendererTest { private val context = ApplicationProvider.getApplicationContext() + // No sample times, for the reason NetworkUsageChartRendererTest gives: a chart test says so + // rather than letting a default say it. private fun usage( temperature: LongArray, power: LongArray = LongArray(temperature.size), thermal: LongArray = LongArray(temperature.size), - ) = PowerUsageWatcher.PowerUsage(temperature, power, thermal) + ) = PowerUsageWatcher.PowerUsage(temperature, power, thermal, LongArray(temperature.size)) private fun rendererFor( usage: PowerUsageWatcher.PowerUsage, diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt index cb9cceba0f..22886b6128 100644 --- a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvTest.kt @@ -43,6 +43,7 @@ class MetricsCsvTest { private fun snapshot( rowTimes: LongArray = longArrayOf(T0, T0 + 1_000L), + sampleIntervalMillis: Long = INTERVAL_MS, memory: Map = emptyMap(), networkReceived: MetricsCsv.Series = MetricsCsv.Series.EMPTY, networkTransmitted: MetricsCsv.Series = MetricsCsv.Series.EMPTY, @@ -52,6 +53,7 @@ class MetricsCsvTest { annotations: List = emptyList(), ) = MetricsCsv.Snapshot( rowTimes = rowTimes, + sampleIntervalMillis = sampleIntervalMillis, memory = memory, networkReceived = networkReceived, networkTransmitted = networkTransmitted, @@ -179,6 +181,78 @@ class MetricsCsvTest { assertThat(cells[MetricsCsv.HEADER.indexOf("annotation_kind")]).isEqualTo("\"TASK\"") } + @Test + fun `a cell a spreadsheet would run as a formula is prefixed`() { + // The annotation columns carry Gradle task names read from the user's own build script, and + // this file is attached to crash reports (ADFA-5526) and feedback (ADFA-5534) that someone + // opens. Quoting alone does not stop the evaluation; the apostrophe does. + val column = MetricsCsv.HEADER.indexOf("annotation") + listOf("=1+1", "+1", "-1", "@SUM(A1)", "\tlater", "\rlater").forEach { label -> + val lines = + render( + snapshot( + rowTimes = longArrayOf(T0), + annotations = listOf(MetricsCsv.Marker(T0, label, "TASK")), + ), + ) + + assertThat(cellsIn(lines[1])[column]).isEqualTo("\"'" + label + "\"") + } + } + + @Test + fun `an ordinary label is not prefixed`() { + // The guard has to be narrow: prefixing every cell would put an apostrophe in front of every + // task name a reader sees. + val lines = + render( + snapshot( + rowTimes = longArrayOf(T0), + annotations = listOf(MetricsCsv.Marker(T0, ":app:assembleV8Debug", "TASK")), + ), + ) + + assertThat(cellsIn(lines[1])[MetricsCsv.HEADER.indexOf("annotation")]) + .isEqualTo("\":app:assembleV8Debug\"") + } + + @Test + fun `an annotation further than one interval from every row is dropped`() { + // An annotation older than the buffer reaches is the ordinary case in a long session: the + // store keeps its own history and the ring buffer has already rolled past it. + val lines = + render( + snapshot( + rowTimes = longArrayOf(T0, T0 + INTERVAL_MS), + annotations = listOf(MetricsCsv.Marker(T0 - 60_000L, "Build started", "BUILD_STARTED")), + ), + ) + + val column = MetricsCsv.HEADER.indexOf("annotation") + assertThat(lines.drop(1).map { cellsIn(it)[column] }).containsExactly("", "") + } + + @Test + fun `a stale annotation does not take the first row from a real one`() { + // Both markers' nearest row is the first one. Sorted by time the stale one comes first, so + // without the cap it took the row and putIfAbsent then dropped the marker that actually + // belongs there -- the file gained an ancient annotation on its oldest row and lost a real + // one, with nothing to say either had happened. + val lines = + render( + snapshot( + rowTimes = longArrayOf(T0, T0 + INTERVAL_MS), + annotations = + listOf( + MetricsCsv.Marker(T0 - 60 * 60 * 1000L, "stale", "TASK"), + MetricsCsv.Marker(T0 + 10L, "real", "TASK"), + ), + ), + ) + + assertThat(cellsIn(lines[1])[MetricsCsv.HEADER.indexOf("annotation")]).isEqualTo("\"real\"") + } + @Test fun `an annotation lands on the sample nearest in time, not only an exact match`() { val times = longArrayOf(T0, T0 + 1_000L, T0 + 2_000L) @@ -240,10 +314,13 @@ class MetricsCsvTest { } @Test - fun `an unconverted monotonic time would land on the oldest row`() { - // What the mix-up looked like on a device: a monotonic time is a few hours and an epoch time - // is decades, so every row is about equally far away and the nearest-row search picks - // whichever number is smallest -- the oldest sample, whenever the event really happened. + fun `an unconverted monotonic time reaches no row at all`() { + // What the mix-up looks like on a device: a monotonic time is a few hours and an epoch time + // is decades, so every row is about equally far away. Before the distance cap the + // nearest-row search picked whichever number was smallest -- the oldest sample, whenever + // the event really happened -- and wrote the marker there. Now it is further from every row + // than a sampling interval, so it is dropped, and the file loses it rather than lying about + // when it happened. Either way [MetricsCsv.epochFor] is what makes it land correctly. val times = longArrayOf(T0, T0 + 1_000L, T0 + 2_000L) val lines = render( @@ -254,8 +331,7 @@ class MetricsCsvTest { ) val column = MetricsCsv.HEADER.indexOf("annotation") - assertThat(cellsIn(lines[1])[column]).isEqualTo("\"Build started\"") - assertThat(cellsIn(lines[3])[column]).isEmpty() + assertThat(lines.drop(1).map { cellsIn(it)[column] }).containsExactly("", "", "") } @Test @@ -334,5 +410,8 @@ class MetricsCsvTest { /** 2026-09-06T22:33:40.123 in America/Los_Angeles, which is UTC-7 at that date. */ const val T0 = 1_788_759_220_123L + + /** The gap between the default rows, and so the distance a marker may sit from one. */ + const val INTERVAL_MS = 1_000L } } From c697bd3767e61deba296ab8dec0ccacfb96f7108 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 13:21:50 -0700 Subject: [PATCH 09/10] ADFA-5531: give the three process names one home MEMORY_COLUMNS spelled "IDE", "Gradle Tooling" and "Gradle Daemon" as literals, and BaseEditorActivity's PROC_* constants spelled the same three again. Nothing joined them but string equality, and the failure mode is silent: a name that matches nothing in the snapshot is written as an absent value rather than as an error, so renaming a process would have gone on emitting the old header and quietly emptied the column. The names live in MetricsCsv now because that is the file that cannot move -- a column name is its published contract, read back by ADFA-5494 and by whoever opens the copy ADFA-5526 and ADFA-5534 attach to a report. The activity's constants alias them rather than repeating them, and stay protected because subclasses use them. No new test: the drift is now unrepresentable rather than merely detected. MemUsageLineColorTest already asserts the colour mapping against the literal names, so a rename that broke the mapping still fails there, and MetricsCsvTest still pins the full header as a hand-written string rather than deriving it from these constants. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../activities/editor/BaseEditorActivity.kt | 10 ++++++--- .../com/itsaky/androidide/utils/MetricsCsv.kt | 22 ++++++++++++++++++- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt index 5990f8507f..767233f2d6 100644 --- a/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt +++ b/app/src/main/java/com/itsaky/androidide/activities/editor/BaseEditorActivity.kt @@ -130,6 +130,7 @@ import com.itsaky.androidide.utils.InstallationResultHandler.onResult import com.itsaky.androidide.utils.IntentUtils import com.itsaky.androidide.utils.MemoryUsageWatcher import com.itsaky.androidide.utils.MetricsAnnotationStore +import com.itsaky.androidide.utils.MetricsCsv import com.itsaky.androidide.utils.StringsInjectionException import com.itsaky.androidide.utils.StringsXmlInjector import com.itsaky.androidide.utils.applyBottomSheetAnchorForOrientation @@ -487,13 +488,16 @@ abstract class BaseEditorActivity : else -> Color.GRAY } - protected val PROC_IDE = "IDE" + // Aliases, not copies. The names belong to the CSV, whose header is a published contract; + // see MetricsCsv.PROC_IDE for why they live there. Kept as protected members because + // subclasses use them. + protected val PROC_IDE = MetricsCsv.PROC_IDE @JvmStatic - protected val PROC_GRADLE_TOOLING = "Gradle Tooling" + protected val PROC_GRADLE_TOOLING = MetricsCsv.PROC_GRADLE_TOOLING @JvmStatic - protected val PROC_GRADLE_DAEMON = "Gradle Daemon" + protected val PROC_GRADLE_DAEMON = MetricsCsv.PROC_GRADLE_DAEMON @JvmStatic protected val log: Logger = LoggerFactory.getLogger(BaseEditorActivity::class.java) diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt index 0ae7657fd0..68f6c695b9 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsCsv.kt @@ -70,6 +70,26 @@ object MetricsCsv { private val TIMESTAMP_FORMAT: DateTimeFormatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX", Locale.ROOT) + /** + * The names the memory watcher is given for the three processes the IDE plots. + * + * Here rather than beside the watcher because this file is the one that cannot move: a column + * name is the file's published contract, read back by ADFA-5494 and by whoever opens the copy + * ADFA-5526 and ADFA-5534 attach to a report. Everything else looks these up. + * + * They were literals in two places -- these and `BaseEditorActivity.PROC_*` -- joined by + * nothing but string equality. Renaming a process there would have gone on writing the old + * header here and quietly emptied the column, because a name that matches nothing in the + * snapshot is written as an absent value rather than as an error. + */ + const val PROC_IDE = "IDE" + + /** @see PROC_IDE */ + const val PROC_GRADLE_TOOLING = "Gradle Tooling" + + /** @see PROC_IDE */ + const val PROC_GRADLE_DAEMON = "Gradle Daemon" + /** * The memory series, in column order. * @@ -78,7 +98,7 @@ object MetricsCsv { * -- and a header that depended on it would describe a different file each time. A process that * is not being watched leaves its column empty. */ - val MEMORY_COLUMNS = listOf("IDE", "Gradle Tooling", "Gradle Daemon") + val MEMORY_COLUMNS = listOf(PROC_IDE, PROC_GRADLE_TOOLING, PROC_GRADLE_DAEMON) @JvmStatic val HEADER: List = From 0b68ae3832956b060d388f5039f82d8f38aefa0b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 8 Sep 2026 15:39:23 -0700 Subject: [PATCH 10/10] ADFA-5531: test the export file, and fix the pruning it exposed MetricsCsvFile made KEEP_RECENT and the clock injectable for a test and then had none, so neither the bound on the export directory nor the name the file is given was asserted anywhere on this branch. Writing that test found a real defect in this ticket's own path. pruneTo chose "the oldest n" across every file and then skipped the one just written, which deleted one too few whenever that file sorted into the set -- and the export directory crept one over KEEP_RECENT each time. Two writes inside a single filesystem timestamp are enough to sort it there. The new file is excluded from the candidates now rather than skipped among them. Both new bound tests fail without that change with "expected to be at most: 3 but was: 4", which is the defect exactly. The fix already existed -- two branches up, on ADFA-5534, along with the test. That is the third time today a fix for one ticket's file has been found sitting on a later ticket's branch, after ADFA-5489's sampler race and this same file's test. Anyone building or QA'ing #1799 alone gets the unfixed pruning; that is what this corrects. Only the cases that belong here came down. The gzip and report-copy tests stay on ADFA-5534, which is the ticket that owns writeForReport. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017CCQUU7tBzZL61EmQhJP8j --- .../androidide/utils/MetricsSnapshot.kt | 13 ++- .../androidide/utils/MetricsCsvFileTest.kt | 110 ++++++++++++++++++ 2 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/utils/MetricsCsvFileTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt b/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt index 7159482843..113b9c89fe 100644 --- a/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt +++ b/app/src/main/java/com/itsaky/androidide/utils/MetricsSnapshot.kt @@ -102,12 +102,17 @@ object MetricsSnapshot { limit: Int, newest: File, ) { - val files = directory.listFiles()?.sortedBy { it.lastModified() } ?: return - if (files.size <= limit) { + // [newest] is excluded from the candidates rather than skipped among them. Skipping it after + // choosing "the oldest n" left one file too many whenever it sorted into that set, and the + // directory then crept one over the limit per collision. Two writes inside one filesystem + // timestamp are enough to sort it there. + val candidates = directory.listFiles()?.filter { it != newest }?.sortedBy { it.lastModified() } ?: return + val excess = candidates.size - (limit - 1) + if (excess <= 0) { return } - files.take(files.size - limit).forEach { file -> - if (file != newest && !file.delete()) { + candidates.take(excess).forEach { file -> + if (!file.delete()) { log.warn("Could not delete the stale chart snapshot at {}", file) } } diff --git a/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvFileTest.kt b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvFileTest.kt new file mode 100644 index 0000000000..61e2aad19d --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/utils/MetricsCsvFileTest.kt @@ -0,0 +1,110 @@ +/* + * This file is part of AndroidIDE. + * + * AndroidIDE is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * AndroidIDE is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with AndroidIDE. If not, see . + */ + +package com.itsaky.androidide.utils + +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import java.io.File +import java.time.ZoneId + +/** + * The file the export button writes (ADFA-5531). + * + * [MetricsCsvFile.KEEP_RECENT] and the clock were made injectable for a test and then had none, so + * the bound on the directory and the name the file is given were both unasserted. + */ +@RunWith(RobolectricTestRunner::class) +class MetricsCsvFileTest { + private val context = ApplicationProvider.getApplicationContext() + + /** + * Fixed, not the machine's own. + * + * The name asserted below is a rendering of [AT] in a particular zone, so leaving the zone to + * the default made this pass here and fail wherever CI happens to be. + */ + private val zone: ZoneId = ZoneId.of("America/Los_Angeles") + + private fun snapshot(rows: Int): MetricsCsv.Snapshot { + val times = LongArray(rows) { AT + it * 1_000L } + return MetricsCsv.Snapshot( + rowTimes = times, + sampleIntervalMillis = INTERVAL_MS, + memory = mapOf(MetricsCsv.PROC_IDE to MetricsCsv.Series(times, LongArray(rows) { 600_000_000L + it })), + ) + } + + @Test + fun `an export is plain csv the user can open`() { + val file = MetricsCsvFile.write(context, snapshot(3), AT, zone)!! + + assertThat(file.name).isEqualTo("2026_09_06_22_33_40_123.csv") + assertThat(file.readText().lineSequence().first()).startsWith("\"timestamp\"") + } + + @Test + fun `the export directory stays bounded`() { + repeat(MetricsCsvFile.KEEP_RECENT + 4) { i -> + MetricsCsvFile.write(context, snapshot(1), AT + i * 1_000L, zone) + } + + val directory = MetricsCsvFile.write(context, snapshot(1), AT + 90_000L, zone)!!.parentFile!! + + assertThat(directory.listFiles()!!.size).isAtMost(MetricsCsvFile.KEEP_RECENT) + } + + @Test + fun `the limit holds when the file just written is not the newest on disk`() { + // Pruning used to pick "the oldest n" across every file and then skip the one just written, + // which deleted one too few whenever that one sorted into the set -- and the directory crept + // one over the limit each time. Two writes inside a single filesystem timestamp are enough + // to sort it there. + // + // Dating the existing files into the future is what puts the new one at the front of the + // sort deterministically. Tying them all to one *past* value does not: the file written last + // still carries a real mtime, so it sorts last, is never in the set, and the skip never + // fires -- which is how the first version of this test passed against the unfixed code. + val future = System.currentTimeMillis() + 1_000_000L + repeat(MetricsCsvFile.KEEP_RECENT + 3) { i -> + MetricsCsvFile.write(context, snapshot(1), AT + i, zone)!!.setLastModified(future) + } + + val directory = MetricsCsvFile.write(context, snapshot(1), AT + 900L, zone)!!.parentFile!! + + assertThat(directory.listFiles()!!.size).isAtMost(MetricsCsvFile.KEEP_RECENT) + } + + @Test + fun `files land under the cache, which the platform may reclaim`() { + val file: File = MetricsCsvFile.write(context, snapshot(2), AT, zone)!! + + assertThat(file.absolutePath).startsWith(context.cacheDir.absolutePath) + } + + private companion object { + /** 2026-09-06T22:33:40.123 local. */ + const val AT = 1_788_759_220_123L + + /** The gap between the rows these fixtures build. */ + const val INTERVAL_MS = 1_000L + } +}