From 04ec4a0308235f9e1d992824464391c15e9b8ac2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan-Otto=20Kr=C3=B6pke?= Date: Thu, 15 Oct 2026 20:32:10 +0200 Subject: [PATCH] adfs: convert database query times with the counter time base (#2719) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jan-Otto Kröpke --- AGENTS.md | 4 + UPGRADE.md | 3 +- docs/collector.adfs.md | 7 +- internal/collector/adfs/AGENTS.md | 6 +- internal/collector/adfs/adfs.go | 170 +++++++++++------- internal/collector/adfs/adfs_internal_test.go | 130 ++++++++++++++ internal/collector/adfs/adfs_test.go | 13 +- internal/collector/adfs/types.go | 13 +- internal/pdh/collector.go | 47 ++++- internal/pdh/collector_internal_test.go | 50 ++++++ 10 files changed, 366 insertions(+), 77 deletions(-) create mode 100644 internal/collector/adfs/adfs_internal_test.go diff --git a/AGENTS.md b/AGENTS.md index 4ace6ef5..f950327e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -103,6 +103,10 @@ exercised a live Windows role. appropriate. Raw PDH values can be cumulative numerators, ticks or counts, even when the Windows counter name says average, percentage or seconds. Convert units explicitly; expose the base/operation count needed for ratios. + The raw value of a `PERF_AVERAGE_TIMER` counter is in ticks of its time + base: in new code, divide it by `pdh.Collector.Frequency` instead of a fixed + factor. The disk and `smbclient` collectors still use the fixed 1e-7 + (10 MHz) factor. Never publish an invalid sample as zero: that can look like a counter reset. - Keep the typed PDH collectors and `perfdata` tags. Preserve duplicate instance occurrences within a sample; their suffixes are not stable identities. diff --git a/UPGRADE.md b/UPGRADE.md index efb93cbf..0bff8ecb 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -19,7 +19,7 @@ Most installations need no changes. Go through the checklist below and read the | You alert on BitLocker status | Replace `status="disabled"` with `status="off"` | [BitLocker status](#bitlocker-status) | | You query Active Directory latency or security descriptor propagation, DFSR, Hyper-V, logical disk queue, cache, IIS output cache, memory, RemoteFX, scheduled task, SMB server share or terminal services metrics | Update queries | [Renamed, deprecated and retyped metrics](#renamed-deprecated-and-retyped-metrics) | | You enable the `scheduled_task` collector and use tasks below `\Microsoft\` or missed runs | Set `--collector.scheduled_task.exclude` or `missed-runs` | [scheduled_task collector](#scheduled_task-collector) | -| You chart SQL Server latencies, SQL Server lock counts, DHCP denials, AD LSA writes, AD computer creations or RemoteFX skipped frames | Expect corrected values | [Corrected values](#corrected-values) | +| You chart SQL Server latencies, SQL Server lock counts, DHCP denials, AD LSA writes, AD computer creations, AD FS database query times or RemoteFX skipped frames | Expect corrected values | [Corrected values](#corrected-values) | | You enable the `hyperv` or `mscluster` collector | Check the new default sub-collectors | [New default sub-collectors](#new-default-sub-collectors) | | You set scrape timeouts above 5 minutes or below the timeout margin | Review the new limits | [Scrape timeouts](#scrape-timeouts) | | You use the sample Grafana dashboard | Use Grafana 13 or later and import the new dashboard | [Dashboard and alerts](#dashboard-and-alerts) | @@ -311,6 +311,7 @@ These metrics keep their names, but 0.31 reported wrong values. Graphs show a st - `windows_mssql_locks_count` is no longer divided by 1000. - `windows_ad_directory_operations_total{operation="write",origin="local_security_authority"}` now counts directory writes from the LSA. In 0.31 it repeated the LSA search count. - `windows_ad_sam_computer_creation_requests_total` and `windows_ad_sam_computer_creation_successful_requests_total` were swapped in 0.31. Requests now count all attempts, successful requests only the successful ones. +- `windows_adfs_db_artifact_query_time_seconds_total` and `windows_adfs_db_config_query_time_seconds_total` are now in seconds. 0.31 scaled the raw ticks by a fixed `1e-8`, so on a typical 10 MHz time base the values were 10 times too small. The new `windows_adfs_db_artifact_queries_total` and `windows_adfs_db_config_queries_total` count the queries, so you can compute the average query time. - `windows_dhcp_denied_due_to_nonmatch_total` now counts denials due to a non-match. In 0.31 it repeated the match denial count. - `windows_remote_fx_gfx_frames_skipped_insufficient_resource_total` with `resource="client"` and `resource="server"` now count the matching frames. In 0.31 the two label values were swapped. - `windows_netframework_clrmemory_gc_time_percent` no longer overflows, and it is left out when Windows reports no base value. The `netframework` collector now reads the CLR counters through PDH instead of WMI; names, labels and types are unchanged. diff --git a/docs/collector.adfs.md b/docs/collector.adfs.md index a8414376..0eecb6e7 100644 --- a/docs/collector.adfs.md +++ b/docs/collector.adfs.md @@ -57,9 +57,11 @@ Name | Description | Type | Labels `windows_adfs_external_authentications_failure_total` | Total number of failed authentications from external MFA providers | counter | None `windows_adfs_external_authentications_success_total` | Total number of successful authentications from external MFA providers | counter | None `windows_adfs_db_artifact_failure_total` | Total number of failures connecting to the artifact database | counter | None -`windows_adfs_db_artifact_query_time_seconds_total` | Accumulator of time taken for an artifact database query | counter | None +`windows_adfs_db_artifact_queries_total` | Total number of artifact database queries | counter | None +`windows_adfs_db_artifact_query_time_seconds_total` | Total time spent on artifact database queries, in seconds | counter | None `windows_adfs_db_config_failure_total` | Total number of failures connecting to the configuration database | counter | None -`windows_adfs_db_config_query_time_seconds_total` | Accumulator of time taken for a configuration database query | counter | None +`windows_adfs_db_config_queries_total` | Total number of configuration database queries | counter | None +`windows_adfs_db_config_query_time_seconds_total` | Total time spent on configuration database queries, in seconds | counter | None `windows_adfs_federation_metadata_requests_total` | Total number of Federation Metadata requests | counter | None ### Example metric @@ -74,6 +76,7 @@ rate(windows_adfs_device_authentications_total[2m]) |---|----| |`rate(windows_adfs_oauth_password_grant_requests_failure_total[5m])`| Rate of OAuth requests failing due to bad client/resource values| |`rate(windows_adfs_userpassword_authentications_failure_total[5m])`| Rate of `/adfs/oauth2/token/` requests failing due to bad username/password values (possible credential spraying)| +|`rate(windows_adfs_db_config_query_time_seconds_total[5m]) / rate(windows_adfs_db_config_queries_total[5m])`| Average configuration database query time in seconds| ## Alerting examples **prometheus.rules** diff --git a/internal/collector/adfs/AGENTS.md b/internal/collector/adfs/AGENTS.md index 20b52919..90550bcf 100644 --- a/internal/collector/adfs/AGENTS.md +++ b/internal/collector/adfs/AGENTS.md @@ -22,9 +22,9 @@ one. duration in ticks of the counter's time base (`PdhGetCounterTimeBase`, 10 MHz on the test host, same as `QueryPerformanceFrequency`); the base (`,secondvalue`) is the number of operations. `pdh.CounterTypeRaw` does not - convert this type, and `internal/pdh` reads the time base only for - `PERF_ELAPSED_TIME`. Seconds are `raw / time base`; do not hardcode a scale - factor. + convert this type; `internal/pdh` reads its time base, and the collector + divides the raw value by `pdh.Collector.Frequency` to get seconds. Do not + hardcode a scale factor. - Sample after about one hour: `Average Config Database Query Time` raw 11521176, base 306, which is 1.152 s in total, 3.8 ms per query. `Federation Metadata Requests` was 1, all authentication counters 0. diff --git a/internal/collector/adfs/adfs.go b/internal/collector/adfs/adfs.go index a7873c6c..6e3ebcb1 100644 --- a/internal/collector/adfs/adfs.go +++ b/internal/collector/adfs/adfs.go @@ -20,7 +20,6 @@ package adfs import ( "fmt" "log/slog" - "math" "time" "github.com/alecthomas/kingpin/v2" @@ -45,8 +44,10 @@ type Collector struct { adLoginConnectionFailures *prometheus.Desc artifactDBFailures *prometheus.Desc - avgArtifactDBQueryTime *prometheus.Desc - avgConfigDBQueryTime *prometheus.Desc + artifactDBQueries *prometheus.Desc + artifactDBQueryTime *prometheus.Desc + configDBQueries *prometheus.Desc + configDBQueryTime *prometheus.Desc certificateAuthentications *prometheus.Desc configDBFailures *prometheus.Desc deviceAuthentications *prometheus.Desc @@ -115,6 +116,19 @@ func (c *Collector) Close() error { } func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error { + c.buildDescriptors() + + var err error + + c.perfDataCollector, err = pdh.NewCollector[perfDataCounterValues](logger.With(slog.String("collector", Name)), pdh.CounterTypeRaw, "AD FS", nil) + if err != nil { + return fmt.Errorf("failed to create AD FS collector: %w", err) + } + + return nil +} + +func (c *Collector) buildDescriptors() { c.adLoginConnectionFailures = prometheus.NewDesc( prometheus.BuildFQName(types.Namespace, Name, "ad_login_connection_failures_total"), "Total number of connection failures to an Active Directory domain controller", @@ -349,9 +363,15 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error { nil, nil, ) - c.avgArtifactDBQueryTime = prometheus.NewDesc( + c.artifactDBQueryTime = prometheus.NewDesc( prometheus.BuildFQName(types.Namespace, Name, "db_artifact_query_time_seconds_total"), - "Accumulator of time taken for an artifact database query", + "Total time spent on artifact database queries, in seconds", + nil, + nil, + ) + c.artifactDBQueries = prometheus.NewDesc( + prometheus.BuildFQName(types.Namespace, Name, "db_artifact_queries_total"), + "Total number of artifact database queries", nil, nil, ) @@ -361,9 +381,15 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error { nil, nil, ) - c.avgConfigDBQueryTime = prometheus.NewDesc( + c.configDBQueryTime = prometheus.NewDesc( prometheus.BuildFQName(types.Namespace, Name, "db_config_query_time_seconds_total"), - "Accumulator of time taken for a configuration database query", + "Total time spent on configuration database queries, in seconds", + nil, + nil, + ) + c.configDBQueries = prometheus.NewDesc( + prometheus.BuildFQName(types.Namespace, Name, "db_config_queries_total"), + "Total number of configuration database queries", nil, nil, ) @@ -373,15 +399,6 @@ func (c *Collector) Build(logger *slog.Logger, _ *mi.Session) error { nil, nil, ) - - var err error - - c.perfDataCollector, err = pdh.NewCollector[perfDataCounterValues](logger.With(slog.String("collector", Name)), pdh.CounterTypeRaw, "AD FS", nil) - if err != nil { - return fmt.Errorf("failed to create AD FS collector: %w", err) - } - - return nil } func (c *Collector) Collect(ch chan<- prometheus.Metric, _ time.Duration) error { @@ -392,263 +409,290 @@ func (c *Collector) Collect(ch chan<- prometheus.Metric, _ time.Duration) error return fmt.Errorf("failed to collect ADFS metrics: %w", types.ErrNoDataUnexpected) } + // The query times are PERF_AVERAGE_TIMER counters: the raw value is the + // summed duration in ticks of the counter's time base. + artifactDBFrequency, _ := c.perfDataCollector.Frequency(counterArtifactDBQueryTime) + configDBFrequency, _ := c.perfDataCollector.Frequency(counterConfigDBQueryTime) + + c.collectMetrics(ch, c.perfDataObject[0], artifactDBFrequency, configDBFrequency) + + return nil +} + +// collectMetrics sends the metrics of data. A query time is only sent if the +// frequency of its counter is known. +func (c *Collector) collectMetrics(ch chan<- prometheus.Metric, data perfDataCounterValues, artifactDBFrequency, configDBFrequency int64) { ch <- prometheus.MustNewConstMetric( c.adLoginConnectionFailures, prometheus.CounterValue, - c.perfDataObject[0].AdLoginConnectionFailures, + data.AdLoginConnectionFailures, ) ch <- prometheus.MustNewConstMetric( c.certificateAuthentications, prometheus.CounterValue, - c.perfDataObject[0].CertificateAuthentications, + data.CertificateAuthentications, ) ch <- prometheus.MustNewConstMetric( c.deviceAuthentications, prometheus.CounterValue, - c.perfDataObject[0].DeviceAuthentications, + data.DeviceAuthentications, ) ch <- prometheus.MustNewConstMetric( c.extranetAccountLockouts, prometheus.CounterValue, - c.perfDataObject[0].ExtranetAccountLockouts, + data.ExtranetAccountLockouts, ) ch <- prometheus.MustNewConstMetric( c.federatedAuthentications, prometheus.CounterValue, - c.perfDataObject[0].FederatedAuthentications, + data.FederatedAuthentications, ) ch <- prometheus.MustNewConstMetric( c.passportAuthentications, prometheus.CounterValue, - c.perfDataObject[0].PassportAuthentications, + data.PassportAuthentications, ) ch <- prometheus.MustNewConstMetric( c.passiveRequests, prometheus.CounterValue, - c.perfDataObject[0].PassiveRequests, + data.PassiveRequests, ) ch <- prometheus.MustNewConstMetric( c.passwordChangeFailed, prometheus.CounterValue, - c.perfDataObject[0].PasswordChangeFailed, + data.PasswordChangeFailed, ) ch <- prometheus.MustNewConstMetric( c.passwordChangeSucceeded, prometheus.CounterValue, - c.perfDataObject[0].PasswordChangeSucceeded, + data.PasswordChangeSucceeded, ) ch <- prometheus.MustNewConstMetric( c.tokenRequests, prometheus.CounterValue, - c.perfDataObject[0].TokenRequests, + data.TokenRequests, ) ch <- prometheus.MustNewConstMetric( c.windowsIntegratedAuthentications, prometheus.CounterValue, - c.perfDataObject[0].WindowsIntegratedAuthentications, + data.WindowsIntegratedAuthentications, ) ch <- prometheus.MustNewConstMetric( c.oAuthAuthZRequests, prometheus.CounterValue, - c.perfDataObject[0].OAuthAuthZRequests, + data.OAuthAuthZRequests, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientAuthentications, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientAuthentications, + data.OAuthClientAuthentications, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientAuthenticationsFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientAuthenticationFailures, + data.OAuthClientAuthenticationFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientCredentialsRequestFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientCredentialRequestFailures, + data.OAuthClientCredentialRequestFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientCredentialsRequests, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientCredentialRequests, + data.OAuthClientCredentialRequests, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientPrivateKeyJwtAuthenticationFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientPrivateKeyJWTAuthenticationFailures, + data.OAuthClientPrivateKeyJWTAuthenticationFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientPrivateKeyJwtAuthentications, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientPrivateKeyJWTAuthentications, + data.OAuthClientPrivateKeyJWTAuthentications, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientSecretBasicAuthenticationFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientBasicAuthenticationFailures, + data.OAuthClientBasicAuthenticationFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientSecretBasicAuthentications, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientBasicAuthentications, + data.OAuthClientBasicAuthentications, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientSecretPostAuthenticationFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientSecretPostAuthenticationFailures, + data.OAuthClientSecretPostAuthenticationFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientSecretPostAuthentications, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientSecretPostAuthentications, + data.OAuthClientSecretPostAuthentications, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientWindowsIntegratedAuthenticationFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientWindowsAuthenticationFailures, + data.OAuthClientWindowsAuthenticationFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthClientWindowsIntegratedAuthentications, prometheus.CounterValue, - c.perfDataObject[0].OAuthClientWindowsAuthentications, + data.OAuthClientWindowsAuthentications, ) ch <- prometheus.MustNewConstMetric( c.oAuthLogonCertificateRequestFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthLogonCertRequestFailures, + data.OAuthLogonCertRequestFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthLogonCertificateTokenRequests, prometheus.CounterValue, - c.perfDataObject[0].OAuthLogonCertTokenRequests, + data.OAuthLogonCertTokenRequests, ) ch <- prometheus.MustNewConstMetric( c.oAuthPasswordGrantRequestFailures, prometheus.CounterValue, - c.perfDataObject[0].OAuthPasswordGrantRequestFailures, + data.OAuthPasswordGrantRequestFailures, ) ch <- prometheus.MustNewConstMetric( c.oAuthPasswordGrantRequests, prometheus.CounterValue, - c.perfDataObject[0].OAuthPasswordGrantRequests, + data.OAuthPasswordGrantRequests, ) ch <- prometheus.MustNewConstMetric( c.oAuthTokenRequests, prometheus.CounterValue, - c.perfDataObject[0].OAuthTokenRequests, + data.OAuthTokenRequests, ) ch <- prometheus.MustNewConstMetric( c.samlPTokenRequests, prometheus.CounterValue, - c.perfDataObject[0].SamlPTokenRequests, + data.SamlPTokenRequests, ) ch <- prometheus.MustNewConstMetric( c.ssoAuthenticationFailures, prometheus.CounterValue, - c.perfDataObject[0].SsoAuthenticationFailures, + data.SsoAuthenticationFailures, ) ch <- prometheus.MustNewConstMetric( c.ssoAuthentications, prometheus.CounterValue, - c.perfDataObject[0].SsoAuthentications, + data.SsoAuthentications, ) ch <- prometheus.MustNewConstMetric( c.wsFedTokenRequests, prometheus.CounterValue, - c.perfDataObject[0].WsFedTokenRequests, + data.WsFedTokenRequests, ) ch <- prometheus.MustNewConstMetric( c.wsTrustTokenRequests, prometheus.CounterValue, - c.perfDataObject[0].WsTrustTokenRequests, + data.WsTrustTokenRequests, ) ch <- prometheus.MustNewConstMetric( c.upAuthenticationFailures, prometheus.CounterValue, - c.perfDataObject[0].UsernamePasswordAuthenticationFailures, + data.UsernamePasswordAuthenticationFailures, ) ch <- prometheus.MustNewConstMetric( c.upAuthentications, prometheus.CounterValue, - c.perfDataObject[0].UsernamePasswordAuthentications, + data.UsernamePasswordAuthentications, ) ch <- prometheus.MustNewConstMetric( c.externalAuthenticationFailures, prometheus.CounterValue, - c.perfDataObject[0].ExternalAuthNFailures, + data.ExternalAuthNFailures, ) ch <- prometheus.MustNewConstMetric( c.externalAuthentications, prometheus.CounterValue, - c.perfDataObject[0].ExternalAuthentications, + data.ExternalAuthentications, ) ch <- prometheus.MustNewConstMetric( c.artifactDBFailures, prometheus.CounterValue, - c.perfDataObject[0].ArtifactDBFailures, + data.ArtifactDBFailures, ) ch <- prometheus.MustNewConstMetric( - c.avgArtifactDBQueryTime, + c.artifactDBQueries, prometheus.CounterValue, - c.perfDataObject[0].AvgArtifactDBQueryTime*math.Pow(10, -8), + data.ArtifactDBQueries, ) + if artifactDBFrequency > 0 { + ch <- prometheus.MustNewConstMetric( + c.artifactDBQueryTime, + prometheus.CounterValue, + data.ArtifactDBQueryTimeTicks/float64(artifactDBFrequency), + ) + } + ch <- prometheus.MustNewConstMetric( c.configDBFailures, prometheus.CounterValue, - c.perfDataObject[0].ConfigDBFailures, + data.ConfigDBFailures, ) ch <- prometheus.MustNewConstMetric( - c.avgConfigDBQueryTime, + c.configDBQueries, prometheus.CounterValue, - c.perfDataObject[0].AvgConfigDBQueryTime*math.Pow(10, -8), + data.ConfigDBQueries, ) + if configDBFrequency > 0 { + ch <- prometheus.MustNewConstMetric( + c.configDBQueryTime, + prometheus.CounterValue, + data.ConfigDBQueryTimeTicks/float64(configDBFrequency), + ) + } + ch <- prometheus.MustNewConstMetric( c.federationMetadataRequests, prometheus.CounterValue, - c.perfDataObject[0].FederationMetadataRequests, + data.FederationMetadataRequests, ) - - return nil } diff --git a/internal/collector/adfs/adfs_internal_test.go b/internal/collector/adfs/adfs_internal_test.go new file mode 100644 index 00000000..047d9c5b --- /dev/null +++ b/internal/collector/adfs/adfs_internal_test.go @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Copyright The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build windows + +package adfs + +import ( + "testing" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/require" +) + +// TestCollectDatabaseQueryTimes checks that the raw ticks of the database +// query time counters are converted to seconds with the counter's time base, +// and that the query counts are exported. +func TestCollectDatabaseQueryTimes(t *testing.T) { + t.Parallel() + + data := perfDataCounterValues{ + ArtifactDBQueryTimeTicks: 2_500_000, + ArtifactDBQueries: 5, + ConfigDBQueryTimeTicks: 11_521_176, + ConfigDBQueries: 306, + } + + for _, tc := range []struct { + name string + artifactDBFrequency int64 + configDBFrequency int64 + expected map[string]float64 + absent []string + }{ + { + name: "10 MHz", + artifactDBFrequency: 10_000_000, + configDBFrequency: 10_000_000, + expected: map[string]float64{ + "windows_adfs_db_artifact_query_time_seconds_total": 0.25, + "windows_adfs_db_artifact_queries_total": 5, + "windows_adfs_db_config_query_time_seconds_total": 1.1521176, + "windows_adfs_db_config_queries_total": 306, + }, + }, + { + name: "other frequency", + artifactDBFrequency: 2_500_000, + configDBFrequency: 3_000_000, + expected: map[string]float64{ + "windows_adfs_db_artifact_query_time_seconds_total": 1, + "windows_adfs_db_config_query_time_seconds_total": 3.840392, + }, + }, + { + name: "unknown frequency", + expected: map[string]float64{ + "windows_adfs_db_artifact_queries_total": 5, + "windows_adfs_db_config_queries_total": 306, + }, + absent: []string{ + "windows_adfs_db_artifact_query_time_seconds_total", + "windows_adfs_db_config_query_time_seconds_total", + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + c := New(nil) + c.buildDescriptors() + + registry := prometheus.NewPedanticRegistry() + require.NoError(t, registry.Register(metricCollector{ + collector: c, + data: data, + artifactDBFrequency: tc.artifactDBFrequency, + configDBFrequency: tc.configDBFrequency, + })) + + families, err := registry.Gather() + require.NoError(t, err) + + byName := make(map[string]*dto.MetricFamily, len(families)) + for _, family := range families { + byName[family.GetName()] = family + } + + for name, value := range tc.expected { + family, ok := byName[name] + require.True(t, ok, "missing metric %s", name) + require.Equal(t, dto.MetricType_COUNTER, family.GetType(), name) + require.Len(t, family.GetMetric(), 1, name) + require.InDelta(t, value, family.GetMetric()[0].GetCounter().GetValue(), 1e-9, name) + } + + for _, name := range tc.absent { + require.NotContains(t, byName, name) + } + }) + } +} + +type metricCollector struct { + collector *Collector + data perfDataCounterValues + artifactDBFrequency int64 + configDBFrequency int64 +} + +func (c metricCollector) Describe(ch chan<- *prometheus.Desc) { + prometheus.DescribeByCollect(c, ch) +} + +func (c metricCollector) Collect(ch chan<- prometheus.Metric) { + c.collector.collectMetrics(ch, c.data, c.artifactDBFrequency, c.configDBFrequency) +} diff --git a/internal/collector/adfs/adfs_test.go b/internal/collector/adfs/adfs_test.go index 14aa2e87..39193559 100644 --- a/internal/collector/adfs/adfs_test.go +++ b/internal/collector/adfs/adfs_test.go @@ -22,6 +22,7 @@ import ( "github.com/prometheus-community/windows_exporter/internal/collector/adfs" "github.com/prometheus-community/windows_exporter/internal/utils/testutils" + "github.com/stretchr/testify/require" ) func BenchmarkCollector(b *testing.B) { @@ -29,5 +30,15 @@ func BenchmarkCollector(b *testing.B) { } func TestCollector(t *testing.T) { - testutils.TestCollector(t, adfs.New, nil) + families := testutils.TestCollector(t, adfs.New, nil) + + // The query times need the time base of their PERF_AVERAGE_TIMER counter. + for _, name := range []string{ + "windows_adfs_db_artifact_queries_total", + "windows_adfs_db_artifact_query_time_seconds_total", + "windows_adfs_db_config_queries_total", + "windows_adfs_db_config_query_time_seconds_total", + } { + require.Contains(t, families, name) + } } diff --git a/internal/collector/adfs/types.go b/internal/collector/adfs/types.go index 44893e39..0f840319 100644 --- a/internal/collector/adfs/types.go +++ b/internal/collector/adfs/types.go @@ -17,11 +17,20 @@ package adfs +// PERF_AVERAGE_TIMER counters. The raw value is the summed query time in ticks +// of the counter's time base, the second value the number of queries. +const ( + counterArtifactDBQueryTime = "Average Artifact Database Query Time" + counterConfigDBQueryTime = "Average Config Database Query Time" +) + type perfDataCounterValues struct { AdLoginConnectionFailures float64 `perfdata:"AD Login Connection Failures"` ArtifactDBFailures float64 `perfdata:"Artifact Database Connection Failures"` - AvgArtifactDBQueryTime float64 `perfdata:"Average Artifact Database Query Time"` - AvgConfigDBQueryTime float64 `perfdata:"Average Config Database Query Time"` + ArtifactDBQueryTimeTicks float64 `perfdata:"Average Artifact Database Query Time"` + ArtifactDBQueries float64 `perfdata:"Average Artifact Database Query Time,secondvalue"` + ConfigDBQueryTimeTicks float64 `perfdata:"Average Config Database Query Time"` + ConfigDBQueries float64 `perfdata:"Average Config Database Query Time,secondvalue"` CertificateAuthentications float64 `perfdata:"Certificate Authentications"` ConfigDBFailures float64 `perfdata:"Configuration Database Connection Failures"` DeviceAuthentications float64 `perfdata:"Device Authentications"` diff --git a/internal/pdh/collector.go b/internal/pdh/collector.go index d070f23b..ab2e58a3 100644 --- a/internal/pdh/collector.go +++ b/internal/pdh/collector.go @@ -345,12 +345,19 @@ func newCollector[T any](logger *slog.Logger, query *Query, resultType CounterTy counter.Type = counterInfo.DwType counter.MetricType = metricType(resultType, counter.Type) - if counter.Type == PERF_ELAPSED_TIME { + switch counter.Type { + case PERF_ELAPSED_TIME: if ret := GetCounterTimeBase(counterHandle, &counter.Frequency); ret != ErrorSuccess && ret != NoData { errs = append(errs, fmt.Errorf("GetCounterTimeBase: %w", NewPdhError(ret))) continue } + case PERF_AVERAGE_TIMER: + // Only [Collector.Frequency] uses the time base of an average + // timer, so a missing one is not an error. Collect retries it. + if ret := GetCounterTimeBase(counterHandle, &counter.Frequency); ret != ErrorSuccess { + counter.Frequency = 0 + } } } } @@ -440,6 +447,29 @@ func (c *Collector[T]) MetricType(counterName string) (prometheus.ValueType, boo return 0, false } +// Frequency returns the time base in ticks per second of the raw timer counter +// named counterName. The raw value of a PERF_AVERAGE_TIMER counter is the sum +// of the measured durations in these ticks; divide it by the frequency to get +// seconds. It returns false if the counter is unknown, is not such a timer or +// its time base is not known yet. [Collector.Collect] reads a missing time +// base again. +func (c *Collector[T]) Frequency(counterName string) (int64, bool) { + if c == nil { + return 0, false + } + + c.mu.RLock() + defer c.mu.RUnlock() + + for _, counter := range c.counters { + if counter.Name == counterName { + return counter.Frequency, counter.Frequency > 0 + } + } + + return 0, false +} + // Collect replaces the content of dst with one row per collected instance. func (c *Collector[T]) Collect(dst *[]T) error { if c == nil { @@ -540,12 +570,19 @@ func (c *Collector[T]) collect(dst *[]T, state *collectState) (err error) { return nil } -// loadTimeBase reads the time base of a raw elapsed time counter that had none -// when the collector was created. Early after a reboot, PDH can report no data +// hasTimeBase reports whether the raw value of a counter of counterType is +// measured in ticks of the counter's time base. +func hasTimeBase(counterType uint32) bool { + return counterType == PERF_ELAPSED_TIME || counterType == PERF_AVERAGE_TIMER +} + +// loadTimeBase reads the time base of a raw timer counter that had none when +// the collector was created. Early after a reboot, PDH can report no data // for it, e.g. for APP_POOL_WAS while WAS is still starting (#2313). Without a -// time base, the counter has no valid value and every instance is omitted. +// time base, an elapsed time counter has no valid value and every instance is +// omitted. func (c *Collector[T]) loadTimeBase(counter *Counter) { - if c.resultType != CounterTypeRaw || counter.Type != PERF_ELAPSED_TIME || counter.Frequency > 0 { + if c.resultType != CounterTypeRaw || !hasTimeBase(counter.Type) || counter.Frequency > 0 { return } diff --git a/internal/pdh/collector_internal_test.go b/internal/pdh/collector_internal_test.go index e9395ac5..eb13e9f5 100644 --- a/internal/pdh/collector_internal_test.go +++ b/internal/pdh/collector_internal_test.go @@ -397,6 +397,56 @@ func TestCollectLoadsMissingTimeBase(t *testing.T) { require.Positive(t, dst[0].SystemUpTime) } +// TestFrequencyOfAverageTimer checks that the time base of a raw average timer +// is available, also when it was missing at startup, and that other counters +// have none. +func TestFrequencyOfAverageTimer(t *testing.T) { + t.Parallel() + + type physicalDisk struct { + AvgDiskSecPerRead float64 `perfdata:"Avg. Disk sec/Read"` + AvgDiskSecPerReadBase float64 `perfdata:"Avg. Disk sec/Read,secondvalue"` + DiskReadsPerSec float64 `perfdata:"Disk Reads/sec"` + CurrentDiskQueueLength float64 `perfdata:"Current Disk Queue Length"` + } + + collector, err := NewCollector[physicalDisk](slog.New(slog.DiscardHandler), CounterTypeRaw, "PhysicalDisk", []string{"_Total"}) + require.NoError(t, err) + + t.Cleanup(collector.Close) + + frequency, ok := collector.Frequency("Avg. Disk sec/Read") + require.True(t, ok) + require.Positive(t, frequency) + + for _, name := range []string{"Disk Reads/sec", "Current Disk Queue Length", "unknown"} { + _, ok = collector.Frequency(name) + require.False(t, ok, name) + } + + // As if GetCounterTimeBase failed at startup. + idx := slices.IndexFunc(collector.counters, func(counter Counter) bool { return counter.Name == "Avg. Disk sec/Read" }) + require.NotEqual(t, -1, idx) + + collector.counters[idx].Frequency = 0 + + _, ok = collector.Frequency("Avg. Disk sec/Read") + require.False(t, ok) + + var dst []physicalDisk + + require.NoError(t, collector.Collect(&dst)) + + reloaded, ok := collector.Frequency("Avg. Disk sec/Read") + require.True(t, ok) + require.Equal(t, frequency, reloaded) + + var nilCollector *Collector[physicalDisk] + + _, ok = nilCollector.Frequency("Avg. Disk sec/Read") + require.False(t, ok) +} + func TestMetricTypeForCounterType(t *testing.T) { t.Parallel()