Move TableLogBuffer into its own DumpManager collection

Moving TableLogBuffer out of the `dumpable` collection and into its own
individually-addressable space allows us to implement more type-specific
features using the SystemUI cli

Test: DumpManagerTest
Test: DumpHandlerTest
Test: <dump invocation> tables
Test: <dump invocation> tables --list
Test: <dump invocation> dump-normal | grep <any table log>
Bug: 278094048
(cherry picked from https://googleplex-android-review.googlesource.com/q/commit:c455e14e48147472f7db273cfe4d25afd60b40ae)
Merged-In: I0fd9cab64ed9320a25546e2842b6aaf625f129a1
Change-Id: I0fd9cab64ed9320a25546e2842b6aaf625f129a1
This commit is contained in:
Evan Laird
2023-05-24 14:00:37 -04:00
parent a001eb709d
commit c9c4f9d445
7 changed files with 187 additions and 30 deletions

View File

@@ -26,8 +26,10 @@ import com.android.systemui.dump.DumpHandler.Companion.PRIORITY_ARG_CRITICAL
import com.android.systemui.dump.DumpHandler.Companion.PRIORITY_ARG_NORMAL
import com.android.systemui.dump.DumpsysEntry.DumpableEntry
import com.android.systemui.dump.DumpsysEntry.LogBufferEntry
import com.android.systemui.dump.DumpsysEntry.TableLogBufferEntry
import com.android.systemui.dump.nano.SystemUIProtoDump
import com.android.systemui.log.LogBuffer
import com.android.systemui.log.table.TableLogBuffer
import com.google.protobuf.nano.MessageNano
import java.io.BufferedOutputStream
import java.io.FileDescriptor
@@ -41,7 +43,7 @@ import javax.inject.Provider
*
* Dump output is split into two sections, CRITICAL and NORMAL. In general, the CRITICAL section
* contains all dumpables that were registered to the [DumpManager], while the NORMAL sections
* contains all [LogBuffer]s (due to their length).
* contains all [LogBuffer]s and [TableLogBuffer]s (due to their length).
*
* The CRITICAL and NORMAL sections can be found within a bug report by searching for "SERVICE
* com.android.systemui/.SystemUIService" and "SERVICE
@@ -74,6 +76,7 @@ import javax.inject.Provider
* # To dump all dumpables or all buffers:
* $ <invocation> dumpables
* $ <invocation> buffers
* $ <invocation> tables
*
* # Finally, the following will simulate what we dump during the CRITICAL and NORMAL sections of a
* # bug report:
@@ -125,6 +128,7 @@ constructor(
"bugreport-normal" -> dumpNormal(pw, args)
"dumpables" -> dumpDumpables(pw, args)
"buffers" -> dumpBuffers(pw, args)
"tables" -> dumpTables(pw, args)
"config" -> dumpConfig(pw)
"help" -> dumpHelp(pw)
else -> {
@@ -160,26 +164,22 @@ constructor(
dumpBuffer(buffer, pw, args.tailLength)
}
val tableBuffers = dumpManager.getTableLogBuffers()
for (table in tableBuffers) {
dumpTableBuffer(table, pw, args.rawArgs)
}
logBufferEulogizer.readEulogyIfPresent(pw)
}
private fun dumpDumpables(pw: PrintWriter, args: ParsedArgs) =
dumpManager.getDumpables().run {
if (args.listOnly) {
listTargetNames(this, pw)
} else {
forEach { dumpable -> dumpDumpable(dumpable, pw, args.rawArgs) }
}
}
dumpManager.getDumpables().listOrDumpEntries(pw, args)
private fun dumpBuffers(pw: PrintWriter, args: ParsedArgs) =
dumpManager.getLogBuffers().run {
if (args.listOnly) {
listTargetNames(this, pw)
} else {
forEach { buf -> dumpBuffer(buf, pw, args.tailLength) }
}
}
dumpManager.getLogBuffers().listOrDumpEntries(pw, args)
private fun dumpTables(pw: PrintWriter, args: ParsedArgs) =
dumpManager.getTableLogBuffers().listOrDumpEntries(pw, args)
private fun listTargetNames(targets: Collection<DumpsysEntry>, pw: PrintWriter) {
for (target in targets) {
@@ -215,9 +215,10 @@ constructor(
if (targets.isNotEmpty()) {
val dumpables = dumpManager.getDumpables()
val buffers = dumpManager.getLogBuffers()
val tableBuffers = dumpManager.getTableLogBuffers()
targets.forEach { target ->
findTargetInCollection(target, dumpables, buffers)?.dump(pw, args)
findTargetInCollection(target, dumpables, buffers, tableBuffers)?.dump(pw, args)
}
} else {
if (args.listOnly) {
@@ -240,10 +241,12 @@ constructor(
target: String,
dumpables: Collection<DumpableEntry>,
logBuffers: Collection<LogBufferEntry>,
tableBuffers: Collection<TableLogBufferEntry>,
) =
sequence {
findBestTargetMatch(dumpables, target)?.let { yield(it) }
findBestTargetMatch(logBuffers, target)?.let { yield(it) }
findBestTargetMatch(tableBuffers, target)?.let { yield(it) }
}
.sortedBy { it.name }
.minByOrNull { it.name.length }
@@ -258,6 +261,11 @@ constructor(
entry.buffer.dump(pw, tailLength)
}
private fun dumpTableBuffer(buffer: TableLogBufferEntry, pw: PrintWriter, args: Array<String>) {
pw.preamble(buffer)
buffer.table.dump(pw, args)
}
private fun dumpConfig(pw: PrintWriter) {
pw.println("SystemUiServiceComponents configuration:")
pw.print("vendor component: ")
@@ -307,6 +315,7 @@ constructor(
pw.println("Special commands:")
pw.println("$ <invocation> dumpables")
pw.println("$ <invocation> buffers")
pw.println("$ <invocation> tables")
pw.println("$ <invocation> bugreport-critical")
pw.println("$ <invocation> bugreport-normal")
pw.println("$ <invocation> config")
@@ -316,6 +325,7 @@ constructor(
pw.println("$ <invocation> --list")
pw.println("$ <invocation> dumpables --list")
pw.println("$ <invocation> buffers --list")
pw.println("$ <invocation> tables --list")
pw.println()
pw.println("Show only the most recent N lines of buffers")
@@ -393,6 +403,14 @@ constructor(
when (this) {
is DumpableEntry -> dumpDumpable(this, pw, args.rawArgs)
is LogBufferEntry -> dumpBuffer(this, pw, args.tailLength)
is TableLogBufferEntry -> dumpTableBuffer(this, pw, args.rawArgs)
}
private fun Collection<DumpsysEntry>.listOrDumpEntries(pw: PrintWriter, args: ParsedArgs) =
if (args.listOnly) {
listTargetNames(this, pw)
} else {
forEach { it.dump(pw, args) }
}
companion object {
@@ -432,7 +450,8 @@ constructor(
when (entry) {
// Historically TableLogBuffer was not separate from dumpables, so they have the
// same header
is DumpableEntry -> {
is DumpableEntry,
is TableLogBufferEntry -> {
println()
println(entry.name)
println(DUMPSYS_DUMPABLE_DIVIDER)
@@ -463,6 +482,15 @@ constructor(
entry.buffer.dump(pw, 0)
}
/**
* Zero-arg utility to write a [TableLogBufferEntry] to the given [PrintWriter] in a
* dumpsys-appropriate format.
*/
private fun dumpTableBuffer(entry: TableLogBufferEntry, pw: PrintWriter) {
pw.preamble(entry)
entry.table.dump(pw, arrayOf())
}
/**
* Zero-arg utility to write a [DumpsysEntry] to the given [PrintWriter] in a
* dumpsys-appropriate format.
@@ -471,6 +499,7 @@ constructor(
when (this) {
is DumpableEntry -> dumpDumpable(this, pw)
is LogBufferEntry -> dumpBuffer(this, pw)
is TableLogBufferEntry -> dumpTableBuffer(this, pw)
}
}
@@ -484,7 +513,15 @@ constructor(
private val PRIORITY_OPTIONS = arrayOf(PRIORITY_ARG_CRITICAL, PRIORITY_ARG_NORMAL)
private val COMMANDS =
arrayOf("bugreport-critical", "bugreport-normal", "buffers", "dumpables", "config", "help")
arrayOf(
"bugreport-critical",
"bugreport-normal",
"buffers",
"dumpables",
"tables",
"config",
"help"
)
private class ParsedArgs(val rawArgs: Array<String>, val nonFlagArgs: List<String>) {
var dumpPriority: String? = null

View File

@@ -20,7 +20,9 @@ import com.android.systemui.Dumpable
import com.android.systemui.ProtoDumpable
import com.android.systemui.dump.DumpsysEntry.DumpableEntry
import com.android.systemui.dump.DumpsysEntry.LogBufferEntry
import com.android.systemui.dump.DumpsysEntry.TableLogBufferEntry
import com.android.systemui.log.LogBuffer
import com.android.systemui.log.table.TableLogBuffer
import java.util.TreeMap
import javax.inject.Inject
import javax.inject.Singleton
@@ -39,6 +41,7 @@ open class DumpManager @Inject constructor() {
// NOTE: Using TreeMap ensures that iteration is in a predictable & alphabetical order.
private val dumpables: MutableMap<String, DumpableEntry> = TreeMap()
private val buffers: MutableMap<String, LogBufferEntry> = TreeMap()
private val tableLogBuffers: MutableMap<String, TableLogBufferEntry> = TreeMap()
/** See [registerCriticalDumpable]. */
fun registerCriticalDumpable(module: Dumpable) {
@@ -123,15 +126,28 @@ open class DumpManager @Inject constructor() {
throw IllegalArgumentException("'$name' is already registered")
}
buffers[name] = LogBufferEntry(buffer, name)
}
/** Register a [TableLogBuffer] to be dumped during a bugreport */
@Synchronized
fun registerTableLogBuffer(name: String, buffer: TableLogBuffer) {
if (!canAssignToNameLocked(name, buffer)) {
throw IllegalArgumentException("'$name' is already registered")
}
// All buffers must be priority NORMAL, not CRITICAL, because they often contain a lot of
// data.
buffers[name] = LogBufferEntry(buffer, name, DumpPriority.NORMAL)
tableLogBuffers[name] = TableLogBufferEntry(buffer, name)
}
@Synchronized fun getDumpables(): Collection<DumpableEntry> = dumpables.values.toList()
@Synchronized fun getLogBuffers(): Collection<LogBufferEntry> = buffers.values.toList()
@Synchronized
fun getTableLogBuffers(): Collection<TableLogBufferEntry> = tableLogBuffers.values.toList()
@Synchronized
fun freezeBuffers() {
for (buffer in buffers.values) {
@@ -147,7 +163,8 @@ open class DumpManager @Inject constructor() {
}
private fun canAssignToNameLocked(name: String, newDumpable: Any): Boolean {
val existingDumpable = dumpables[name]?.dumpable ?: buffers[name]?.buffer
val existingDumpable =
dumpables[name]?.dumpable ?: buffers[name]?.buffer ?: tableLogBuffers[name]?.table
return existingDumpable == null || newDumpable == existingDumpable
}
}

View File

@@ -18,6 +18,7 @@ package com.android.systemui.dump
import com.android.systemui.Dumpable
import com.android.systemui.log.LogBuffer
import com.android.systemui.log.table.TableLogBuffer
/**
* A DumpsysEntry is a named, registered entry tracked by [DumpManager] which can be addressed and
@@ -44,6 +45,18 @@ sealed interface DumpsysEntry {
data class LogBufferEntry(
val buffer: LogBuffer,
override val name: String,
override val priority: DumpPriority,
) : DumpsysEntry
) : DumpsysEntry {
// All buffers must be priority NORMAL, not CRITICAL, because they often contain a lot of
// data.
override val priority: DumpPriority = DumpPriority.NORMAL
}
data class TableLogBufferEntry(
val table: TableLogBuffer,
override val name: String,
) : DumpsysEntry {
// All buffers must be priority NORMAL, not CRITICAL, because they often contain a lot of
// data.
override val priority: DumpPriority = DumpPriority.NORMAL
}
}

View File

@@ -61,7 +61,7 @@ constructor(
bgDispatcher,
coroutineScope,
)
dumpManager.registerNormalDumpable(name, tableBuffer)
dumpManager.registerTableLogBuffer(name, tableBuffer)
tableBuffer.init()
return tableBuffer
}

View File

@@ -22,6 +22,7 @@ import com.android.systemui.Dumpable
import com.android.systemui.ProtoDumpable
import com.android.systemui.SysuiTestCase
import com.android.systemui.log.LogBuffer
import com.android.systemui.log.table.TableLogBuffer
import com.android.systemui.util.mockito.any
import com.android.systemui.util.mockito.eq
import com.google.common.truth.Truth.assertThat
@@ -67,6 +68,11 @@ class DumpHandlerTest : SysuiTestCase() {
@Mock
private lateinit var buffer2: LogBuffer
@Mock
private lateinit var table1: TableLogBuffer
@Mock
private lateinit var table2: TableLogBuffer
private val dumpManager = DumpManager()
@Before
@@ -91,9 +97,11 @@ class DumpHandlerTest : SysuiTestCase() {
dumpManager.registerCriticalDumpable("dumpable3", dumpable3)
dumpManager.registerBuffer("buffer1", buffer1)
dumpManager.registerBuffer("buffer2", buffer2)
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN some of them are dumped explicitly
val args = arrayOf("dumpable1", "dumpable3", "buffer2")
val args = arrayOf("dumpable1", "dumpable3", "buffer2", "table2")
dumpHandler.dump(fd, pw, args)
// THEN only the requested ones have their dump() method called
@@ -104,6 +112,8 @@ class DumpHandlerTest : SysuiTestCase() {
verify(dumpable3).dump(pw, args)
verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt())
verify(buffer2).dump(pw, 0)
verify(table1, never()).dump(any(), any())
verify(table2).dump(pw, args)
}
@Test
@@ -127,6 +137,8 @@ class DumpHandlerTest : SysuiTestCase() {
dumpManager.registerNormalDumpable("dumpable3", dumpable3)
dumpManager.registerBuffer("buffer1", buffer1)
dumpManager.registerBuffer("buffer2", buffer2)
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN a critical dump is requested
val args = arrayOf("--dump-priority", "CRITICAL")
@@ -140,6 +152,8 @@ class DumpHandlerTest : SysuiTestCase() {
any(Array<String>::class.java))
verify(buffer1, never()).dump(any(PrintWriter::class.java), anyInt())
verify(buffer2, never()).dump(any(PrintWriter::class.java), anyInt())
verify(table1, never()).dump(any(), any())
verify(table2, never()).dump(any(), any())
}
@Test
@@ -150,6 +164,8 @@ class DumpHandlerTest : SysuiTestCase() {
dumpManager.registerNormalDumpable("dumpable3", dumpable3)
dumpManager.registerBuffer("buffer1", buffer1)
dumpManager.registerBuffer("buffer2", buffer2)
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN a normal dump is requested
val args = arrayOf("--dump-priority", "NORMAL")
@@ -165,6 +181,8 @@ class DumpHandlerTest : SysuiTestCase() {
verify(dumpable3).dump(pw, args)
verify(buffer1).dump(pw, 0)
verify(buffer2).dump(pw, 0)
verify(table1).dump(pw, args)
verify(table2).dump(pw, args)
}
@Test
@@ -181,33 +199,39 @@ class DumpHandlerTest : SysuiTestCase() {
@Test
fun testDumpBuffers() {
// GIVEN a variety of registered dumpables and buffers
// GIVEN a variety of registered dumpables and buffers and tables
dumpManager.registerCriticalDumpable("dumpable1", dumpable1)
dumpManager.registerCriticalDumpable("dumpable2", dumpable2)
dumpManager.registerNormalDumpable("dumpable3", dumpable3)
dumpManager.registerBuffer("buffer1", buffer1)
dumpManager.registerBuffer("buffer2", buffer2)
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN a buffer dump is requested
val args = arrayOf("buffers", "--tail", "1")
dumpHandler.dump(fd, pw, args)
// THEN all buffers are dumped (and no dumpables)
// THEN all buffers are dumped (and no dumpables or tables)
verify(dumpable1, never()).dump(any(), any())
verify(dumpable2, never()).dump(any(), any())
verify(dumpable3, never()).dump(any(), any())
verify(buffer1).dump(pw, tailLength = 1)
verify(buffer2).dump(pw, tailLength = 1)
verify(table1, never()).dump(any(), any())
verify(table2, never()).dump(any(), any())
}
@Test
fun testDumpDumpables() {
// GIVEN a variety of registered dumpables and buffers
// GIVEN a variety of registered dumpables and buffers and tables
dumpManager.registerCriticalDumpable("dumpable1", dumpable1)
dumpManager.registerCriticalDumpable("dumpable2", dumpable2)
dumpManager.registerNormalDumpable("dumpable3", dumpable3)
dumpManager.registerBuffer("buffer1", buffer1)
dumpManager.registerBuffer("buffer2", buffer2)
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN a dumpable dump is requested
val args = arrayOf("dumpables")
@@ -219,8 +243,34 @@ class DumpHandlerTest : SysuiTestCase() {
verify(dumpable3).dump(pw, args)
verify(buffer1, never()).dump(any(), anyInt())
verify(buffer2, never()).dump(any(), anyInt())
verify(table1, never()).dump(any(), any())
verify(table2, never()).dump(any(), any())
}
@Test
fun testDumpTables() {
// GIVEN a variety of registered dumpables and buffers and tables
dumpManager.registerCriticalDumpable("dumpable1", dumpable1)
dumpManager.registerCriticalDumpable("dumpable2", dumpable2)
dumpManager.registerNormalDumpable("dumpable3", dumpable3)
dumpManager.registerBuffer("buffer1", buffer1)
dumpManager.registerBuffer("buffer2", buffer2)
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN a dumpable dump is requested
val args = arrayOf("tables")
dumpHandler.dump(fd, pw, args)
// THEN all dumpables are dumped (both critical and normal) (and no dumpables)
verify(dumpable1, never()).dump(any(), any())
verify(dumpable2, never()).dump(any(), any())
verify(dumpable3, never()).dump(any(), any())
verify(buffer1, never()).dump(any(), anyInt())
verify(buffer2, never()).dump(any(), anyInt())
verify(table1).dump(pw, args)
verify(table2).dump(pw, args)
}
@Test
fun testDumpAllProtoDumpables() {

View File

@@ -20,6 +20,7 @@ import androidx.test.filters.SmallTest
import com.android.systemui.Dumpable
import com.android.systemui.SysuiTestCase
import com.android.systemui.log.LogBuffer
import com.android.systemui.log.table.TableLogBuffer
import com.google.common.truth.Truth.assertThat
import org.junit.Assert.assertThrows
import org.junit.Before
@@ -37,6 +38,9 @@ class DumpManagerTest : SysuiTestCase() {
@Mock private lateinit var buffer1: LogBuffer
@Mock private lateinit var buffer2: LogBuffer
@Mock private lateinit var table1: TableLogBuffer
@Mock private lateinit var table2: TableLogBuffer
private val dumpManager = DumpManager()
@Before
@@ -81,11 +85,25 @@ class DumpManagerTest : SysuiTestCase() {
assertThat(dumpables).containsExactly(buffer1, buffer2)
}
@Test
fun testRegister_tableLogBuffers() {
// GIVEN a set of registered buffers
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN the collection is requested
val tables = dumpManager.getTableLogBuffers().map { it.table }
// THEN it contains the registered entries
assertThat(tables).containsExactly(table1, table2)
}
@Test
fun registerDumpable_throwsWhenNameCannotBeAssigned() {
// GIVEN dumpable1 and buffer1 are registered
// GIVEN dumpable1 and buffer1 and table1 are registered
dumpManager.registerCriticalDumpable("dumpable1", dumpable1)
dumpManager.registerBuffer("buffer1", buffer1)
dumpManager.registerTableLogBuffer("table1", table1)
// THEN an exception is thrown when trying to re-register a new dumpable under the same key
assertThrows(IllegalArgumentException::class.java) {
@@ -94,6 +112,9 @@ class DumpManagerTest : SysuiTestCase() {
assertThrows(IllegalArgumentException::class.java) {
dumpManager.registerBuffer("buffer1", buffer2)
}
assertThrows(IllegalArgumentException::class.java) {
dumpManager.registerTableLogBuffer("table1", table2)
}
}
@Test
@@ -149,4 +170,23 @@ class DumpManagerTest : SysuiTestCase() {
// AND the previous collection is unchanged
assertThat(buffers).hasSize(2)
}
@Test
fun getTableBuffers_returnsSafeCollection() {
// GIVEN a set of registered buffers
dumpManager.registerTableLogBuffer("table1", table1)
dumpManager.registerTableLogBuffer("table2", table2)
// WHEN the collection is requested
val tables = dumpManager.getTableLogBuffers()
// WHEN the collection changes
dumpManager.registerTableLogBuffer("table3", table1)
// THEN the new entry is represented
assertThat(dumpManager.getTableLogBuffers()).hasSize(3)
// AND the previous collection is unchanged
assertThat(tables).hasSize(2)
}
}

View File

@@ -73,8 +73,8 @@ class LogEulogizerTest : SysuiTestCase() {
@Before
fun setUp() {
MockitoAnnotations.initMocks(this)
logBufferEntry1 = DumpsysEntry.LogBufferEntry(logBuffer1, "logbuffer1", DumpPriority.NORMAL)
logBufferEntry2 = DumpsysEntry.LogBufferEntry(logBuffer2, "logbuffer2", DumpPriority.NORMAL)
logBufferEntry1 = DumpsysEntry.LogBufferEntry(logBuffer1, "logbuffer1")
logBufferEntry2 = DumpsysEntry.LogBufferEntry(logBuffer2, "logbuffer2")
eulogizer = LogBufferEulogizer(dumpManager, clock, files, path, MIN_WRITE_GAP, MAX_READ_AGE)