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.

Sign up for GitHub

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

Jump to bottom

fix: readability improvements to artifact size metrics #41

Merged
merged 3 commits into from
May 1, 2024
Merged

fix: readability improvements to artifact size metrics #41

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ef4930
fix: readability improvements
0marperez Apr 30, 2024
3ee1e93
Use full GitHub link
0marperez Apr 30, 2024
abe1da3
remove ⚠ & sort descending
0marperez Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 66 additions & 21 deletions ...in/kotlin/aws/sdk/kotlin/gradle/plugins/artifactsizemetrics/AnalyzeArtifactSizeMetrics.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import org.gradle.api.tasks.InputFile
import org.gradle.api.tasks.OutputFile
import org.gradle.api.tasks.TaskAction
import java.io.File
import kotlin.math.abs

/**
* Gradle task that analyzes/compares a project's local artifact size metrics to
Expand Down Expand Up @@ -59,8 +60,8 @@ internal abstract class AnalyzeArtifactSizeMetrics : DefaultTask() {
val analysis = analyzeArtifactSizeMetrics(latestReleaseMetrics, currentMetrics)

hasSignificantChangeFile.get().asFile.writeText(analysis.significantChange.toString())
val diffTable = createDiffTable(analysis)
val output = if (analysis.hasDelta) diffTable else noDiffMessage
val diffTables = createDiffTables(analysis)
val output = if (analysis.hasDelta) diffTables else noDiffMessage

this.analysisFile.get().asFile.writeText(output)
}
Expand Down Expand Up @@ -100,7 +101,7 @@ internal abstract class AnalyzeArtifactSizeMetrics : DefaultTask() {
)
}

val changeHappened = artifactSizeMetrics.values.any { it.delta != 0L }
val changeHappened = artifactSizeMetrics.values.any { it.delta.isNotaFluctuation() }
val significantChange = artifactSizeMetrics.values.any {
(it.percentage > pluginConfig.significantChangeThresholdPercentage) || // Increase in size above threshold
(it.latestReleaseSize == 0L) // New artifact
Expand All @@ -109,34 +110,73 @@ internal abstract class AnalyzeArtifactSizeMetrics : DefaultTask() {
return ArtifactSizeMetricsAnalysis(artifactSizeMetrics, significantChange, changeHappened)
}

// There are small fluctuations in artifact size that are not real delta
private fun Long.isNotaFluctuation() = abs(this) > 5L
Copy link
Member

Choose a reason for hiding this comment

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

question: Is this number big enough? In your example comment, there was a delta of 10 bytes that also seems like a fluctuation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm basing it off the fluctuations seen here: awslabs/aws-sdk-kotlin#1291. They're all less than 5. That 10 I added manually but I'm not opposed to upping the threshold

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, that seems fine then. I didn't realize the 10 was a manual data point

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're hiding the unimportant/no-action-required changes by default, do we still need to filter out these fluctuations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really necessary but I also wouldn't consider it real delta

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It pollutes an already large table


private data class ArtifactSizeMetricsAnalysis(
val metrics: Map<String, ArtifactSizeMetric>,
val significantChange: Boolean,
val hasDelta: Boolean,
)

private fun createDiffTable(analysis: ArtifactSizeMetricsAnalysis): String = buildString {
private fun createDiffTables(analysis: ArtifactSizeMetricsAnalysis): String = buildString {
appendLine("Affected Artifacts\n=")
appendLine("| Artifact |Pull Request (bytes) | Latest Release (bytes) | Delta (bytes) | Delta (percentage) |")
appendLine("| -------- | ------------------: | ---------------------: | ------------: | -----------------: |")
analysis.metrics.forEach { metric ->
if (metric.value.delta != 0L) {
append("|")
append(metric.key)
append("|")
if (metric.value.currentSize == 0L) append("(does not exist)") else append("%,d".format(metric.value.currentSize))
append("|")
if (metric.value.latestReleaseSize == 0L) append("(does not exist)") else append("%,d".format(metric.value.latestReleaseSize))
append("|")
append("%,d".format(metric.value.delta))
append("|")
append("%.2f".format(metric.value.percentage))
append("%")
appendLine("|")

val requiresAttention = StringBuilder()
.appendLine("Significantly increased in size")
.appendLine()
.append(tableHeader)
var artifactRequiresAttention = false

val ordinary = StringBuilder()
.appendLine("<details>") // See: https://tiny.amazon.com/zhuf9fhk/docsgithengetswritworkorga
.appendLine("<summary>Changed in size</summary>")
.appendLine()
.append(tableHeader)
var artifactOrdinaryChange = false

analysis.metrics
.toList()
.sortedBy { it.second.percentage }.toMap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This sorts in ascending order but I think we probably want descending order.

Style: When using fluent call patterns, put each chained property access and method call on its own line.

analysis
    .metrics
    .toList()
    .sortedByDescending { it.second.percentage }
    .toMap()
    .forEach { metric ->
        …

.forEach { metric ->
if (metric.value.delta.isNotaFluctuation()) {
val row = buildString {
append("|")
append(metric.key)
append("|")
if (metric.value.currentSize == 0L) append("(does not exist)") else append("%,d".format(metric.value.currentSize))
append("|")
if (metric.value.latestReleaseSize == 0L) append("(does not exist)") else append("%,d".format(metric.value.latestReleaseSize))
append("|")
append("%,d".format(metric.value.delta))
append("|")
append("%.2f".format(metric.value.percentage))
append("%")
if (metric.value.requiresAttention()) append("⚠️")
appendLine("|")
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is the ⚠️ necessary now that there are two tables? Won't every entry in the Significantly increased in size table have the ⚠️ next to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it looks good but I also see how it can be an eye sore. I'm open to getting rid of it

Copy link
Member

Choose a reason for hiding this comment

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

I think it's just redundant information, I support removing it

}

if (metric.value.requiresAttention()) {
requiresAttention.append(row)
artifactRequiresAttention = true
} else {
ordinary.append(row)
artifactOrdinaryChange = true
}
}
}
}

ordinary
.appendLine()
.append("</details>")

if (artifactRequiresAttention) appendLine(requiresAttention)
if (artifactOrdinaryChange) appendLine(ordinary)
}

private fun ArtifactSizeMetric.requiresAttention() =
this.percentage > pluginConfig.significantChangeThresholdPercentage || this.percentage.isNaN()

private data class ArtifactSizeMetric(
val currentSize: Long,
val latestReleaseSize: Long,
Expand All @@ -162,4 +202,9 @@ internal abstract class AnalyzeArtifactSizeMetrics : DefaultTask() {
=
No artifacts changed size
""".trimIndent()

private val tableHeader = buildString {
appendLine("| Artifact |Pull Request (bytes) | Latest Release (bytes) | Delta (bytes) | Delta (percentage) |")
appendLine("| -------- | ------------------: | ---------------------: | ------------: | -----------------: |")
}
}
Loading