diff --git a/docs/collector.file.md b/docs/collector.file.md index b8ec1a78..49ca4920 100644 --- a/docs/collector.file.md +++ b/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 | diff --git a/docs/collector.registry.md b/docs/collector.registry.md index c3940db0..c01e9d56 100644 --- a/docs/collector.registry.md +++ b/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 diff --git a/internal/collector/file/file.go b/internal/collector/file/file.go index 138f3445..bd2d6402 100644 --- a/internal/collector/file/file.go +++ b/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) } diff --git a/internal/collector/file/file_test.go b/internal/collector/file/file_test.go index 5e7487c8..763072bd 100644 --- a/internal/collector/file/file_test.go +++ b/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") + } +} diff --git a/internal/collector/registry/registry.go b/internal/collector/registry/registry.go index 9ab136f9..108e50b6 100644 --- a/internal/collector/registry/registry.go +++ b/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. diff --git a/internal/collector/registry/registry_output_test.go b/internal/collector/registry/registry_output_test.go index d64c05b9..82c39ff1 100644 --- a/internal/collector/registry/registry_output_test.go +++ b/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() diff --git a/internal/collector/registry/registry_test.go b/internal/collector/registry/registry_test.go index 5bf0518a..1ee30dd3 100644 --- a/internal/collector/registry/registry_test.go +++ b/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{ diff --git a/internal/collector/update/update.go b/internal/collector/update/update.go index 89a7360a..e080680a 100644 --- a/internal/collector/update/update.go +++ b/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 diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d4a11974..589d3aa2 100644 --- a/internal/config/config_test.go +++ b/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()