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 all commits
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://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/organizing-information-with-collapsed-sections
.appendLine("<summary>Changed in size</summary>")
.appendLine()
.append(tableHeader)
var artifactOrdinaryChange = false

analysis.metrics
.toList()
.sortedByDescending { it.second.percentage }
.toMap()
.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("%")
appendLine("|")
}

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