Add percent-change histogram to metrics diff report#4247
Add percent-change histogram to metrics diff report#4247arnaud-lacurie wants to merge 4 commits into
Conversation
📊 Metrics Diff Analysis ReportSummary
ℹ️ About this analysisThis automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:
The last category in particular may indicate planner regressions that should be investigated. Only Metrics ChangedThese queries experienced only metrics changes without any plan changes. If these metrics have substantially changed, Total: 16 queries Statistical Summary (Only Metrics Changed)
There were no queries with significant regressions detected. Minor Changes (Only Metrics Changed)In addition, there were 16 queries with minor changes. |
…togram" This reverts commit 7ae9557.
| final double binWidth = 2.0; | ||
| final int barWidth = 30; | ||
| final double lo = Math.floor(sortedPercentDiffs.get(0) / binWidth) * binWidth; | ||
| double hi = Math.ceil(sortedPercentDiffs.get(sortedPercentDiffs.size() - 1) / binWidth) * binWidth; |
There was a problem hiding this comment.
| double hi = Math.ceil(sortedPercentDiffs.get(sortedPercentDiffs.size() - 1) / binWidth) * binWidth; | |
| final double hi = (Math.floor(sortedPercentDiffs.get(sortedPercentDiffs.size() - 1) / binWidth) + 1) * binWidth; |
That way, the if (hi <= lo) check below is not necessary.
| final var standardDeviation = Math.sqrt(variance); | ||
|
|
||
| builder.put(fieldName, new FieldStatistics(values, mean, standardDeviation)); | ||
| final List<Double> sortedPcts = percentDifferences.getOrDefault(fieldName, ImmutableList.of()); |
There was a problem hiding this comment.
Rather name it just pcts instead of sortedPcts. At this point it is not actually sorted yet!
| final double binWidth = 2.0; | ||
| final int barWidth = 30; |
There was a problem hiding this comment.
Consider turning these into actual arguments (or maybe private static constants)?
There’s a comment further down that mentions BIN_WIDTH. That would make more sense if there were an actual private static BIN_WIDTH.
|
|
||
| report.append(String.format(Locale.ROOT, "`%s` %% change distribution (%d queries, bin = 2%%):%n%n", fieldName, sortedPercentDiffs.size())); | ||
| report.append("```\n"); | ||
| report.append(String.format(Locale.ROOT, "%14s %-30s n%n", "Range", "")); |
There was a problem hiding this comment.
Is there an (easy) way to avoid hard-coding the %14s width here?
| } | ||
| } | ||
|
|
||
| private void appendHistogram(@Nonnull final StringBuilder report, |
There was a problem hiding this comment.
This looks like it can be a static method.
It’s a matter of taste, but I would consider splitting off two more submethods:
- The part that computes the
binsfromsortedPercentDiffs. - The part that formats a
Stringfrom thebins.
These could then be understood and (hypothetically) unit-tested independently.
2 participants
Appends a text histogram of the % change distribution to each field's stats block in the metrics diff report.
MetricsStatistics: collect percent diffs per field inBuilder, expose as sortedFieldStatistics.sortedPercentDiffsMetricsDiffAnalyzer: render a 2%-wide bin bar chart after each field summary; guard againstlo == hiedge case (all values identical on a bin boundary)