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

yamal-coding · 2022-11-16T17:38:30Z

What has changed

Possibility to exclude the list of files of each component or dynamic feature in order to decrease the size of the report (both HTML and JSON).

The PR is not small so I will try to explain how I did it:

Decouple the calculus of insights and ownership from the frontend and let the JsonReporter to precalculate that data, that way, the data will remain the same even if we do not pass any list of files. I have done that in two main commits:
- 8f3492a: Calculate insights before rendering
- 6ce88b3: Calculate ownership before rendering
Add a new property in the RulerExtension class (false by default) to expose a way to exclude files when desired. Done in fbe92f7
Update UI so we show non-expandable rows if there are no files to show. Done in d08998c

Files exclusion would be configured like this:

ruler {
    // … other ruler configuration.
    shouldExcludeFileListing.set(true) // false by default
}

Why was it changed

There are a couple of open issues at the time this PR is being opened regarding the size of the report. Maybe we could try to improve the generation of the Javascript and HTML code but it would be a small optimization since the main reason behind this problem seems to be the huge amount of files that belongs to the application components.

Removing that file listing entirely is not an option because it is a very useful information if we are looking for a file that may be heavily increasing our app size.

The solution to that problem that I’m proposing here is to give the developers the chance to ignore that files if they are struggling with report size on their CI machines (for example). The data reported (overview, breakdown, charts, etc) is not affected by this exclusion, the only difference when enabling this option is that it won’t be possible to expand a component and see its files.

In case some report without files shows some unexpected download/install size for a component or the whole app, a new execution of the task without this flag would be enough to have the complete report.

I have tested my proposal in a project with 286 components and the report has been reduced from 15.6MB down to 2.1MB.

Next steps: If you think this is something that makes sense, an iteration would be to allow a whitelist of components whose files won't be excluded.

I hope you find this useful :)

Related issues

#96
#74 (this specific issue is not directly related with file size, but with this new optional config, performance is improved as well)

yamal-coding · 2022-11-16T17:43:16Z

ruler-frontend/src/main/kotlin/com/spotify/ruler/frontend/components/Breakdown.kt

+        if (container.files.isEmpty()) {
+            containerWithoutFilesListItemHeader(container, sizeType)
+        } else {
+            containerListItemHeader(id, container, sizeType)
+            containerListItemBody(id, container, sizeType)
+        }


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

yamal-coding · 2022-11-16T17:45:36Z

ruler-frontend/src/main/kotlin/com/spotify/ruler/frontend/components/Common.kt

+        Tab("/insights", "Insights") { insights(report.insights) },
+        Tab("/ownership", "Ownership", hasOwnershipInfo) { ownership(report.components, sizeType, report.ownershipOverview ?: emptyMap()) },


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

yamal-coding · 2022-11-16T17:46:42Z

ruler-frontend/src/main/kotlin/com/spotify/ruler/frontend/components/Insights.kt

+    fileTypeInsights.forEach { (fileType, insights) ->
+        val index = fileType.ordinal
+        downloadSizes[index] = insights.downloadSize
+        installSizes[index] = insights.installSize
+        fileCounts[index] = insights.count


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

yamal-coding · 2022-11-16T17:47:45Z

ruler-frontend/src/main/kotlin/com/spotify/ruler/frontend/components/Ownership.kt

-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 }


Owners chart data is already calculated as well

yamal-coding · 2022-11-16T17:48:49Z

ruler-gradle-plugin/src/main/kotlin/com/spotify/ruler/plugin/RulerExtension.kt

+    val shouldExcludeFileListing: Property<Boolean> = objects.property(Boolean::class.java)
+
    // Set up default values
    init {
        defaultOwner.convention("unknown")
+        shouldExcludeFileListing.convention(false)


this is the new ruler optional config

yamal-coding · 2022-11-16T17:49:48Z

ruler-gradle-plugin/src/main/kotlin/com/spotify/ruler/plugin/RulerPlugin.kt

@@ -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 ?: "-",


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

yamal-coding · 2022-11-16T17:51:19Z

ruler-models/src/commonMain/kotlin/com/spotify/ruler/models/AppReport.kt

-    override val downloadSize: Long,
-    override val installSize: Long,
    val components: List<AppComponent>,
    val dynamicFeatures: List<DynamicFeature>,
-) : Measurable
+    val insights: Insights,


downloadSize and installSize moved to new Insights class

yamal-coding · 2022-11-16T17:57:57Z

ruler-gradle-plugin/src/main/kotlin/com/spotify/ruler/plugin/report/JsonReporter.kt

@@ -38,6 +48,7 @@ class JsonReporter {
     * @param appInfo General info about the analyzed app.
     * @param components Map of app component names to their respective files
     * @param ownershipInfo Optional info about the owners of components.
+     * @param shouldExcludeFileListing Flag to determine if file listing should be included in the report


main change if this PR. I will try to explain the main idea:

New Insights field on AppReport: pre-calculated insights about the app (same info shown on "Insights" tab"). It contains the same calculated app sizes and sizes by type of file/component that were previously calculated on Breakdown.kt file

New ownershipOverview: Map<String, OwnershipOverview> field on AppReport: it holds data about the size of the app that belongs to each owner and the size of their files that doesn't belong to any of their owned components

New Owner class on FileContainer interface: it contains the name of the owner of a FileContainer and its corresponding owned size (it can be equal to the whole size of the component or less if that container contains files owned by other teams)

Filter out every file from components and dynamic features if shouldExcludeFileListing flag is true

simonschiller

Thanks a lot for the PR, I think it's a very good idea to add the possibility to exlucde the file breakdown. I feel like it would be better if we got rid of all the file-level information as well (if the flag is checked), including charts. This would make the code a lot simpler and would probably also be easier to understand for the user (do I want file-level stats or not). In my head, if the flag is set we would just omit the File type distribution charts and only compute ownership based on components.

On the technical side, we could just make the files list inside AppComponent nullable and set it to null if the flag is checked (inside the `JsonReporter. In the frontend we'd simply check if the list is null or not and omit certain things if it is. There should be no need to otherwise change how the JSON report looks like. Makes the whole thing a lot simpler, what do you think?

I get that you loose file-level graphs with this, but I think that's totally ok and might even avoid confusion (i.e. people looking at the graph and wondering where this data is coming from because it's not visible in the report).

simonschiller · 2022-11-17T18:39:10Z

ruler-gradle-plugin/src/main/kotlin/com/spotify/ruler/plugin/RulerExtension.kt

@@ -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)


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

yamal-coding · 2022-11-18T09:35:53Z

Thanks a lot for the PR, I think it's a very good idea to add the possibility to exlucde the file breakdown. I feel like it would be better if we got rid of all the file-level information as well (if the flag is checked), including charts. This would make the code a lot simpler and would probably also be easier to understand for the user (do I want file-level stats or not). In my head, if the flag is set we would just omit the File type distribution charts and only compute ownership based on components.

On the technical side, we could just make the files list inside AppComponent nullable and set it to null if the flag is checked (inside the `JsonReporter. In the frontend we'd simply check if the list is null or not and omit certain things if it is. There should be no need to otherwise change how the JSON report looks like. Makes the whole thing a lot simpler, what do you think?

I get that you loose file-level graphs with this, but I think that's totally ok and might even avoid confusion (i.e. people looking at the graph and wondering where this data is coming from because it's not visible in the report).

Hi! Thanks for the feedback, it sounds good! And you are right, it may be confusing to show charts about some files that are not visible. If the users did activate the flag because they don't care about them on the report, then it means that maybe the charts are not of their interest as well. Also: +1 to keep things simple on the technical side :)

Regarding which charts and displayed info are being omitted if exclusion flag is enabled, I would like to confirm that I'm in the same page as you before applying the new changes:

Breakdown wouldn't include any sublist of files (just like in my initial proposal)
File type distribution and Resource type distribution would be hidden. Only Component type distribution chart would remain inside Insights tab.
Ownership: you said that you would compute ownership based on components. We are ok on not including info about files from owners that do not belong to owned components? With current JsonReporter output (not the one in this PR) and setting files to null, we are not able to calculate those remaining files sizes and add them to the overview info of each owner. This could lead to situations where numbers don't add up because of the missing information of those specific files, and also some missing but expected ownership info.

Despite of my third point analysis, I'm ok with leaving it like that, because in the real world, if you ran a smaller report with this flag, it means that you only wanted a quick view of what's going on with your global app status. If you need more details you should better be generating a full report.

What do you think?

simonschiller · 2022-11-18T13:13:27Z

Yes, it would just show a list without each items being expandable (same for Dynamic features).
Exactly
I would just ignore this. Each component has an owner and we'd use that for calculation if file-level analysis is omitted. So if there is a file inside a component owned by a different team, we would ignore that and assign it to the team owning the component. That would mean numbers could change if you change between component-level to file-level, but I think that's acceptable. They should always add up to the same total though.

yamal-coding · 2022-11-18T13:25:50Z

Yes, it would just show a list without each items being expandable (same for Dynamic features).

Exactly

I would just ignore this. Each component has an owner and we'd use that for calculation if file-level analysis is omitted. So if there is a file inside a component owned by a different team, we would ignore that and assign it to the team owning the component. That would mean numbers could change if you change between component-level to file-level, but I think that's acceptable. They should always add up to the same total though.

Great 👍 I think I'm going to implement that new approach in a different PR because there are a lot of commits here that no longer apply and they are adding some noise..
While I'm working on it, I will leave this PR open so we can discuss possible discussions that may arise :) It will be closed once the new one is opened

yamal-coding · 2022-11-20T14:00:03Z

Thanks a lot for your feedback @simonschiller. I'm closing this PR and opening #102 with new changes

yamal-coding added 6 commits November 14, 2022 14:27

calculate insights before rendering

8f3492a

calculate ownership before rendering

6ce88b3

optional config for excluding file listing

fbe92f7

add missing javadoc

4d275e4

display component items as non collapsable if no files

d08998c

do not expose mutable class

7c57552

yamal-coding commented Nov 16, 2022

View reviewed changes

fix tests

f8b2989

simonschiller reviewed Nov 17, 2022

View reviewed changes

yamal-coding mentioned this pull request Nov 20, 2022

Possibility to omit file breakdown #102

Merged

yamal-coding closed this Nov 20, 2022

yamal-coding deleted the allow-file-exclusion branch November 20, 2022 14:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yamal-coding commented Nov 16, 2022 •

edited

Loading

yamal-coding Nov 16, 2022

yamal-coding Nov 16, 2022

yamal-coding Nov 16, 2022

yamal-coding Nov 16, 2022

yamal-coding Nov 16, 2022

yamal-coding Nov 16, 2022

yamal-coding Nov 16, 2022

yamal-coding Nov 16, 2022 •

edited

Loading

simonschiller left a comment

simonschiller Nov 17, 2022

yamal-coding commented Nov 18, 2022 •

edited

Loading

simonschiller commented Nov 18, 2022

yamal-coding commented Nov 18, 2022

yamal-coding commented Nov 20, 2022

		Tab("/insights", "Insights") { insights(report.insights) },
		Tab("/ownership", "Ownership", hasOwnershipInfo) { ownership(report.components, sizeType, report.ownershipOverview ?: emptyMap()) },

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

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

Conversation

yamal-coding commented Nov 16, 2022 • edited Loading

What has changed

Why was it changed

Related issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamal-coding Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

simonschiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yamal-coding commented Nov 18, 2022 • edited Loading

simonschiller commented Nov 18, 2022

yamal-coding commented Nov 18, 2022

yamal-coding commented Nov 20, 2022

yamal-coding commented Nov 16, 2022 •

edited

Loading

yamal-coding Nov 16, 2022 •

edited

Loading

yamal-coding commented Nov 18, 2022 •

edited

Loading