From 177ac7f50f869361ef2f0bf4c31ab75d588ba7b3 Mon Sep 17 00:00:00 2001
From: Matt Layher <mdlayher@gmail.com>
Date: Fri, 22 Nov 2019 13:20:52 -0500
Subject: [PATCH] collector: refactor textfile collector to avoid looping defer

Signed-off-by: Matt Layher <mdlayher@gmail.com>
---
 collector/textfile.go | 129 ++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 56 deletions(-)

diff --git a/collector/textfile.go b/collector/textfile.go
index 52e88bb780..aee11db815 100644
--- a/collector/textfile.go
+++ b/collector/textfile.go
@@ -162,98 +162,115 @@ func convertMetricFamily(metricFamily *dto.MetricFamily, ch chan<- prometheus.Me
 }
 
 func (c *textFileCollector) exportMTimes(mtimes map[string]time.Time, ch chan<- prometheus.Metric) {
+	if len(mtimes) == 0 {
+		return
+	}
+
 	// Export the mtimes of the successful files.
-	if len(mtimes) > 0 {
-		// Sorting is needed for predictable output comparison in tests.
-		filenames := make([]string, 0, len(mtimes))
-		for filename := range mtimes {
-			filenames = append(filenames, filename)
-		}
-		sort.Strings(filenames)
+	// Sorting is needed for predictable output comparison in tests.
+	filenames := make([]string, 0, len(mtimes))
+	for filename := range mtimes {
+		filenames = append(filenames, filename)
+	}
+	sort.Strings(filenames)
 
-		for _, filename := range filenames {
-			mtime := float64(mtimes[filename].UnixNano() / 1e9)
-			if c.mtime != nil {
-				mtime = *c.mtime
-			}
-			ch <- prometheus.MustNewConstMetric(mtimeDesc, prometheus.GaugeValue, mtime, filename)
+	for _, filename := range filenames {
+		mtime := float64(mtimes[filename].UnixNano() / 1e9)
+		if c.mtime != nil {
+			mtime = *c.mtime
 		}
+		ch <- prometheus.MustNewConstMetric(mtimeDesc, prometheus.GaugeValue, mtime, filename)
 	}
 }
 
 // Update implements the Collector interface.
 func (c *textFileCollector) Update(ch chan<- prometheus.Metric) error {
-	error := 0.0
-	mtimes := map[string]time.Time{}
-
-	// Iterate over files and accumulate their metrics.
+	// Iterate over files and accumulate their metrics, but also track any
+	// parsing errors so an error metric can be reported.
+	var errored bool
 	files, err := ioutil.ReadDir(c.path)
 	if err != nil && c.path != "" {
-		log.Errorf("Error reading textfile collector directory %q: %s", c.path, err)
-		error = 1.0
+		errored = true
+		log.Errorf("failed to read textfile collector directory %q: %v", c.path, err)
 	}
 
+	mtimes := make(map[string]time.Time, len(files))
 	for _, f := range files {
 		if !strings.HasSuffix(f.Name(), ".prom") {
 			continue
 		}
-		path := filepath.Join(c.path, f.Name())
-		file, err := os.Open(path)
-		if err != nil {
-			log.Errorf("Error opening %q: %v", path, err)
-			error = 1.0
-			continue
-		}
-		defer file.Close()
-		var parser expfmt.TextParser
-		parsedFamilies, err := parser.TextToMetricFamilies(file)
-		if err != nil {
-			log.Errorf("Error parsing %q: %v", path, err)
-			error = 1.0
-			continue
-		}
-		if hasTimestamps(parsedFamilies) {
-			log.Errorf("Textfile %q contains unsupported client-side timestamps, skipping entire file", path)
-			error = 1.0
-			continue
-		}
 
-		for _, mf := range parsedFamilies {
-			if mf.Help == nil {
-				help := fmt.Sprintf("Metric read from %s", path)
-				mf.Help = &help
-			}
-		}
-
-		// Only set this once it has been parsed and validated, so that
-		// a failure does not appear fresh.
-		stat, err := file.Stat()
+		mtime, err := c.processFile(f.Name(), ch)
 		if err != nil {
-			log.Errorf("Error stat'ing %q: %v", path, err)
-			error = 1.0
+			errored = true
+			log.Errorf("failed to collect textfile data from %q: %v", f.Name(), err)
 			continue
 		}
-		mtimes[f.Name()] = stat.ModTime()
 
-		for _, mf := range parsedFamilies {
-			convertMetricFamily(mf, ch)
-		}
+		mtimes[f.Name()] = *mtime
 	}
 
 	c.exportMTimes(mtimes, ch)
 
 	// Export if there were errors.
+	var errVal float64
+	if errored {
+		errVal = 1.0
+	}
+
 	ch <- prometheus.MustNewConstMetric(
 		prometheus.NewDesc(
 			"node_textfile_scrape_error",
 			"1 if there was an error opening or reading a file, 0 otherwise",
 			nil, nil,
 		),
-		prometheus.GaugeValue, error,
+		prometheus.GaugeValue, errVal,
 	)
+
 	return nil
 }
 
+// processFile processes a single file, returning its modification time on success.
+func (c *textFileCollector) processFile(name string, ch chan<- prometheus.Metric) (*time.Time, error) {
+	path := filepath.Join(c.path, name)
+	f, err := os.Open(path)
+	if err != nil {
+		return nil, fmt.Errorf("failed to open textfile data file %q: %v", path, err)
+	}
+	defer f.Close()
+
+	var parser expfmt.TextParser
+	families, err := parser.TextToMetricFamilies(f)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse textfile data from %q: %v", path, err)
+	}
+
+	if hasTimestamps(families) {
+		return nil, fmt.Errorf("textfile %q contains unsupported client-side timestamps, skipping entire file", path)
+	}
+
+	for _, mf := range families {
+		if mf.Help == nil {
+			help := fmt.Sprintf("Metric read from %s", path)
+			mf.Help = &help
+		}
+	}
+
+	for _, mf := range families {
+		convertMetricFamily(mf, ch)
+	}
+
+	// Only stat the file once it has been parsed and validated, so that
+	// a failure does not appear fresh.
+	stat, err := f.Stat()
+	if err != nil {
+		return nil, fmt.Errorf("failed to stat %q: %v", path, err)
+	}
+
+	t := stat.ModTime()
+	return &t, nil
+}
+
 // hasTimestamps returns true when metrics contain unsupported timestamps.
 func hasTimestamps(parsedFamilies map[string]*dto.MetricFamily) bool {
 	for _, mf := range parsedFamilies {