fix: registry, update, file duplicate series, config key and error reporting (#2509)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>

docs/collector.file.md

@@ -15,6 +15,10 @@ Enabled by default? | No
 Comma-separated list of file patterns. Each pattern is a glob pattern that can contain `*`, `?`, and `**` (recursive).
 See https://github.com/bmatcuk/doublestar#patterns for an extended description of the pattern syntax.
 
+A pattern that matches nothing, including one whose directory does not exist, is not an error. If a directory
+cannot be read, for example because access is denied, the walk of that pattern stops and the collector is reported
+as failed (`windows_exporter_collector_success{collector="file"} 0`). Metrics of the other patterns are still exported.
+
 ## Metrics
 
 | Name                                   | Description            | Type  | Labels             |

docs/collector.registry.md

@@ -24,7 +24,7 @@ of a JSON array of objects. YAML is supported.
 
 > [!CAUTION]
 > If you are using a configuration file, the value must be kept as a string.
-> 
+>
 > Use a `|-` to keep the value as a string.
 
 #### Example
@@ -93,7 +93,7 @@ JSON:
 
 #### name
 
-Required, unique id for the key group. It is used as the `name` label on
+Required, unique ID for the key group. It is used as the `name` label on
 `windows_registry_key_success`, to identify the key in logs, and to seed
 auto-generated metric names (see [Metric naming](#metric-naming)). Each key must
 have a distinct `name`; duplicates are rejected at build time.
@@ -141,8 +141,8 @@ examples.
 The combination of metric name and labels must be unique across all configured
 keys and values. Two values may deliberately share a `metric` name when their
 labels differ (a common way to aggregate the same measurement from several keys).
-A true duplicate — the same name *and* identical labels — is dropped and logged at
-scrape time.
+A true duplicate — the same name *and* identical labels — is rejected at build
+time.
 
 ##### help
 

internal/collector/file/file.go

@@ -18,6 +18,7 @@
 package file
 
 import (
+	"errors"
 	"fmt"
 	"io/fs"
 	"log/slog"
@@ -139,6 +140,7 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error {
 // to the provided prometheus Metric channel.
 func (c *Collector) Collect(ch chan<- prometheus.Metric, _ time.Duration) error {
 	wg := sync.WaitGroup{}
+	errCh := make(chan error, len(c.config.FilePatterns))
 
 	for _, filePattern := range c.config.FilePatterns {
 		wg.Add(1)
@@ -147,23 +149,31 @@ func (c *Collector) Collect(ch chan<- prometheus.Metric, _ time.Duration) error
 			defer wg.Done()
 
 			if err := c.collectGlobFilePath(ch, filePattern); err != nil {
-				c.logger.Error("failed collecting metrics for filepath",
-					slog.String("filepath", filePattern),
-					slog.Any("err", err),
-				)
+				errCh <- fmt.Errorf("failed collecting metrics for file pattern %s: %w", filePattern, err)
 			}
 		}(filePattern)
 	}
 
 	wg.Wait()
 
-	return nil
+	close(errCh)
+
+	errs := make([]error, 0, len(c.config.FilePatterns))
+
+	for err := range errCh {
+		errs = append(errs, err)
+	}
+
+	return errors.Join(errs...)
 }
 
 func (c *Collector) collectGlobFilePath(ch chan<- prometheus.Metric, filePattern string) error {
 	basePath, pattern := doublestar.SplitPattern(filepath.ToSlash(filePattern))
 	basePathFS := os.DirFS(basePath)
 
+	// WithFailOnIOErrors reports unreadable directories instead of silently
+	// skipping them, so the collector is marked as failed. Paths that do not
+	// exist still count as patterns without matches.
 	err := doublestar.GlobWalk(basePathFS, pattern, func(path string, d fs.DirEntry) error {
 		filePath := filepath.Join(basePath, path)
 
@@ -194,7 +204,7 @@ func (c *Collector) collectGlobFilePath(ch chan<- prometheus.Metric, filePattern
 		)
 
 		return nil
-	}, doublestar.WithFilesOnly(), doublestar.WithCaseInsensitive())
+	}, doublestar.WithFilesOnly(), doublestar.WithCaseInsensitive(), doublestar.WithFailOnIOErrors())
 	if err != nil {
 		return fmt.Errorf("failed to glob: %w", err)
 	}

internal/collector/file/file_test.go

@@ -18,13 +18,17 @@
 package file_test
 
 import (
+	"io/fs"
+	"log/slog"
 	"os"
 	"path/filepath"
 	"testing"
 
 	"github.com/prometheus-community/windows_exporter/internal/collector/file"
 	"github.com/prometheus-community/windows_exporter/internal/utils/testutils"
+	"github.com/prometheus/client_golang/prometheus"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/windows"
 )
 
 func BenchmarkCollector(b *testing.B) {
@@ -62,6 +66,7 @@ func TestCollectorPatterns(t *testing.T) {
 		{name: "extension", pattern: "*.txt", count: 1},
 		{name: "case insensitive", pattern: "FIRST.TXT", count: 1},
 		{name: "missing file", pattern: "missing.txt", count: 0},
+		{name: "missing directory", pattern: filepath.Join("missing", "*.txt"), count: 0},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
@@ -80,3 +85,60 @@ func TestCollectorPatterns(t *testing.T) {
 		})
 	}
 }
+
+// TestCollectUnreadableDirectory verifies that a pattern whose directory cannot
+// be read makes Collect return an error, so the collector is reported as failed
+// instead of silently exporting nothing, while other patterns are still collected.
+func TestCollectUnreadableDirectory(t *testing.T) {
+	t.Parallel()
+
+	unreadable := t.TempDir()
+	denyAllAccess(t, unreadable)
+
+	readable := filepath.Join(t.TempDir(), "readable.txt")
+	require.NoError(t, os.WriteFile(readable, []byte("readable"), 0o600))
+
+	c := file.New(&file.Config{
+		FilePatterns: []string{filepath.Join(unreadable, "*.txt"), readable},
+	})
+	require.NoError(t, c.Build(slog.New(slog.DiscardHandler), nil))
+
+	ch := make(chan prometheus.Metric, 10)
+	err := c.Collect(ch, 0)
+	close(ch)
+
+	require.ErrorIs(t, err, fs.ErrPermission)
+	require.ErrorContains(t, err, unreadable)
+	require.Len(t, ch, 2, "metrics of the readable pattern must still be exported")
+}
+
+// denyAllAccess replaces the DACL of path with an empty, protected one, so it
+// can no longer be read. As the owner, the test keeps the right to change the
+// DACL, which the cleanup uses to restore access so the directory can be removed.
+func denyAllAccess(t *testing.T, path string) {
+	t.Helper()
+
+	original, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION)
+	require.NoError(t, err)
+
+	originalDACL, _, err := original.DACL()
+	require.NoError(t, err)
+
+	denyAll, err := windows.SecurityDescriptorFromString("D:P")
+	require.NoError(t, err)
+
+	denyAllDACL, _, err := denyAll.DACL()
+	require.NoError(t, err)
+
+	require.NoError(t, windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, nil, nil, denyAllDACL, nil))
+
+	t.Cleanup(func() {
+		require.NoError(t, windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT,
+			windows.DACL_SECURITY_INFORMATION|windows.UNPROTECTED_DACL_SECURITY_INFORMATION, nil, nil, originalDACL, nil))
+	})
+
+	if _, err := os.ReadDir(path); err == nil {
+		t.Skip("directory is still readable with an empty DACL, e.g. because the backup privilege is enabled")
+	}
+}

internal/collector/registry/registry.go

@@ -21,8 +21,10 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"maps"
 	"regexp"
 	"slices"
+	"strconv"
 	"strings"
 	"time"
 
@@ -135,6 +137,11 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error {
 	seenMetricHelps := make(map[string]string)
 	seenMetricTypes := make(map[string]prometheus.ValueType)
 
+	// seenSeries maps each metric name plus constant label set to the value
+	// that first produced it. Two values resolving to the same series would make
+	// every scrape fail to gather, so duplicates are rejected here as well.
+	seenSeries := make(map[string]string)
+
 	for _, key := range c.config.Keys {
 		if key.Key == "" {
 			errs = append(errs, errors.New("key is required"))
@@ -248,6 +255,18 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error {
 				seenMetricTypes[value.Metric] = value.metricType
 			}
 
+			series := seriesKey(value.Metric, value.Labels)
+			if prevValue, seen := seenSeries[series]; seen {
+				errs = append(errs, fmt.Errorf(
+					"value %q of key %s: metric %q with labels %v duplicates the series of %s; values sharing a metric name must have distinct labels",
+					value.Name, label, value.Metric, value.Labels, prevValue,
+				))
+
+				continue
+			}
+
+			seenSeries[series] = fmt.Sprintf("value %q of key %s", value.Name, label)
+
 			value.desc = prometheus.NewDesc(
 				value.Metric,
 				help,
@@ -378,6 +397,24 @@ func parseKeyPath(path string) (winregistry.Key, string, string, error) {
 	return hive, subPath, label, nil
 }
 
+// seriesKey returns a string identifying the series a value is exported as: its
+// metric name plus its constant labels sorted by name. Names and values are
+// quoted, so distinct label sets can never produce the same key.
+func seriesKey(metric string, labels map[string]string) string {
+	var sb strings.Builder
+
+	sb.WriteString(strconv.Quote(metric))
+
+	for _, name := range slices.Sorted(maps.Keys(labels)) {
+		sb.WriteString(",")
+		sb.WriteString(strconv.Quote(name))
+		sb.WriteString("=")
+		sb.WriteString(strconv.Quote(labels[name]))
+	}
+
+	return sb.String()
+}
+
 // sanitizeMetricName turns an arbitrary string into a valid Prometheus metric
 // name by lowercasing it, replacing every non-alphanumeric character with an
 // underscore, and trimming leading and trailing underscores.

internal/collector/registry/registry_output_test.go

@@ -209,7 +209,7 @@ func TestCollectMissingValueReturnsErrNoData(t *testing.T) {
 // TestCollectorSharedMetricName proves the supported aggregation pattern from the
 // docs: two values may share one metric name as long as their labels differ. The
 // collector deliberately does not reject this; only a true duplicate (same name and
-// identical labels) would be dropped and logged by the registry at scrape time.
+// identical labels) is rejected at build time.
 func TestCollectorSharedMetricName(t *testing.T) {
 	t.Parallel()
 

internal/collector/registry/registry_test.go

@@ -147,6 +147,34 @@ func TestCollectorBuildErrors(t *testing.T) {
 				},
 			}},
 		},
+		{
+			name: "shared metric name without labels",
+			config: registry.Config{Keys: []registry.Key{
+				{
+					Name: "crash_control",
+					Key:  `HKLM\SYSTEM\CurrentControlSet\Control\CrashControl`,
+					Values: []registry.Value{
+						{Name: "AutoReboot", Metric: "dup"},
+						{Name: "CrashDumpEnabled", Metric: "dup"},
+					},
+				},
+			}},
+		},
+		{
+			name: "shared metric name with identical labels across keys",
+			config: registry.Config{Keys: []registry.Key{
+				{
+					Name:   "crash_control",
+					Key:    `HKLM\SYSTEM\CurrentControlSet\Control\CrashControl`,
+					Values: []registry.Value{{Name: "AutoReboot", Metric: "dup", Labels: map[string]string{"a": "1", "b": "2"}}},
+				},
+				{
+					Name:   "windows_nt",
+					Key:    `HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion`,
+					Values: []registry.Value{{Name: "CurrentMajorVersionNumber", Metric: "dup", Labels: map[string]string{"b": "2", "a": "1"}}},
+				},
+			}},
+		},
 		{
 			name: "missing name",
 			config: registry.Config{Keys: []registry.Key{

internal/collector/update/update.go

@@ -40,7 +40,7 @@ const Name = "update"
 
 type Config struct {
 	Online         bool          `yaml:"online"`
-	ScrapeInterval time.Duration `yaml:"scrape_interval"`
+	ScrapeInterval time.Duration `yaml:"scrape-interval"`
 }
 
 //nolint:gochecknoglobals

internal/config/config_test.go

@@ -20,11 +20,18 @@ package config_test
 import (
 	"os"
 	"path/filepath"
+	"reflect"
+	"regexp"
+	"slices"
+	"strings"
 	"testing"
+	"time"
 
 	"github.com/alecthomas/kingpin/v2"
 	"github.com/prometheus-community/windows_exporter/internal/config"
+	"github.com/prometheus-community/windows_exporter/pkg/collector"
 	"github.com/stretchr/testify/require"
+	"go.yaml.in/yaml/v3"
 )
 
 func TestParseConfigFile(t *testing.T) {
@@ -91,6 +98,105 @@ func TestNewConfigFileResolver(t *testing.T) {
 	})
 }
 
+// TestNewConfigFileResolverCollectorFlags verifies that every collector flag can
+// be set from the configuration file: the nested key derived from the flag name
+// must exist in collector.Config, pass the strict validation, and be bound back
+// to the same flag.
+func TestNewConfigFileResolverCollectorFlags(t *testing.T) {
+	t.Parallel()
+
+	app := kingpin.New("test", "test")
+	collector.NewWithFlags(app)
+
+	for _, flag := range app.Model().Flags {
+		// collector.exchange.list prints the available sub-collectors and exits;
+		// it is not a configuration option.
+		if !strings.HasPrefix(flag.Name, "collector.") || flag.Name == "collector.exchange.list" {
+			continue
+		}
+
+		t.Run(flag.Name, func(t *testing.T) {
+			t.Parallel()
+
+			keys := strings.Split(flag.Name, ".")
+
+			field, ok := configFieldType(reflect.TypeFor[collector.Config](), keys[1:])
+			require.True(t, ok, "collector.Config has no field with the yaml path %s", flag.Name)
+
+			value, want := sampleConfigValue(t, field)
+
+			contents := value
+			for _, key := range slices.Backward(keys) {
+				contents = map[string]any{key: contents}
+			}
+
+			data, err := yaml.Marshal(contents)
+			require.NoError(t, err)
+
+			path := filepath.Join(t.TempDir(), "config.yaml")
+			require.NoError(t, os.WriteFile(path, data, 0o600))
+
+			resolver, err := config.NewConfigFileResolver(path)
+			require.NoError(t, err, "config:\n%s", data)
+
+			bindApp := kingpin.New("test", "test")
+			collector.NewWithFlags(bindApp)
+
+			require.NoError(t, resolver.Bind(bindApp, nil))
+			require.Equal(t, []string{want}, bindApp.GetFlag(flag.Name).Model().Default)
+		})
+	}
+}
+
+// configFieldType returns the type of the struct field reached by following
+// the yaml tags in keys, starting at typ.
+func configFieldType(typ reflect.Type, keys []string) (reflect.Type, bool) {
+	if len(keys) == 0 {
+		return typ, true
+	}
+
+	if typ.Kind() != reflect.Struct {
+		return nil, false
+	}
+
+	for field := range typ.Fields() {
+		if name, _, _ := strings.Cut(field.Tag.Get("yaml"), ","); name == keys[0] {
+			return configFieldType(field.Type, keys[1:])
+		}
+	}
+
+	return nil, false
+}
+
+// sampleConfigValue returns a configuration file value that decodes into typ,
+// together with the flag value the resolver flattens it to.
+func sampleConfigValue(t *testing.T, typ reflect.Type) (any, string) {
+	t.Helper()
+
+	switch {
+	case typ == reflect.TypeFor[time.Duration]():
+		return "1m", "1m"
+	case typ == reflect.TypeFor[*regexp.Regexp]():
+		return "test", "test"
+	}
+
+	switch typ.Kind() {
+	case reflect.Bool:
+		return true, "true"
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
+		reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
+		return 1, "1"
+	case reflect.Slice:
+		return []string{"test"}, "test"
+	case reflect.String:
+		return "test", "test"
+	default:
+		t.Fatalf("no sample configuration value for type %s", typ)
+
+		return nil, ""
+	}
+}
+
 func TestParse(t *testing.T) {
 	t.Parallel()