From d53224c917f6031d0c860516486c390abbe5c157 Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Fri, 16 Sep 2016 14:06:12 +0200 Subject: [PATCH 1/4] Rephrase log messages - Do not leak the programming abstraction "task" to the user. - Only print run time if greater than zero. In Go 1.6, a zero duration prints as "0", which makes for a strange sentence, and not generally useful. --- main.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/main.go b/main.go index 0e50988..fad40f7 100644 --- a/main.go +++ b/main.go @@ -158,14 +158,18 @@ func main() { runner := NewDumpRunner(basePath) - log.Print("Running dump and analyse tasks...") + log.Print("Collecting system information...") RunAllDumpTasks(runner, basePath, *concurrentTasks, fileOnlyLogger) + + log.Print("Analyzing data...") analysisResults := RunAllAnalysisTasks(runner, basePath, *concurrentTasks) delta := time.Since(start) // Remove sub-second precision. delta -= delta % time.Second - log.Printf("Run all tasks in %v.", delta) + if delta > time.Second { + log.Printf("Finished in %v", delta) + } RunOutputTask(os.Stdout, os.Stderr, analysisResults) } From 051ac4327d1182eb9bba405a6d16d21c8aaa12dc Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Fri, 16 Sep 2016 14:10:20 +0200 Subject: [PATCH 2/4] Print data collection errors at the end Easier to read than when interleaved with dots that denote progress. --- main.go | 12 +++++++++++- tasks.go | 26 ++++++++++++-------------- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/main.go b/main.go index fad40f7..909c937 100644 --- a/main.go +++ b/main.go @@ -159,7 +159,17 @@ func main() { runner := NewDumpRunner(basePath) log.Print("Collecting system information...") - RunAllDumpTasks(runner, basePath, *concurrentTasks, fileOnlyLogger) + errs := RunAllDumpTasks(runner, basePath, *concurrentTasks, os.Stderr) + + for _, err := range errs { + if ierr, ok := err.(IgnorableError); ok && ierr.Ignore() { + fileOnlyLogger.Printf("Task error: %v", err) + continue + } + // TODO: there should be a way to identify which task + // had an error. + log.Printf("Task error: %v", err) + } log.Print("Analyzing data...") analysisResults := RunAllAnalysisTasks(runner, basePath, *concurrentTasks) diff --git a/tasks.go b/tasks.go index 06c8d4e..bb73e8d 100644 --- a/tasks.go +++ b/tasks.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "io/ioutil" "log" "os" @@ -19,9 +20,12 @@ type Logger interface { Printf(format string, v ...interface{}) } -// RunAllDumpTasks runs all tasks known to the dump tool using concurrent workers. -// Dump output goes to path. -func RunAllDumpTasks(runner Runner, path string, workers int, fileOnlyLogger Logger) { +// RunAllDumpTasks runs all tasks known to the dump tool using concurrent +// workers and returns task errors. Dump output goes to path. Progress is +// communicated by writing to out. +func RunAllDumpTasks(runner Runner, path string, workers int, out io.Writer) []error { + var errs []error + tasks := GetAllDumpTasks(runner, path) results := make(chan error) @@ -46,19 +50,13 @@ func RunAllDumpTasks(runner Runner, path string, workers int, fileOnlyLogger Log // Loop through the task execution results and log errors. for err := range results { if err != nil { - if ierr, ok := err.(IgnorableError); ok && ierr.Ignore() { - fileOnlyLogger.Printf("Task error: %v", err) - continue - } - // TODO: there should be a way to identify which task - // had an error. - fmt.Fprintln(os.Stderr) - log.Printf("Task error: %v", err) - continue + errs = append(errs, err) } - fmt.Fprint(os.Stderr, ".") + fmt.Fprint(out, ".") } - fmt.Fprintln(os.Stderr) + fmt.Fprintln(out) + + return errs } // GetAllDumpTasks returns a channel of all tasks known to the dump tool. It returns From 146b6e43ddd3b97d9ca740be8f7d7a78de3a4f3e Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Fri, 16 Sep 2016 14:11:19 +0200 Subject: [PATCH 3/4] Add developer-only toggle to print all errors When developing, we often want to see errors that we don't want in the final output to a regular user. Even though all errors are always logged to dump.log in the generated archive, it is more convenient to see them straigh away in stderr, without needing to go open the dump archive. --- main.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/main.go b/main.go index 909c937..1c5ee7d 100644 --- a/main.go +++ b/main.go @@ -35,6 +35,11 @@ var ( printVersion = flag.Bool("version", false, "print version and exit") ) +// showAllErrors enables printing of ignorable errors, suitable for debugging. +// This is intentionally not exposed as a flag, and shall stay intentionally +// undocumented, used for development only. +var _, showAllErrors = os.LookupEnv("FH_SYSTEM_DUMP_TOOL_DEBUG") + // GetProjects returns a list of project names visible by the current logged in // user. func GetProjects(runner Runner) ([]string, error) { @@ -162,7 +167,7 @@ func main() { errs := RunAllDumpTasks(runner, basePath, *concurrentTasks, os.Stderr) for _, err := range errs { - if ierr, ok := err.(IgnorableError); ok && ierr.Ignore() { + if ierr, ok := err.(IgnorableError); !showAllErrors && ok && ierr.Ignore() { fileOnlyLogger.Printf("Task error: %v", err) continue } From 657726dd710e3a1c017a416a13e273e080606553 Mon Sep 17 00:00:00 2001 From: Rodolfo Carvalho Date: Fri, 16 Sep 2016 14:59:11 +0200 Subject: [PATCH 4/4] Refactor printing analysis summary - Print a message in case no issues were found - Test using Go values, no decoding from JSON --- analysis.go | 27 +++++++++++ analysis_test.go | 101 +++++++++++++++++++++++++++++++++++++++++ main.go | 2 +- output_results.go | 27 ----------- output_results_test.go | 101 ----------------------------------------- 5 files changed, 129 insertions(+), 129 deletions(-) delete mode 100644 output_results.go delete mode 100644 output_results_test.go diff --git a/analysis.go b/analysis.go index 12dd868..26c1d26 100644 --- a/analysis.go +++ b/analysis.go @@ -3,8 +3,10 @@ package main import ( "encoding/json" "fmt" + "io" "io/ioutil" "path/filepath" + "strings" "github.com/feedhenry/fh-system-dump-tool/openshift/api/types" ) @@ -177,3 +179,28 @@ func CheckDeploymentConfigs(deploymentConfigs types.DeploymentConfigList) CheckR } return result } + +// PrintAnalysisReport writes a summary of problems found in analysisResult to +// out. +func PrintAnalysisReport(analysisResult AnalysisResult, out io.Writer) { + ok := true + // TODO: handle analysisResult.Platform. + for _, projectResult := range analysisResult.Projects { + for _, checkResult := range projectResult.Results { + if !checkResult.Ok { + ok = false + fmt.Fprintf(out, "Potential issue in project %s: %s\n", projectResult.Project, checkResult.CheckName) + fmt.Fprintf(out, " Details:\n") + for _, info := range checkResult.Info { + fmt.Fprintf(out, " %s\n", strings.Replace(strings.TrimSpace(info.Message), "\n", "\n ", -1)) + } + for _, event := range checkResult.Events { + fmt.Fprintf(out, " %s\n", strings.Replace(strings.TrimSpace(event.Message), "\n", "\n ", -1)) + } + } + } + } + if ok { + fmt.Fprintln(out, "No issues found") + } +} diff --git a/analysis_test.go b/analysis_test.go index 05ced9c..08ec694 100644 --- a/analysis_test.go +++ b/analysis_test.go @@ -1,9 +1,11 @@ package main import ( + "bytes" "encoding/json" "fmt" "reflect" + "strings" "testing" "github.com/feedhenry/fh-system-dump-tool/openshift/api/types" @@ -304,3 +306,102 @@ func (l fakeDefinitionLoader) Err() error { } var _ DefinitionLoader = (*fakeDefinitionLoader)(nil) + +func TestPrintAnalysisReport(t *testing.T) { + tests := []struct { + description string + analysisResult AnalysisResult + // set want for exact matches or contains/notContains for + // inexact matches. + want string + contains []string + notContains []string + }{ + { + description: "empty analysis", + analysisResult: AnalysisResult{}, + want: "No issues found\n", + }, + { + description: "no errors", + analysisResult: AnalysisResult{ + Projects: []ProjectResult{ + { + Project: "dev", + Results: []CheckResult{ + { + CheckName: "check event log for errors", + Ok: true, + Message: "this issue was not detected", + }, + { + CheckName: "check number of replicas in deployment configs", + Ok: true, + Message: "this issue was not detected", + }, + { + CheckName: "check pods for containers in waiting state", + Ok: true, + Message: "this issue was not detected", + }, + }, + }, + }, + }, + want: "No issues found\n", + }, + { + description: "errors found", + analysisResult: AnalysisResult{ + Projects: []ProjectResult{ + { + Project: "rhmap-core", + Results: []CheckResult{ + { + CheckName: "check event log for errors", + Ok: false, + Message: "errors detected in event log", + Events: []types.Event{ + { + TypeMeta: types.TypeMeta{Kind: "Event"}, + InvolvedObject: types.ObjectReference{ + Namespace: "rhmap-core", + Name: "fh-ngui", + }, + Reason: "FailedUpdate", + Message: "Cannot update deployment rhmap-core/fh-ngui-3 status to Pending: replicationcontrollers \"fh-ngui-3\" cannot be updated: the object has been modified; please apply your changes to the latest version and try again", + Count: 1, + Type: "Warning", + }, + }, + }, + }, + }, + }, + }, + contains: []string{"rhmap-core", "fh-ngui", "Cannot update deployment"}, + notContains: []string{"No issues found"}, + }, + } + for _, tt := range tests { + var out bytes.Buffer + PrintAnalysisReport(tt.analysisResult, &out) + got := out.String() + if len(tt.contains) > 0 || len(tt.notContains) > 0 { + for _, want := range tt.contains { + if !strings.Contains(got, want) { + t.Errorf("%s: got %q, want to contain %q", tt.description, got, want) + } + } + for _, notWant := range tt.notContains { + if strings.Contains(got, notWant) { + t.Errorf("%s: got %q, want not to contain %q", tt.description, got, notWant) + } + } + } else { + if got != tt.want { + t.Errorf("%s: got %q, want %q", tt.description, got, tt.want) + } + } + } +} diff --git a/main.go b/main.go index 1c5ee7d..d4531ba 100644 --- a/main.go +++ b/main.go @@ -186,5 +186,5 @@ func main() { log.Printf("Finished in %v", delta) } - RunOutputTask(os.Stdout, os.Stderr, analysisResults) + PrintAnalysisReport(analysisResults, os.Stdout) } diff --git a/output_results.go b/output_results.go deleted file mode 100644 index 38e6c02..0000000 --- a/output_results.go +++ /dev/null @@ -1,27 +0,0 @@ -package main - -import ( - "fmt" - "io" - "strings" -) - -// RunOutputTask will read in the analysis.json file and output -// any useful information to o or any errors to e. -func RunOutputTask(o io.Writer, e io.Writer, analysisResult AnalysisResult) { - // TODO: handle analysisResult.Platform. - for _, projectResult := range analysisResult.Projects { - for _, checkResult := range projectResult.Results { - if !checkResult.Ok { - fmt.Fprintf(o, "Potential issue discovered in project %s: %s\n", projectResult.Project, checkResult.CheckName) - fmt.Fprintf(o, " Details:\n") - for _, info := range checkResult.Info { - fmt.Fprintf(o, " %s\n", strings.Replace(strings.TrimSpace(info.Message), "\n", "\n ", -1)) - } - for _, event := range checkResult.Events { - fmt.Fprintf(o, " %s\n", strings.Replace(strings.TrimSpace(event.Message), "\n", "\n ", -1)) - } - } - } - } -} diff --git a/output_results_test.go b/output_results_test.go deleted file mode 100644 index 2560b6a..0000000 --- a/output_results_test.go +++ /dev/null @@ -1,101 +0,0 @@ -package main - -import ( - "bytes" - "encoding/json" - "strings" - "testing" -) - -func mockAnalysisNoErrors(p string, d interface{}) error { - contents := `{ - "projects": [{ - "project": "dev", - "checks": [{ - "checkName": "check event log for errors", - "ok": true, - "message": "this issue was not detected" - }, { - "checkName": "check number of replicas in deployment configs", - "ok": true, - "message": "this issue was not detected" - }, { - "checkName": "check pods for containers in waiting state", - "ok": true, - "message": "this issue was not detected" - }] - }] - }` - - decoder := json.NewDecoder(bytes.NewBuffer([]byte(contents))) - err := decoder.Decode(&d) - if err != nil { - return err - } - return nil -} - -func mockAnalysisErrors(p string, d interface{}) error { - contents := `{ - "projects": [{ - "project": "rhmap-core", - "checks": [{ - "checkName": "check event log for errors", - "ok": false, - "message": "Errors detected in event log", - "events": [{ - "kind": "Event", - "involvedObject": { - "namespace": "rhmap-core", - "name": "fh-ngui" - }, - "reason": "FailedUpdate", - "message": "Cannot update deployment rhmap-core/fh-ngui-3 status to Pending: replicationcontrollers \"fh-ngui-3\" cannot be updated: the object has been modified; please apply your changes to the latest version and try again", - "count": 1, - "type": "Warning" - }] - }] - }] - }` - - decoder := json.NewDecoder(bytes.NewBuffer([]byte(contents))) - err := decoder.Decode(&d) - if err != nil { - return err - } - return nil -} - -func TestRunOutputTaskNoErrors(t *testing.T) { - o := bytes.NewBuffer([]byte{}) - e := bytes.NewBuffer([]byte{}) - var analysisResult AnalysisResult - if err := mockAnalysisNoErrors("", &analysisResult); err != nil { - t.Fatal(err) - } - RunOutputTask(o, e, analysisResult) - - if got := len(o.Bytes()); got > 0 { - t.Fatal("len(o.Bytes()), expected: 0, got:", got) - } - if got := len(e.Bytes()); got > 0 { - t.Fatal("len(e.Bytes()), expected: 0, got:", got) - } -} - -func TestRunOutputTaskFoundErrors(t *testing.T) { - o := bytes.NewBuffer([]byte{}) - e := bytes.NewBuffer([]byte{}) - var analysisResult AnalysisResult - if err := mockAnalysisErrors("", &analysisResult); err != nil { - t.Fatal(err) - } - RunOutputTask(o, e, analysisResult) - - if !strings.Contains(string(o.Bytes()), "rhmap-core") { - t.Fatal("string(o.Bytes()), expected: to contain 'rhmap-core', got:", string(o.Bytes())) - } - if got := len(e.Bytes()); got > 0 { - t.Fatal("len(e.Bytes()), expected: 0, got:", got) - } -}