From 1e0ea20cd56083540da916dbc7b909f3706d08e0 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 10 Jun 2026 18:11:42 +0100 Subject: [PATCH] fix(reporting): make report PDFs read like client documents, not generated artifacts A design-partner review of real output called the PDFs out as thin, and the page renders agreed: fleet reports spent one nearly blank A4 page per resource, metric cards showed raw store keys with unformatted values ('diskread 880000.00'), and value cells overlapped the adjacent column for long readings. - Fleet per-resource blocks now flow several to a page with separators, breaking only when the next block will not fit (a 6-resource report drops from 8 pages to 3); each block gains an availability line - Rate metrics get display names and humane units (Disk Read 859.38 KiB/s instead of diskread 880000.00); byte units are self-describing so the old +unit suffix no longer renders '12.00 GiBbytes' - Metric card stat columns use bounded cells so long values cannot run under the Avg/Samples labels - Single reports merge Resource Details and Performance Summary onto a shared page when they fit instead of stranding a third of a page of content on each of two pages Verified by regenerating branded fleet and single reports and reviewing every rendered page. --- pkg/reporting/csv.go | 17 +++- pkg/reporting/engine.go | 16 ++++ pkg/reporting/pdf.go | 155 +++++++++++++++++++++++++---------- pkg/reporting/pdf_ux_test.go | 73 +++++++++++++++++ 4 files changed, 217 insertions(+), 44 deletions(-) diff --git a/pkg/reporting/csv.go b/pkg/reporting/csv.go index f38cf5616..82f4405d1 100644 --- a/pkg/reporting/csv.go +++ b/pkg/reporting/csv.go @@ -366,12 +366,27 @@ func (g *CSVGenerator) GenerateMulti(data *MultiReportData) ([]byte, error) { // formatValue formats a metric value with appropriate precision. func formatValue(value float64, unit string) string { - if unit == "bytes" { + switch unit { + case "bytes": return formatBytes(value) + case "bytes/s": + return formatBytes(value) + "/s" } return fmt.Sprintf("%.2f", value) } +// formatMetricValue renders a value with its unit for display. Byte-based +// units are self-describing ("1.50 GiB", "880 KiB/s"); symbolic units are +// appended ("22.10%"); unitless values render bare. +func formatMetricValue(value float64, unit string) string { + switch unit { + case "bytes", "bytes/s", "": + return formatValue(value, unit) + default: + return formatValue(value, unit) + unit + } +} + // formatBytes converts bytes to human-readable format. func formatBytes(bytes float64) string { const unit = 1024 diff --git a/pkg/reporting/engine.go b/pkg/reporting/engine.go index 488c05d71..69174cb6a 100644 --- a/pkg/reporting/engine.go +++ b/pkg/reporting/engine.go @@ -707,6 +707,18 @@ func GetMetricTypeDisplayName(metricType string) string { return "Total Space" case "avail": return "Available Space" + case "diskread": + return "Disk Read" + case "diskwrite": + return "Disk Write" + case "netin": + return "Network In" + case "netout": + return "Network Out" + case "iops": + return "Disk IOPS" + case "temperature": + return "Temperature" default: return metricType } @@ -719,6 +731,10 @@ func GetMetricUnit(metricType string) string { return "%" case "used", "total", "avail": return "bytes" + case "diskread", "diskwrite", "netin", "netout": + return "bytes/s" + case "temperature": + return "C" default: return "" } diff --git a/pkg/reporting/pdf.go b/pkg/reporting/pdf.go index 96c3399db..e1960dc34 100644 --- a/pkg/reporting/pdf.go +++ b/pkg/reporting/pdf.go @@ -172,10 +172,21 @@ func (g *PDFGenerator) Generate(data *ReportData) ([]byte, error) { g.addPageHeader(pdf, data, "Executive Summary") g.writeExecutiveSummary(pdf, data) - // Resource details page (if enrichment data available) + // Resource details (if enrichment data available). The details grid is + // usually a third of a page; giving it a page of its own left the + // report mostly whitespace, so it shares the executive summary page + // whenever there is room. if data.Resource != nil { - pdf.AddPage() - g.addPageHeader(pdf, data, "Resource Details") + if pdf.GetY() > 160 { + pdf.AddPage() + g.addPageHeader(pdf, data, "Resource Details") + } else { + pdf.Ln(4) + pdf.SetFont("Arial", "B", 14) + pdf.SetTextColor(colorTextDark[0], colorTextDark[1], colorTextDark[2]) + pdf.CellFormat(0, 9, "Resource Details", "", 1, "L", false, 0, "") + pdf.Ln(2) + } g.writeResourceDetails(pdf, data) // Storage section for nodes @@ -203,9 +214,19 @@ func (g *PDFGenerator) Generate(data *ReportData) ([]byte, error) { g.writeAlertsSection(pdf, data) } - // Summary page with metrics - pdf.AddPage() - g.addPageHeader(pdf, data, "Performance Summary") + // Summary with metrics. The details grid above is usually short, so + // the performance summary continues on the same page when it fits + // instead of stranding the details on a nearly blank page. + if pdf.GetY() > 110 { + pdf.AddPage() + g.addPageHeader(pdf, data, "Performance Summary") + } else { + pdf.Ln(6) + pdf.SetFont("Arial", "B", 14) + pdf.SetTextColor(colorTextDark[0], colorTextDark[1], colorTextDark[2]) + pdf.CellFormat(0, 9, "Performance Summary", "", 1, "L", false, 0, "") + pdf.Ln(2) + } g.writeSummarySection(pdf, data) // Charts page(s) @@ -900,46 +921,29 @@ func (g *PDFGenerator) writeSummarySection(pdf *fpdf.Fpdf, data *ReportData) { pdf.SetXY(cardX+5, cardY+14) pdf.SetFont("Arial", "B", 20) pdf.SetTextColor(headerColor[0], headerColor[1], headerColor[2]) - pdf.CellFormat(cardWidth-10, 10, formatValue(stats.Current, unit)+unit, "", 1, "L", false, 0, "") + pdf.CellFormat(cardWidth-10, 10, formatMetricValue(stats.Current, unit), "", 1, "L", false, 0, "") // Stats row pdf.SetFont("Arial", "", 8) pdf.SetTextColor(colorTextMuted[0], colorTextMuted[1], colorTextMuted[2]) statsY := cardY + 28 - // Min - pdf.SetXY(cardX+5, statsY) - pdf.CellFormat(25, 5, "Min", "", 0, "L", false, 0, "") - pdf.SetFont("Arial", "B", 8) - pdf.SetTextColor(colorTextDark[0], colorTextDark[1], colorTextDark[2]) - pdf.CellFormat(0, 5, formatValue(stats.Min, unit)+unit, "", 1, "L", false, 0, "") - - // Max - pdf.SetFont("Arial", "", 8) - pdf.SetTextColor(colorTextMuted[0], colorTextMuted[1], colorTextMuted[2]) - pdf.SetXY(cardX+5, statsY+6) - pdf.CellFormat(25, 5, "Max", "", 0, "L", false, 0, "") - pdf.SetFont("Arial", "B", 8) - pdf.SetTextColor(colorTextDark[0], colorTextDark[1], colorTextDark[2]) - pdf.CellFormat(0, 5, formatValue(stats.Max, unit)+unit, "", 1, "L", false, 0, "") - - // Avg - pdf.SetFont("Arial", "", 8) - pdf.SetTextColor(colorTextMuted[0], colorTextMuted[1], colorTextMuted[2]) - pdf.SetXY(cardX+45, statsY) - pdf.CellFormat(15, 5, "Avg", "", 0, "L", false, 0, "") - pdf.SetFont("Arial", "B", 8) - pdf.SetTextColor(colorTextDark[0], colorTextDark[1], colorTextDark[2]) - pdf.CellFormat(0, 5, formatValue(stats.Avg, unit)+unit, "", 1, "L", false, 0, "") - - // Count - pdf.SetFont("Arial", "", 8) - pdf.SetTextColor(colorTextMuted[0], colorTextMuted[1], colorTextMuted[2]) - pdf.SetXY(cardX+45, statsY+6) - pdf.CellFormat(15, 5, "Samples", "", 0, "L", false, 0, "") - pdf.SetFont("Arial", "B", 8) - pdf.SetTextColor(colorTextDark[0], colorTextDark[1], colorTextDark[2]) - pdf.CellFormat(0, 5, fmt.Sprintf("%d", stats.Count), "", 1, "L", false, 0, "") + // Two bounded label/value column pairs. Value cells must not use + // width 0 (extend to margin): long byte-rate values would run + // under the second column's labels. + writeStat := func(x, y float64, label, value string) { + pdf.SetFont("Arial", "", 8) + pdf.SetTextColor(colorTextMuted[0], colorTextMuted[1], colorTextMuted[2]) + pdf.SetXY(x, y) + pdf.CellFormat(13, 5, label, "", 0, "L", false, 0, "") + pdf.SetFont("Arial", "B", 8) + pdf.SetTextColor(colorTextDark[0], colorTextDark[1], colorTextDark[2]) + pdf.CellFormat(26, 5, value, "", 0, "L", false, 0, "") + } + writeStat(cardX+5, statsY, "Min", formatMetricValue(stats.Min, unit)) + writeStat(cardX+5, statsY+6, "Max", formatMetricValue(stats.Max, unit)) + writeStat(cardX+46, statsY, "Avg", formatMetricValue(stats.Avg, unit)) + writeStat(cardX+46, statsY+6, "Samples", fmt.Sprintf("%d", stats.Count)) } // Calculate final Y position based on number of rows @@ -1784,10 +1788,24 @@ func (g *PDFGenerator) GenerateMulti(data *MultiReportData) ([]byte, error) { g.addMultiPageHeader(pdf, data, "Fleet Summary") g.writeFleetSummary(pdf, data) - // Pages 3+: Condensed per-resource pages + // Pages 3+: condensed per-resource blocks, flowed several to a page. + // One near-empty A4 page per resource read as padding in client-facing + // reports, so a new page starts only when the next block will not fit. + _, pageHeight := pdf.GetPageSize() + bottomLimit := pageHeight - 30 + pageOpen := false for _, rd := range data.Resources { - pdf.AddPage() - g.addMultiPageHeader(pdf, data, "Resource Detail") + startNewPage := !pageOpen || pdf.GetY()+condensedResourceBlockHeight(rd) > bottomLimit + if startNewPage { + pdf.AddPage() + g.addMultiPageHeader(pdf, data, "Resource Details") + pageOpen = true + } else { + pdf.Ln(3) + pdf.SetDrawColor(colorGridLine[0], colorGridLine[1], colorGridLine[2]) + pdf.Line(20, pdf.GetY(), 190, pdf.GetY()) + pdf.Ln(5) + } g.writeCondensedResourcePage(pdf, rd) } @@ -2329,6 +2347,37 @@ func writeFleetNarrativeSection(pdf *fpdf.Fpdf, fn *FleetNarrative) { } // writeCondensedResourcePage writes a condensed single-page view for one resource. +// condensedResourceBlockHeight estimates the vertical space a condensed +// resource block needs so the fleet layout can decide whether it fits on +// the current page. Estimates err slightly high; a block that still +// overflows is carried across pages by fpdf's auto page break. +func condensedResourceBlockHeight(rd *ReportData) float64 { + height := 11.0 + 6.0 + 27.0 + 8.0 // name header + availability line + stats bar + padding + if len(rd.Metrics["cpu"]) >= 2 || len(rd.Metrics["memory"]) >= 2 { + height += 58 // chart title + canvas + legend + } + active := 0 + for _, alert := range rd.Alerts { + if alert.ResolvedTime == nil { + active++ + } + } + if active > 0 { + shown := active + if shown > 3 { + shown = 4 // three alerts plus the "and N more" line + } + height += 10 + float64(shown)*5 + } + if len(rd.Storage) > 0 { + height += 10 + float64(len(rd.Storage))*5 + } + if len(rd.Backups) > 0 { + height += 20 + } + return height +} + func (g *PDFGenerator) writeCondensedResourcePage(pdf *fpdf.Fpdf, rd *ReportData) { // Resource header resourceName := rd.ResourceID @@ -2364,6 +2413,26 @@ func (g *PDFGenerator) writeCondensedResourcePage(pdf *fpdf.Fpdf, rd *ReportData statusStr += fmt.Sprintf(" | Uptime: %s", formatUptime(rd.Resource.Uptime)) } pdf.CellFormat(0, 8, statusStr, "", 1, "L", false, 0, "") + + // Availability over the window, when the state timeline produced one. + if rd.Availability != nil { + line := "Availability: not observed this period" + lineColor := colorTextMuted + if rd.Availability.Observed() { + line = fmt.Sprintf("Availability: %s", availabilityUptimeLabel(rd.Availability.UptimePercent)) + if rd.Availability.DownIncidents > 0 { + outageWord := "outages" + if rd.Availability.DownIncidents == 1 { + outageWord = "outage" + } + line += fmt.Sprintf(" (%d %s, %s down)", rd.Availability.DownIncidents, outageWord, formatOutageDuration(rd.Availability.TotalDowntime)) + } + lineColor = colorTextDark + } + pdf.SetFont("Arial", "", 9) + pdf.SetTextColor(lineColor[0], lineColor[1], lineColor[2]) + pdf.CellFormat(0, 5, line, "", 1, "L", false, 0, "") + } pdf.Ln(3) // Stats bar - CPU, Memory, Disk averages and maxes diff --git a/pkg/reporting/pdf_ux_test.go b/pkg/reporting/pdf_ux_test.go index c931e9542..fda4e4844 100644 --- a/pkg/reporting/pdf_ux_test.go +++ b/pkg/reporting/pdf_ux_test.go @@ -3,8 +3,10 @@ package reporting import ( "bytes" "compress/zlib" + "fmt" "io" "regexp" + "strconv" "strings" "testing" "time" @@ -394,3 +396,74 @@ func TestAvailabilityUptimeLabel_NeverOverstates(t *testing.T) { t.Fatalf("availabilityUptimeLabel(99.4249) = %q, want 99.42%%", got) } } + +// TestMetricFormatting_RateMetricsAreHumanReadable pins the display +// vocabulary for the rate metrics that previously rendered as raw keys +// with unformatted values ("diskread 880000.00") in client-facing +// reports. +func TestMetricFormatting_RateMetricsAreHumanReadable(t *testing.T) { + if got := GetMetricTypeDisplayName("diskread"); got != "Disk Read" { + t.Fatalf("GetMetricTypeDisplayName(diskread) = %q", got) + } + if got := GetMetricTypeDisplayName("netout"); got != "Network Out" { + t.Fatalf("GetMetricTypeDisplayName(netout) = %q", got) + } + if got := GetMetricUnit("diskwrite"); got != "bytes/s" { + t.Fatalf("GetMetricUnit(diskwrite) = %q", got) + } + if got := formatMetricValue(880000, "bytes/s"); got != "859.38 KiB/s" { + t.Fatalf("formatMetricValue(880000, bytes/s) = %q", got) + } + if got := formatMetricValue(22.1, "%"); got != "22.10%" { + t.Fatalf("formatMetricValue(22.1, %%) = %q", got) + } + // Byte units are self-describing; the old +unit concatenation + // produced "12.00 GiBbytes". + if got := formatMetricValue(12884901888, "bytes"); got != "12.00 GiB" { + t.Fatalf("formatMetricValue(12GiB, bytes) = %q", got) + } +} + +// TestGenerateMulti_FlowsResourceBlocksOntoSharedPages asserts the fleet +// report no longer spends one near-empty A4 page per resource: six sparse +// resources must fit on a handful of pages, not eight. +func TestGenerateMulti_FlowsResourceBlocksOntoSharedPages(t *testing.T) { + now := time.Now() + multi := &MultiReportData{ + Title: "Fleet", + Start: now.Add(-24 * time.Hour), + End: now, + GeneratedAt: now, + } + for i := 0; i < 6; i++ { + multi.Resources = append(multi.Resources, &ReportData{ + ResourceID: fmt.Sprintf("vm-%d", i), + ResourceType: "vm", + Resource: &ResourceInfo{Name: fmt.Sprintf("guest-%02d", i), Status: "online"}, + Summary: MetricSummary{ByMetric: map[string]MetricStats{"cpu": {Avg: 10, Max: 20, Count: 1}}}, + TotalPoints: 1, + Availability: &AvailabilityInfo{UptimePercent: 100, ObservedPercent: 100}, + }) + } + + gen := NewPDFGenerator() + out, err := gen.GenerateMulti(multi) + if err != nil { + t.Fatalf("GenerateMulti: %v", err) + } + text := extractPDFText(t, out) + + pageTotal := regexp.MustCompile(`Page \d+ of (\d+)`).FindStringSubmatch(text) + if pageTotal == nil { + t.Fatalf("no page footer found in:\n%s", text) + } + if pages, _ := strconv.Atoi(pageTotal[1]); pages > 4 { + t.Fatalf("6 sparse resources rendered %d pages; blocks must flow onto shared pages", pages) + } + for i := 0; i < 6; i++ { + name := fmt.Sprintf("guest-%02d", i) + if !strings.Contains(text, name) { + t.Fatalf("resource %s missing from flowed detail pages:\n%s", name, text) + } + } +}