Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possibility to exclude the lists of files in order to have smaller reports #101

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -88,8 +88,8 @@ class ReleaseReportTest {
downloadSize += component.downloadSize
installSize += component.installSize
}
assertThat(downloadSize).isEqualTo(report.downloadSize)
assertThat(installSize).isEqualTo(report.installSize)
assertThat(downloadSize).isEqualTo(report.insights.appDownloadSize)
assertThat(installSize).isEqualTo(report.insights.appInstallSize)
}

@Test
Expand Down
Expand Up @@ -52,8 +52,12 @@ fun RBuilder.containerList(containers: List<FileContainer>, sizeType: Measurable
@Suppress("UNUSED_PARAMETER")
fun RBuilder.containerListItem(id: Int, container: FileContainer, sizeType: Measurable.SizeType, @RKey key: String) {
div(classes = "accordion-item") {
containerListItemHeader(id, container, sizeType)
containerListItemBody(id, container, sizeType)
if (container.files.isEmpty()) {
containerWithoutFilesListItemHeader(container, sizeType)
} else {
containerListItemHeader(id, container, sizeType)
containerListItemBody(id, container, sizeType)
}
Comment on lines +55 to +60
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UI change: if there isn't any list of files for a given component, it won't be shown as an expandable/collapsable row

}
}

Expand All @@ -63,15 +67,27 @@ fun RBuilder.containerListItemHeader(id: Int, container: FileContainer, sizeType
button(classes = "accordion-button collapsed") {
attrs["data-bs-toggle"] = "collapse"
attrs["data-bs-target"] = "#module-$id-body"
span(classes = "font-monospace text-truncate me-3") { +container.name }
container.owner?.let { owner -> span(classes = "badge bg-secondary me-3") { +owner } }
span(classes = "ms-auto me-3 text-nowrap") {
+formatSize(container, sizeType)
}
containerHeader(container, sizeType)
}
}
}

@RFunction
fun RBuilder.containerWithoutFilesListItemHeader(container: FileContainer, sizeType: Measurable.SizeType) {
div(classes = "list-group-item d-flex border-0") {
containerHeader(container, sizeType)
}
}

@RFunction
fun RBuilder.containerHeader(container: FileContainer, sizeType: Measurable.SizeType) {
span(classes = "font-monospace text-truncate me-3") { +container.name }
container.owner?.let { owner -> span(classes = "badge bg-secondary me-3") { +owner.name } }
span(classes = "ms-auto me-3 text-nowrap") {
+formatSize(container, sizeType)
}
}

@RFunction
fun RBuilder.containerListItemBody(id: Int, container: FileContainer, sizeType: Measurable.SizeType) {
div(classes = "accordion-collapse collapse") {
Expand Down
Expand Up @@ -44,13 +44,13 @@ import react.useState
fun RBuilder.report(report: AppReport) {
val (sizeType, setSizeType) = useState(Measurable.SizeType.DOWNLOAD)

val hasOwnershipInfo = report.components.any { component -> component.owner != null }
val hasOwnershipInfo = report.ownershipOverview?.isNotEmpty() == true
val hasDynamicFeatures = report.dynamicFeatures.isNotEmpty()

val tabs = listOf(
Tab("/", "Breakdown") { breakdown(report.components, sizeType) },
Tab("/insights", "Insights") { insights(report.components) },
Tab("/ownership", "Ownership", hasOwnershipInfo) { ownership(report.components, sizeType) },
Tab("/insights", "Insights") { insights(report.insights) },
Tab("/ownership", "Ownership", hasOwnershipInfo) { ownership(report.components, sizeType, report.ownershipOverview ?: emptyMap()) },
Comment on lines +52 to +53
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we propagate already calculated data to the frontend, it doesn't have to iterate over files to obtain the insights and ownership overview, that way we can get rid of files listing if needed

Tab("/dynamic", "Dynamic features", hasDynamicFeatures) { dynamicFeatures(report.dynamicFeatures, sizeType) },
)

Expand All @@ -74,8 +74,8 @@ fun RBuilder.header(report: AppReport) {
h3 { +report.name }
span(classes = "text-muted") { +"Version ${report.version} (${report.variant})" }
}
headerSizeItem(report.downloadSize, "Download size")
headerSizeItem(report.installSize, "Install size")
headerSizeItem(report.insights.appDownloadSize, "Download size")
headerSizeItem(report.insights.appInstallSize, "Install size")
}
}

Expand Down
Expand Up @@ -22,8 +22,11 @@ import com.spotify.ruler.frontend.chart.ChartConfig
import com.spotify.ruler.frontend.chart.BarChartConfig
import com.spotify.ruler.frontend.formatSize
import com.spotify.ruler.frontend.chart.seriesOf
import com.spotify.ruler.models.AppComponent
import com.spotify.ruler.models.Measurable
import com.spotify.ruler.models.ComponentType
import com.spotify.ruler.models.FileType
import com.spotify.ruler.models.Insights
import com.spotify.ruler.models.TypeInsights
import com.spotify.ruler.models.ResourceType
import kotlinx.browser.document
import kotlinx.html.id
import react.RBuilder
Expand All @@ -33,30 +36,30 @@ import react.dom.p
import react.useEffect

@RFunction
fun RBuilder.insights(components: List<AppComponent>) {
fun RBuilder.insights(insights: Insights) {
div(classes = "row mb-3") {
fileTypeGraphs(components)
fileTypeGraphs(insights.fileTypeInsights)
}
div(classes = "row") {
componentTypeGraphs(components)
componentTypeGraphs(insights.componentTypeInsights)
}
div(classes = "row") {
resourcesTypeGraphs(components)
resourcesTypeGraphs(insights.resourcesTypeInsights)
}
}

@RFunction
fun RBuilder.fileTypeGraphs(components: List<AppComponent>) {
val labels = arrayOf("Classes", "Resources", "Assets", "Native libraries", "Other")
fun RBuilder.fileTypeGraphs(fileTypeInsights: Map<FileType, TypeInsights>) {
val labels = FileType.values().map { it.label }.toTypedArray()
val downloadSizes = LongArray(labels.size)
val installSizes = LongArray(labels.size)
val fileCounts = LongArray(labels.size)

components.flatMap(AppComponent::files).forEach { file ->
val index = file.type.ordinal
downloadSizes[index] += file.getSize(Measurable.SizeType.DOWNLOAD)
installSizes[index] += file.getSize(Measurable.SizeType.INSTALL)
fileCounts[index]++
fileTypeInsights.forEach { (fileType, insights) ->
val index = fileType.ordinal
downloadSizes[index] = insights.downloadSize
installSizes[index] = insights.installSize
fileCounts[index] = insights.count
Comment on lines +58 to +62
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

insights are already calculated, here we are just populating the arrays passed to the frontend. Same for the two bellow methods

}

chart(
Expand All @@ -83,17 +86,17 @@ fun RBuilder.fileTypeGraphs(components: List<AppComponent>) {
}

@RFunction
fun RBuilder.componentTypeGraphs(components: List<AppComponent>) {
val labels = arrayOf("Internal", "External")
fun RBuilder.componentTypeGraphs(componentTypeInsights: Map<ComponentType, TypeInsights>) {
val labels = ComponentType.values().map { it.label }.toTypedArray()
val downloadSizes = LongArray(labels.size)
val installSizes = LongArray(labels.size)
val fileCounts = LongArray(labels.size)

components.forEach { component ->
val index = component.type.ordinal
downloadSizes[index] += component.getSize(Measurable.SizeType.DOWNLOAD)
installSizes[index] += component.getSize(Measurable.SizeType.INSTALL)
fileCounts[index]++
componentTypeInsights.forEach { (componentType, insights) ->
val index = componentType.ordinal
downloadSizes[index] = insights.downloadSize
installSizes[index] = insights.installSize
fileCounts[index] = insights.count
}

chart(
Expand Down Expand Up @@ -122,17 +125,17 @@ fun RBuilder.componentTypeGraphs(components: List<AppComponent>) {
}

@RFunction
fun RBuilder.resourcesTypeGraphs(components: List<AppComponent>) {
val labels = arrayOf("Drawable", "Layout", "Raw", "Values", "Font", "Other")
fun RBuilder.resourcesTypeGraphs(resourcesTypeInsights: Map<ResourceType, TypeInsights>) {
val labels = ResourceType.values().map { it.label }.toTypedArray()
val downloadSizes = LongArray(labels.size)
val installSizes = LongArray(labels.size)
val fileCounts = LongArray(labels.size)

components.flatMap(AppComponent::files).filter { it.resourceType != null }.forEach { file ->
val index = file.resourceType!!.ordinal
downloadSizes[index] += file.getSize(Measurable.SizeType.DOWNLOAD)
installSizes[index] += file.getSize(Measurable.SizeType.INSTALL)
fileCounts[index]++
resourcesTypeInsights.forEach { (resourceType, insights) ->
val index = resourceType.ordinal
downloadSizes[index] = insights.downloadSize
installSizes[index] = insights.installSize
fileCounts[index] = insights.count
}

chart(
Expand Down
Expand Up @@ -22,9 +22,11 @@ import com.spotify.ruler.frontend.chart.BarChartConfig
import com.spotify.ruler.frontend.chart.seriesOf
import com.spotify.ruler.frontend.formatSize
import com.spotify.ruler.models.AppComponent
import com.spotify.ruler.models.AppFile
import com.spotify.ruler.models.ComponentType
import com.spotify.ruler.models.Measurable
import com.spotify.ruler.models.OwnedComponentSize
import com.spotify.ruler.models.ComponentOwner
import com.spotify.ruler.models.OwnershipOverview
import react.RBuilder
import react.dom.div
import react.dom.h4
Expand All @@ -34,25 +36,21 @@ import react.useState
const val PAGE_SIZE = 10

@RFunction
fun RBuilder.ownership(components: List<AppComponent>, sizeType: Measurable.SizeType) {
componentOwnershipOverview(components)
componentOwnershipPerTeam(components, sizeType)
fun RBuilder.ownership(
components: List<AppComponent>,
sizeType: Measurable.SizeType,
ownershipOverview: Map<String, OwnershipOverview>,
) {
componentOwnershipOverview(ownershipOverview)
componentOwnershipPerTeam(components, sizeType, ownershipOverview)
}

@RFunction
fun RBuilder.componentOwnershipOverview(components: List<AppComponent>) {
val sizes = mutableMapOf<String, Measurable.Mutable>()
components.flatMap(AppComponent::files).forEach { file ->
val owner = file.owner ?: return@forEach
val current = sizes.getOrPut(owner) { Measurable.Mutable(0, 0) }
current.downloadSize += file.downloadSize
current.installSize += file.installSize
}

val sorted = sizes.entries.sortedByDescending { (_, measurable) -> measurable.downloadSize }
fun RBuilder.componentOwnershipOverview(ownershipOverview: Map<String, OwnershipOverview>) {
val sorted = ownershipOverview.entries.sortedByDescending { (_, ownership) -> ownership.totalInstallSize }
val owners = sorted.map { (owner, _) -> owner }
val downloadSizes = sorted.map { (_, measurable) -> measurable.downloadSize }
val installSizes = sorted.map { (_, measurable) -> measurable.installSize }
val downloadSizes = sorted.map { (_, ownership) -> ownership.totalDownloadSize }
val installSizes = sorted.map { (_, ownership) -> ownership.totalInstallSize }
Comment on lines -43 to +53
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Owners chart data is already calculated as well


pagedContent(owners.size, PAGE_SIZE) { pageStartIndex, pageEndIndex ->
chart(
Expand All @@ -74,44 +72,60 @@ fun RBuilder.componentOwnershipOverview(components: List<AppComponent>) {
}

@RFunction
fun RBuilder.componentOwnershipPerTeam(components: List<AppComponent>, sizeType: Measurable.SizeType) {
fun RBuilder.componentOwnershipPerTeam(
components: List<AppComponent>,
sizeType: Measurable.SizeType,
ownershipOverview: Map<String, OwnershipOverview>,
) {
val files = components.flatMap(AppComponent::files)
val owners = files.mapNotNull(AppFile::owner).distinct().sorted()
val owners = ownershipOverview.keys.sorted()
var selectedOwner by useState(owners.first())

val ownedComponents = components.filter { component -> component.owner == selectedOwner }
val ownedComponents = components.filter { component -> component.owner?.name == selectedOwner }
val ownedFiles = files.filter { file -> file.owner == selectedOwner }
val ownedFilesCount = ownershipOverview[selectedOwner]?.filesCount ?: 0

val totalOwnedDownloadSize = ownershipOverview[selectedOwner]?.totalDownloadSize ?: 0
val totalOwnedInstallSize = ownershipOverview[selectedOwner]?.totalInstallSize ?: 0
val remainingOwnedDownloadSize = ownershipOverview[selectedOwner]?.filesFromNotOwnedComponentsDownloadSize ?: 0
val remainingOwnedInstallSize = ownershipOverview[selectedOwner]?.filesFromNotOwnedComponentsInstallSize ?: 0

val remainingOwnedFiles = ownedFiles.toMutableSet()
val processedComponents = ownedComponents.map { component ->
val ownedFilesFromComponent = component.files.filter { file -> file.owner == selectedOwner }
remainingOwnedFiles.removeAll(ownedFilesFromComponent)
component.copy(
downloadSize = ownedFilesFromComponent.sumOf(AppFile::downloadSize),
installSize = ownedFilesFromComponent.sumOf(AppFile::installSize),
downloadSize = component.owner?.ownedSize?.downloadSize ?: 0,
installSize = component.owner?.ownedSize?.installSize ?: 0,
files = ownedFilesFromComponent,
)
}.toMutableList()

// Group together all owned files which belong to components not owned by the currently selected owner
if (remainingOwnedFiles.isNotEmpty()) {
if (remainingOwnedDownloadSize > 0 || remainingOwnedInstallSize > 0) {
processedComponents += AppComponent(
name = "Other owned files",
type = ComponentType.INTERNAL,
downloadSize = remainingOwnedFiles.sumOf(AppFile::downloadSize),
installSize = remainingOwnedFiles.sumOf(AppFile::installSize),
downloadSize = remainingOwnedDownloadSize,
installSize = remainingOwnedInstallSize,
files = remainingOwnedFiles.toList(),
owner = selectedOwner,
owner = ComponentOwner(
name = selectedOwner,
ownedSize = OwnedComponentSize(
downloadSize = remainingOwnedDownloadSize,
installSize = remainingOwnedInstallSize,
),
),
)
}

h4(classes = "mb-3 mt-4") { +"Components and files grouped by owner" }
dropdown(owners, "owner-dropdown") { owner -> selectedOwner = owner }
div(classes = "row mt-4 mb-4") {
highlightedValue(ownedComponents.size, "Component(s)")
highlightedValue(ownedFiles.size, "File(s)")
highlightedValue(ownedFiles.sumOf(AppFile::downloadSize), "Download size", ::formatSize)
highlightedValue(ownedFiles.sumOf(AppFile::installSize), "Install size", ::formatSize)
highlightedValue(ownedFilesCount, "File(s)")
highlightedValue(totalOwnedDownloadSize, "Download size", ::formatSize)
highlightedValue(totalOwnedInstallSize, "Install size", ::formatSize)
}
containerList(processedComponents, sizeType)
}
Expand Down
Expand Up @@ -29,8 +29,11 @@ open class RulerExtension(objects: ObjectFactory) {
val ownershipFile: RegularFileProperty = objects.fileProperty()
val defaultOwner: Property<String> = objects.property(String::class.java)

val shouldExcludeFileListing: Property<Boolean> = objects.property(Boolean::class.java)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit - let's go for something like omitFileBreakdown here.


// Set up default values
init {
defaultOwner.convention("unknown")
shouldExcludeFileListing.convention(false)
Comment on lines +32 to +37
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the new ruler optional config

}
}
Expand Up @@ -53,6 +53,8 @@ class RulerPlugin : Plugin<Project> {
task.workingDir.set(project.layout.buildDirectory.dir("intermediates/ruler/${variant.name}"))
task.reportDir.set(project.layout.buildDirectory.dir("reports/ruler/${variant.name}"))

task.shouldExcludeFileListing.set(rulerExtension.shouldExcludeFileListing)

// Add explicit dependency to support DexGuard
task.dependsOn("bundle$variantName")
}
Expand All @@ -64,7 +66,7 @@ class RulerPlugin : Plugin<Project> {
private fun getAppInfo(project: Project, variant: ApplicationVariant) = project.provider {
AppInfo(
applicationId = variant.applicationId.get(),
versionName = variant.outputs.first().versionName.get() ?: "-",
versionName = variant.outputs.first().versionName.orNull ?: "-",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated: fixing IDE warning: unnecessary elvis operator -> using orNull instead

variantName = variant.name,
)
}
Expand Down
Expand Up @@ -69,6 +69,9 @@ abstract class RulerTask : DefaultTask() {
@get:Input
abstract val defaultOwner: Property<String>

@get:Input
abstract val shouldExcludeFileListing: Property<Boolean>

@get:OutputDirectory
abstract val workingDir: DirectoryProperty

Expand Down Expand Up @@ -132,7 +135,14 @@ abstract class RulerTask : DefaultTask() {
val reportDir = reportDir.asFile.get()

val jsonReporter = JsonReporter()
val jsonReport = jsonReporter.generateReport(appInfo.get(), components, features, ownershipInfo, reportDir)
val jsonReport = jsonReporter.generateReport(
appInfo.get(),
components,
features,
ownershipInfo,
shouldExcludeFileListing.get(),
reportDir,
)
project.logger.lifecycle("Wrote JSON report to ${jsonReport.toPath().toUri()}")

val htmlReporter = HtmlReporter()
Expand Down