From af513468ebe0ed39c53e0531095699ad557b1e87 Mon Sep 17 00:00:00 2001 From: Julius Volz Date: Sun, 13 Sep 2015 01:06:40 +0200 Subject: [PATCH] Fix some dead code, missing error checks, shadowings. I applied https://medium.com/@jgautheron/quality-pipeline-for-go-projects-497e34d6567 and was greeted with a deluge of warnings, most of which were not applicable or really fixable realistically. These are some of the first ones I decided to fix. --- cmd/prometheus/config.go | 4 +++- promql/test.go | 1 - retrieval/discovery/file.go | 4 +++- storage/local/crashrecovery.go | 4 +++- storage/local/index/index.go | 18 +++++++++++++----- storage/local/persistence.go | 10 +++++++--- storage/local/storage.go | 4 +++- storage/local/test_helpers.go | 4 +++- template/template.go | 4 ++-- util/cli/cli.go | 5 ++++- util/flock/flock_test.go | 4 ++-- util/httputil/client.go | 12 ++++++++---- 12 files changed, 51 insertions(+), 23 deletions(-) diff --git a/cmd/prometheus/config.go b/cmd/prometheus/config.go index ef76003ad..cce47d198 100644 --- a/cmd/prometheus/config.go +++ b/cmd/prometheus/config.go @@ -304,5 +304,7 @@ func usage() { } } - t.Execute(os.Stdout, groups) + if err := t.Execute(os.Stdout, groups); err != nil { + panic(fmt.Errorf("error executing usage template: %s", err)) + } } diff --git a/promql/test.go b/promql/test.go index bdfc76eab..7b15c8f4a 100644 --- a/promql/test.go +++ b/promql/test.go @@ -41,7 +41,6 @@ var ( const ( testStartTime = model.Time(0) epsilon = 0.000001 // Relative error allowed for sample values. - maxErrorCount = 10 ) // Test is a sequence of read and write commands that are run diff --git a/retrieval/discovery/file.go b/retrieval/discovery/file.go index 756ccca44..23dd93b83 100644 --- a/retrieval/discovery/file.go +++ b/retrieval/discovery/file.go @@ -179,7 +179,9 @@ func (fd *FileDiscovery) stop() { } } }() - fd.watcher.Close() + if err := fd.watcher.Close(); err != nil { + log.Errorf("Error closing file watcher for %s: %s", fd.paths, err) + } log.Debugf("File discovery for %s stopped.", fd.paths) } diff --git a/storage/local/crashrecovery.go b/storage/local/crashrecovery.go index 69086ed1a..dbcdbd983 100644 --- a/storage/local/crashrecovery.go +++ b/storage/local/crashrecovery.go @@ -44,7 +44,9 @@ func (p *persistence) recoverFromCrash(fingerprintToSeries map[model.Fingerprint // Delete the fingerprint mapping file as it might be stale or // corrupt. We'll rebuild the mappings as we go. - os.Remove(p.mappingsFileName()) + if err := os.RemoveAll(p.mappingsFileName()); err != nil { + return fmt.Errorf("couldn't remove old fingerprint mapping file %s: %s", p.mappingsFileName(), err) + } // The mappings to rebuild. fpm := fpMappings{} diff --git a/storage/local/index/index.go b/storage/local/index/index.go index 8f1985f04..14200092a 100644 --- a/storage/local/index/index.go +++ b/storage/local/index/index.go @@ -57,7 +57,9 @@ func (i *FingerprintMetricIndex) IndexBatch(mapping FingerprintMetricMapping) er b := i.NewBatch() for fp, m := range mapping { - b.Put(codable.Fingerprint(fp), codable.Metric(m)) + if err := b.Put(codable.Fingerprint(fp), codable.Metric(m)); err != nil { + return err + } } return i.Commit(b) @@ -72,7 +74,9 @@ func (i *FingerprintMetricIndex) UnindexBatch(mapping FingerprintMetricMapping) b := i.NewBatch() for fp := range mapping { - b.Delete(codable.Fingerprint(fp)) + if err := b.Delete(codable.Fingerprint(fp)); err != nil { + return err + } } return i.Commit(b) @@ -196,14 +200,18 @@ type LabelPairFingerprintIndex struct { // // While this method is fundamentally goroutine-safe, note that the order of // execution for multiple batches executed concurrently is undefined. -func (i *LabelPairFingerprintIndex) IndexBatch(m LabelPairFingerprintsMapping) error { +func (i *LabelPairFingerprintIndex) IndexBatch(m LabelPairFingerprintsMapping) (err error) { batch := i.NewBatch() for pair, fps := range m { if len(fps) == 0 { - batch.Delete(codable.LabelPair(pair)) + err = batch.Delete(codable.LabelPair(pair)) } else { - batch.Put(codable.LabelPair(pair), fps) + err = batch.Put(codable.LabelPair(pair), fps) + } + + if err != nil { + return err } } diff --git a/storage/local/persistence.go b/storage/local/persistence.go index 5eaf0b13d..4f07f02e7 100644 --- a/storage/local/persistence.go +++ b/storage/local/persistence.go @@ -1123,7 +1123,7 @@ func (p *persistence) fingerprintsModifiedBefore(beforeTime model.Time) ([]model var fp codable.Fingerprint var tr codable.TimeRange fps := []model.Fingerprint{} - p.archivedFingerprintToTimeRange.ForEach(func(kv index.KeyValueAccessor) error { + err := p.archivedFingerprintToTimeRange.ForEach(func(kv index.KeyValueAccessor) error { if err := kv.Value(&tr); err != nil { return err } @@ -1135,7 +1135,7 @@ func (p *persistence) fingerprintsModifiedBefore(beforeTime model.Time) ([]model } return nil }) - return fps, nil + return fps, err } // archivedMetric retrieves the archived metric with the given fingerprint. This @@ -1438,11 +1438,15 @@ func (p *persistence) checkpointFPMappings(fpm fpMappings) (err error) { } defer func() { - f.Sync() + syncErr := f.Sync() closeErr := f.Close() if err != nil { return } + err = syncErr + if err != nil { + return + } err = closeErr if err != nil { return diff --git a/storage/local/storage.go b/storage/local/storage.go index 41c55be13..1982abe68 100644 --- a/storage/local/storage.go +++ b/storage/local/storage.go @@ -1111,7 +1111,9 @@ func (s *memorySeriesStorage) maintainArchivedSeries(fp model.Fingerprint, befor s.seriesOps.WithLabelValues(archivePurge).Inc() return } - s.persistence.updateArchivedTimeRange(fp, newFirstTime, lastTime) + if err := s.persistence.updateArchivedTimeRange(fp, newFirstTime, lastTime); err != nil { + log.Errorf("Error updating archived time range for fingerprint %v: %s", fp, err) + } } // See persistence.loadChunks for detailed explanation. diff --git a/storage/local/test_helpers.go b/storage/local/test_helpers.go index 33accf571..4e914b724 100644 --- a/storage/local/test_helpers.go +++ b/storage/local/test_helpers.go @@ -30,7 +30,9 @@ type testStorageCloser struct { } func (t *testStorageCloser) Close() { - t.storage.Stop() + if err := t.storage.Stop(); err != nil { + panic(err) + } t.directory.Close() } diff --git a/template/template.go b/template/template.go index 0c67362ef..3c66bbd2b 100644 --- a/template/template.go +++ b/template/template.go @@ -272,11 +272,11 @@ func (te Expander) Expand() (result string, resultErr error) { } }() - var buffer bytes.Buffer tmpl, err := text_template.New(te.name).Funcs(te.funcMap).Parse(te.text) if err != nil { return "", fmt.Errorf("error parsing template %v: %v", te.name, err) } + var buffer bytes.Buffer err = tmpl.Execute(&buffer, te.data) if err != nil { return "", fmt.Errorf("error executing template %v: %v", te.name, err) @@ -296,7 +296,6 @@ func (te Expander) ExpandHTML(templateFiles []string) (result string, resultErr } }() - var buffer bytes.Buffer tmpl := html_template.New(te.name).Funcs(html_template.FuncMap(te.funcMap)) tmpl.Funcs(html_template.FuncMap{ "tmpl": func(name string, data interface{}) (html_template.HTML, error) { @@ -315,6 +314,7 @@ func (te Expander) ExpandHTML(templateFiles []string) (result string, resultErr return "", fmt.Errorf("error parsing template files for %v: %v", te.name, err) } } + var buffer bytes.Buffer err = tmpl.Execute(&buffer, te.data) if err != nil { return "", fmt.Errorf("error executing template %v: %v", te.name, err) diff --git a/util/cli/cli.go b/util/cli/cli.go index 063db561a..065dfafe3 100644 --- a/util/cli/cli.go +++ b/util/cli/cli.go @@ -156,13 +156,16 @@ func BasicHelp(app *App, ts string) func() string { } var buf bytes.Buffer - t.Execute(&buf, struct { + err := t.Execute(&buf, struct { Name string Commands []command }{ Name: app.Name, Commands: cmds, }) + if err != nil { + panic(fmt.Errorf("error executing help template: %s", err)) + } return strings.TrimSpace(buf.String()) } } diff --git a/util/flock/flock_test.go b/util/flock/flock_test.go index d29957af8..6c26fd5f6 100644 --- a/util/flock/flock_test.go +++ b/util/flock/flock_test.go @@ -27,7 +27,7 @@ func TestLocking(t *testing.T) { } // File must now exist. - if _, err := os.Stat(fileName); err != nil { + if _, err = os.Stat(fileName); err != nil { t.Errorf("Could not stat file %q expected to exist: %s", fileName, err) } @@ -48,7 +48,7 @@ func TestLocking(t *testing.T) { } // File must still exist. - if _, err := os.Stat(fileName); err != nil { + if _, err = os.Stat(fileName); err != nil { t.Errorf("Could not stat file %q expected to exist: %s", fileName, err) } diff --git a/util/httputil/client.go b/util/httputil/client.go index c05b0494d..d7526cc6e 100644 --- a/util/httputil/client.go +++ b/util/httputil/client.go @@ -48,12 +48,16 @@ func NewDeadlineRoundTripper(timeout time.Duration, proxyURL *url.URL) http.Roun start := time.Now() c, err = net.DialTimeout(netw, addr, timeout) - - if err == nil { - c.SetDeadline(start.Add(timeout)) + if err != nil { + return nil, err } - return + if err = c.SetDeadline(start.Add(timeout)); err != nil { + c.Close() + return nil, err + } + + return c, nil }, } }