From de921ee4383baaa00c4a6cbc66ef88cf81126b77 Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Wed, 4 Dec 2024 13:24:53 +0100 Subject: [PATCH] feat: summarize to use errors instead of panics Signed-off-by: Andres Taylor --- go/summarize/force-graph.go | 27 ++++---- go/summarize/markdown.go | 8 ++- go/summarize/reading.go | 36 ++++++----- go/summarize/reading_test.go | 4 +- go/summarize/summarize-keys.go | 24 +++---- go/summarize/summarize-keys_test.go | 26 +++++--- go/summarize/summarize-trace_test.go | 4 +- go/summarize/summarize-transactions_test.go | 11 ++-- go/summarize/summarize.go | 69 ++++++++++++++------- go/summarize/summary.go | 21 +++++-- 10 files changed, 146 insertions(+), 84 deletions(-) diff --git a/go/summarize/force-graph.go b/go/summarize/force-graph.go index 2e130df..856d34b 100644 --- a/go/summarize/force-graph.go +++ b/go/summarize/force-graph.go @@ -18,6 +18,7 @@ package summarize import ( "encoding/json" + "errors" "fmt" "html/template" "net" @@ -132,43 +133,43 @@ func createGraphKey(tableA, tableB string) graphKey { return graphKey{Tbl1: tableB, Tbl2: tableA} } -func renderQueryGraph(s *Summary) { +func renderQueryGraph(s *Summary) error { data := createForceGraphData(s) listener, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { - panic(err) + return fmt.Errorf("could not create a listener: %w", err) } defer listener.Close() // Get the assigned port addr, ok := listener.Addr().(*net.TCPAddr) if !ok { - exit("could not create a listener") + return errors.New("could not create a listener") } fmt.Printf("Server started at http://localhost:%d\nExit the program with CTRL+C\n", addr.Port) // Start the server // nolint: gosec,nolintlint // this is all ran locally so no need to care about vulnerabilities around timeouts - err = http.Serve(listener, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - serveIndex(w, data) + return http.Serve(listener, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + err := serveIndex(w, data) + if err != nil { + fmt.Println(err.Error()) + } })) - if err != nil { - exit(err.Error()) - } } // Function to dynamically generate and serve index.html -func serveIndex(w http.ResponseWriter, data forceGraphData) { +func serveIndex(w http.ResponseWriter, data forceGraphData) error { dataBytes, err := json.Marshal(data.data) if err != nil { - exit(err.Error()) + return fmt.Errorf("could not marshal data: %w", err) } tmpl, err := template.New("index").Parse(templateHTML) if err != nil { http.Error(w, "Failed to parse template", http.StatusInternalServerError) - return + return err } d := struct { @@ -182,8 +183,10 @@ func serveIndex(w http.ResponseWriter, data forceGraphData) { if err := tmpl.Execute(w, d); err != nil { http.Error(w, "Failed to execute template", http.StatusInternalServerError) - return + return err } + + return nil } /* diff --git a/go/summarize/markdown.go b/go/summarize/markdown.go index f3efa1d..8fb802d 100644 --- a/go/summarize/markdown.go +++ b/go/summarize/markdown.go @@ -80,7 +80,13 @@ func renderHotQueries(md *markdown.MarkDown, queries []keys.QueryAnalysisResult, } } -func renderTableUsage(md *markdown.MarkDown, tableSummaries []*TableSummary, includeRowCount bool) { +func renderTableUsage(md *markdown.MarkDown, in []*TableSummary, includeRowCount bool) { + var tableSummaries []*TableSummary + for _, tbl := range in { + if !tbl.IsEmpty() { + tableSummaries = append(tableSummaries, tbl) + } + } if len(tableSummaries) == 0 { return } diff --git a/go/summarize/reading.go b/go/summarize/reading.go index 1d01e67..c5bccd1 100644 --- a/go/summarize/reading.go +++ b/go/summarize/reading.go @@ -18,6 +18,7 @@ package summarize import ( "encoding/json" + "fmt" "os" "sort" "strconv" @@ -27,10 +28,10 @@ import ( "github.com/vitessio/vt/go/transactions" ) -func readTracedFile(fileName string) traceSummary { +func readTracedFile(fileName string) (traceSummary, error) { c, err := os.ReadFile(fileName) if err != nil { - exit("Error opening file: " + err.Error()) + return traceSummary{}, fmt.Errorf("error opening file: %w", err) } type traceOutput struct { @@ -40,7 +41,7 @@ func readTracedFile(fileName string) traceSummary { var to traceOutput err = json.Unmarshal(c, &to) if err != nil { - exit("Error parsing json: " + err.Error()) + return traceSummary{}, fmt.Errorf("error parsing json: %w", err) } sort.Slice(to.Queries, func(i, j int) bool { @@ -58,13 +59,15 @@ func readTracedFile(fileName string) traceSummary { return traceSummary{ Name: fileName, TracedQueries: to.Queries, - } + }, nil } -func readTransactionFile(fileName string) func(s *Summary) error { +type summarizer = func(s *Summary) error + +func readTransactionFile(fileName string) (summarizer, error) { c, err := os.ReadFile(fileName) if err != nil { - exit("Error opening file: " + err.Error()) + return nil, fmt.Errorf("error opening file: %w", err) } type txOutput struct { @@ -75,37 +78,36 @@ func readTransactionFile(fileName string) func(s *Summary) error { var to txOutput err = json.Unmarshal(c, &to) if err != nil { - exit("Error parsing json: " + err.Error()) + return nil, fmt.Errorf("error parsing json: %w", err) } return func(s *Summary) error { s.analyzedFiles = append(s.analyzedFiles, fileName) return summarizeTransactions(s, to.Signatures) - } + }, nil } -func readKeysFile(fileName string) func(s *Summary) error { +func readKeysFile(fileName string) (summarizer, error) { c, err := os.ReadFile(fileName) if err != nil { - exit("Error opening file: " + err.Error()) + return nil, fmt.Errorf("error opening file: %w", err) } var ko keys.Output err = json.Unmarshal(c, &ko) if err != nil { - exit("Error parsing json: " + err.Error()) + return nil, fmt.Errorf("error parsing json: %w", err) } return func(s *Summary) error { s.analyzedFiles = append(s.analyzedFiles, fileName) - summarizeKeysQueries(s, &ko) - return nil - } + return summarizeKeysQueries(s, &ko) + }, nil } -func readDBInfoFile(fileName string) func(s *Summary) error { +func readDBInfoFile(fileName string) (summarizer, error) { schemaInfo, err := dbinfo.Load(fileName) if err != nil { - panic(err) + return nil, fmt.Errorf("error parsing dbinfo: %w", err) } return func(s *Summary) error { @@ -120,5 +122,5 @@ func readDBInfoFile(fileName string) func(s *Summary) error { table.RowCount = ti.Rows } return nil - } + }, nil } diff --git a/go/summarize/reading_test.go b/go/summarize/reading_test.go index edfdb19..f1c5266 100644 --- a/go/summarize/reading_test.go +++ b/go/summarize/reading_test.go @@ -20,9 +20,11 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestReadTransaction(t *testing.T) { - file := readTransactionFile("../testdata/small-slow-query-transactions.json") + file, err := readTransactionFile("../testdata/small-slow-query-transactions.json") + require.NoError(t, err) assert.NotNil(t, file) } diff --git a/go/summarize/summarize-keys.go b/go/summarize/summarize-keys.go index be6cfef..3b9fa4a 100644 --- a/go/summarize/summarize-keys.go +++ b/go/summarize/summarize-keys.go @@ -85,7 +85,7 @@ func (p Position) String() string { return "GROUP" } - panic("unknown Position") + return "UNKNOWN" } func (ci ColumnInformation) String() string { @@ -139,31 +139,30 @@ func (ts TableSummary) UseCount() int { type getMetric = func(q keys.QueryAnalysisResult) float64 -func getMetricForHotness(metric string) getMetric { +func getMetricForHotness(metric string) (getMetric, error) { switch metric { case "usage-count": return func(q keys.QueryAnalysisResult) float64 { return float64(q.UsageCount) - } + }, nil case "total-rows-examined": return func(q keys.QueryAnalysisResult) float64 { return float64(q.RowsExamined) - } + }, nil case "avg-rows-examined": return func(q keys.QueryAnalysisResult) float64 { return float64(q.RowsExamined) / float64(q.UsageCount) - } + }, nil case "total-time", "": return func(q keys.QueryAnalysisResult) float64 { return q.QueryTime - } + }, nil case "avg-time": return func(q keys.QueryAnalysisResult) float64 { return q.QueryTime / float64(q.UsageCount) - } + }, nil default: - exit(fmt.Sprintf("unknown metric: %s", metric)) - panic("unreachable") + return nil, fmt.Errorf("unknown metric: %s", metric) } } @@ -185,7 +184,7 @@ func makeKey(lhs, rhs operators.Column) graphKey { return graphKey{rhs.Table, lhs.Table} } -func summarizeKeysQueries(summary *Summary, queries *keys.Output) { +func summarizeKeysQueries(summary *Summary, queries *keys.Output) error { tableSummaries := make(map[string]*TableSummary) tableUsageWriteCounts := make(map[string]int) tableUsageReadCounts := make(map[string]int) @@ -218,11 +217,11 @@ func summarizeKeysQueries(summary *Summary, queries *keys.Output) { table.ReadQueryCount = tblSummary.ReadQueryCount table.WriteQueryCount = tblSummary.WriteQueryCount if table.ColumnUses != nil { - panic("ColumnUses already set for table" + tblSummary.Table) + return fmt.Errorf("ColumnUses already set for table %s", tblSummary.Table) } table.ColumnUses = tblSummary.ColumnUses if table.JoinPredicates != nil { - panic("JoinPredicates already set for table" + tblSummary.Table) + return fmt.Errorf("JoinPredicates already set for table %s", tblSummary.Table) } table.JoinPredicates = tblSummary.JoinPredicates } @@ -269,6 +268,7 @@ func summarizeKeysQueries(summary *Summary, queries *keys.Output) { } return summary.joins[i].Tbl2 < summary.joins[j].Tbl2 }) + return nil } func checkQueryForHotness(hotQueries *[]keys.QueryAnalysisResult, query keys.QueryAnalysisResult, metricReader getMetric) { diff --git a/go/summarize/summarize-keys_test.go b/go/summarize/summarize-keys_test.go index 731780e..4a19ee6 100644 --- a/go/summarize/summarize-keys_test.go +++ b/go/summarize/summarize-keys_test.go @@ -70,18 +70,23 @@ func TestSummarizeKeysFile(t *testing.T) { sb := &strings.Builder{} now := time.Date(2024, time.January, 1, 1, 2, 3, 0, time.UTC) - fnKeys := readKeysFile("../testdata/keys-log.json") - fnSchemaInfo := readDBInfoFile("../testdata/keys-schema-info.json") + fnKeys, err := readKeysFile("../testdata/keys-log.json") + require.NoError(t, err) + + fnSchemaInfo, err := readDBInfoFile("../testdata/keys-schema-info.json") + require.NoError(t, err) - s := NewSummary("") + s, err := NewSummary("") + require.NoError(t, err) - err := fnKeys(s) + err = fnKeys(s) require.NoError(t, err) err = fnSchemaInfo(s) require.NoError(t, err) - s.PrintMarkdown(sb, now) + err = s.PrintMarkdown(sb, now) + require.NoError(t, err) expected, err := os.ReadFile("../testdata/keys-summary.md") require.NoError(t, err) @@ -102,16 +107,19 @@ func TestSummarizeKeysWithHotnessFile(t *testing.T) { for _, metric := range tests { t.Run(metric, func(t *testing.T) { - fn := readKeysFile("../testdata/bigger_slow_query_log.json") + fn, err := readKeysFile("../testdata/bigger_slow_query_log.json") + require.NoError(t, err) sb := &strings.Builder{} now := time.Date(2024, time.January, 1, 1, 2, 3, 0, time.UTC) - s := NewSummary(metric) + s, err := NewSummary(metric) + require.NoError(t, err) - err := fn(s) + err = fn(s) require.NoError(t, err) - s.PrintMarkdown(sb, now) + err = s.PrintMarkdown(sb, now) + require.NoError(t, err) expected, err := os.ReadFile(fmt.Sprintf("../testdata/bigger_slow_log_%s.md", metric)) require.NoError(t, err) diff --git a/go/summarize/summarize-trace_test.go b/go/summarize/summarize-trace_test.go index 7502db6..533f2a6 100644 --- a/go/summarize/summarize-trace_test.go +++ b/go/summarize/summarize-trace_test.go @@ -21,6 +21,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func tf1() traceSummary { @@ -183,7 +184,8 @@ Summary: } func TestSummarizeTraceFile(t *testing.T) { - tq := readTracedFile("../testdata/trace-log.json") + tq, err := readTracedFile("../testdata/trace-log.json") + require.NoError(t, err) sb := &strings.Builder{} printTraceSummary(sb, 80, noHighlight, tq) expected := `Query: INSERT INTO region (R_REGIONKEY, R_NAME, R_COMMENT) VALUES (1, 'ASIA',... diff --git a/go/summarize/summarize-transactions_test.go b/go/summarize/summarize-transactions_test.go index 4d57ff5..82b694e 100644 --- a/go/summarize/summarize-transactions_test.go +++ b/go/summarize/summarize-transactions_test.go @@ -30,14 +30,17 @@ func TestSummarizeTransactionsFile(t *testing.T) { sb := &strings.Builder{} now := time.Date(2024, time.January, 1, 1, 2, 3, 0, time.UTC) - fnTx := readTransactionFile("../testdata/small-slow-query-transactions.json") + fnTx, err := readTransactionFile("../testdata/small-slow-query-transactions.json") + require.NoError(t, err) - s := NewSummary("") + s, err := NewSummary("") + require.NoError(t, err) - err := fnTx(s) + err = fnTx(s) require.NoError(t, err) - s.PrintMarkdown(sb, now) + err = s.PrintMarkdown(sb, now) + require.NoError(t, err) expected, err := os.ReadFile("../testdata/transactions-summary.md") require.NoError(t, err) diff --git a/go/summarize/summarize.go b/go/summarize/summarize.go index be31ff6..520853d 100644 --- a/go/summarize/summarize.go +++ b/go/summarize/summarize.go @@ -17,6 +17,7 @@ limitations under the License. package summarize import ( + "errors" "fmt" "io" "os" @@ -45,30 +46,52 @@ func Run(files []string, hotMetric string, showGraph bool) { if err != nil { panic(err.Error()) } + var w summarizer + var t traceSummary switch typ { case dbInfoFile: - workers = append(workers, readDBInfoFile(file)) + w, err = readDBInfoFile(file) case transactionFile: - workers = append(workers, readTransactionFile(file)) + w, err = readTransactionFile(file) case traceFile: - traces = append(traces, readTracedFile(file)) + t, err = readTracedFile(file) case keysFile: - workers = append(workers, readKeysFile(file)) + w, err = readKeysFile(file) default: - panic("Unknown file type") + err = errors.New("unknown file type") } + + if err != nil { + panic(err.Error()) + } + + if w != nil { + workers = append(workers, w) + continue + } + + traces = append(traces, t) } traceCount := len(traces) if traceCount <= 0 { - s := printSummary(hotMetric, workers) + s, err := printSummary(hotMetric, workers) + if err != nil { + panic(err.Error()) + } if showGraph { - renderQueryGraph(s) + err := renderQueryGraph(s) + if err != nil { + panic(err.Error()) + } } return } - checkTraceConditions(traces, workers, hotMetric) + err := checkTraceConditions(traces, workers, hotMetric) + if err != nil { + panic(err.Error()) + } switch traceCount { case 1: printTraceSummary(os.Stdout, terminalWidth(), highlightQuery, traces[0]) @@ -77,33 +100,35 @@ func Run(files []string, hotMetric string, showGraph bool) { } } -func printSummary(hotMetric string, workers []summaryWorker) *Summary { - s := NewSummary(hotMetric) +func printSummary(hotMetric string, workers []summaryWorker) (*Summary, error) { + s, err := NewSummary(hotMetric) + if err != nil { + return nil, err + } for _, worker := range workers { err := worker(s) if err != nil { - exit(err.Error()) + return nil, err } } - s.PrintMarkdown(os.Stdout, time.Now()) - return s + err = s.PrintMarkdown(os.Stdout, time.Now()) + if err != nil { + return nil, err + } + return s, nil } -func checkTraceConditions(traces []traceSummary, workers []summaryWorker, hotMetric string) { +func checkTraceConditions(traces []traceSummary, workers []summaryWorker, hotMetric string) error { if len(workers) > 0 { - panic("Trace files cannot be mixed with other file types") + return errors.New("trace files cannot be mixed with other file types") } if len(traces) > 2 { - panic("Can only summarize up to two trace files at once") + return errors.New("can only summarize up to two trace files at once") } if hotMetric != "" { - exit("hotMetric flag is only supported for 'vt keys' output") + return errors.New("hotMetric flag is only supported for 'vt keys' output") } -} - -func exit(msg string) { - fmt.Println(msg) - os.Exit(1) + return nil } const queryPrefix = "Query: " diff --git a/go/summarize/summary.go b/go/summarize/summary.go index 1d320e0..4108ac7 100644 --- a/go/summarize/summary.go +++ b/go/summarize/summary.go @@ -17,6 +17,7 @@ limitations under the License. package summarize import ( + "fmt" "io" "strings" "time" @@ -67,14 +68,19 @@ type ( } ) -func NewSummary(hotMetric string) *Summary { +func NewSummary(hotMetric string) (*Summary, error) { + hotness, err := getMetricForHotness(hotMetric) + if err != nil { + return nil, err + } + return &Summary{ queryGraph: make(queryGraph), - hotQueryFn: getMetricForHotness(hotMetric), - } + hotQueryFn: hotness, + }, nil } -func (s *Summary) PrintMarkdown(out io.Writer, now time.Time) { +func (s *Summary) PrintMarkdown(out io.Writer, now time.Time) error { md := &markdown.MarkDown{} filePlural := "" msg := `# Query Analysis Report @@ -98,8 +104,9 @@ func (s *Summary) PrintMarkdown(out io.Writer, now time.Time) { _, err := md.WriteTo(out) if err != nil { - panic(err) + return fmt.Errorf("error writing markdown: %w", err) } + return nil } func (s *Summary) GetTable(name string) *TableSummary { @@ -114,3 +121,7 @@ func (s *Summary) GetTable(name string) *TableSummary { func (s *Summary) AddTable(table *TableSummary) { s.tables = append(s.tables, table) } + +func (ts TableSummary) IsEmpty() bool { + return ts.ReadQueryCount == 0 && ts.WriteQueryCount == 0 && len(ts.ColumnUses) == 0 && ts.RowCount == 0 +}