Merge pull request #1549 from prometheus/mdl-textfile

collector: refactor textfile collector to avoid looping defer

collector/textfile.go

@@ -162,8 +162,11 @@ 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 {
@@ -178,82 +181,96 @@ func (c *textFileCollector) exportMTimes(mtimes map[string]time.Time, ch chan<-
 		}
 		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)
+
+		mtime, err := c.processFile(f.Name(), ch)
 		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
+			errored = true
+			log.Errorf("failed to collect textfile data from %q: %v", f.Name(), err)
 			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()
-		if err != nil {
-			log.Errorf("Error stat'ing %q: %v", path, err)
-			error = 1.0
-			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 {