From f030da9ec2d3c0d66a952262942d6b2517378331 Mon Sep 17 00:00:00 2001 From: Alexandre Beaulieu Date: Mon, 15 Apr 2024 09:55:36 -0400 Subject: [PATCH 1/7] [receiver/windowsperfcounters] fix: Drop metrics with empty datapoints. --- .../windowsperfcounters_scraper.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go index 1155a645796e8..ffaf9a9657893 100644 --- a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go +++ b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go @@ -148,9 +148,21 @@ func (s *scraper) scrape(context.Context) (pmetric.Metrics, error) { initializeMetricDps(metric, now, val, watcher.MetricRep.Attributes) } } + + // Drop metrics with no datapoints. This happens when configured counters don't exist on the host. + // This may result in a Metrics message with no metrics if all counters are missing. + metricSlice.RemoveIf(func(m pmetric.Metric) bool { + if m.Type() == pmetric.MetricTypeGauge { + return m.Gauge().DataPoints().Len() == 0 + } else { + return m.Sum().DataPoints().Len() == 0 + } + }) + if scrapeFailures != 0 && scrapeFailures != len(s.watchers) { errs = scrapererror.NewPartialScrapeError(errs, scrapeFailures) } + return md, errs } From e51736b571b3f87f3ac6677877401ed3ed95105f Mon Sep 17 00:00:00 2001 From: Alexandre Beaulieu Date: Thu, 18 Apr 2024 10:05:07 -0400 Subject: [PATCH 2/7] [receiver/windowsperfcounters] fix(tests): avoid panic in create receiver test. --- receiver/windowsperfcountersreceiver/factory_windows.go | 1 + 1 file changed, 1 insertion(+) diff --git a/receiver/windowsperfcountersreceiver/factory_windows.go b/receiver/windowsperfcountersreceiver/factory_windows.go index 10eaba63c3eae..207fd71f97343 100644 --- a/receiver/windowsperfcountersreceiver/factory_windows.go +++ b/receiver/windowsperfcountersreceiver/factory_windows.go @@ -8,6 +8,7 @@ package windowsperfcountersreceiver // import "github.com/open-telemetry/opentel import ( "context" + "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/windowsperfcountersreceiver/internal/metadata" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/receiver" From e5e60240b6d2ccdf3e173aaa28a95b7f29fcdaac Mon Sep 17 00:00:00 2001 From: Alexandre Beaulieu Date: Thu, 18 Apr 2024 12:59:31 -0400 Subject: [PATCH 3/7] [receiver/windowsperfcounters] fix(tests): implement validation --- .../testdata/scraper/metric_not_scraped.yaml | 14 ++++++ .../testdata/scraper/no_metric_def.yaml | 1 + .../testdata/scraper/standard.yaml | 48 ------------------- .../testdata/scraper/sum_metric.yaml | 1 + .../windowsperfcounters_scraper_test.go | 36 ++++++++++++-- 5 files changed, 49 insertions(+), 51 deletions(-) create mode 100644 receiver/windowsperfcountersreceiver/testdata/scraper/metric_not_scraped.yaml diff --git a/receiver/windowsperfcountersreceiver/testdata/scraper/metric_not_scraped.yaml b/receiver/windowsperfcountersreceiver/testdata/scraper/metric_not_scraped.yaml new file mode 100644 index 0000000000000..5137373a35edd --- /dev/null +++ b/receiver/windowsperfcountersreceiver/testdata/scraper/metric_not_scraped.yaml @@ -0,0 +1,14 @@ +resourceMetrics: + - resource: {} + scopeMetrics: + - metrics: + - description: percentage of time CPU is idle. + gauge: + dataPoints: + - asDouble: 0 + timeUnixNano: "1646857199239674900" + name: cpu.idle + unit: '%' + # Should not be present in the scrape output. + # - name: no.counter + scope: {} diff --git a/receiver/windowsperfcountersreceiver/testdata/scraper/no_metric_def.yaml b/receiver/windowsperfcountersreceiver/testdata/scraper/no_metric_def.yaml index 534e3f7003ada..df2422d670e3e 100644 --- a/receiver/windowsperfcountersreceiver/testdata/scraper/no_metric_def.yaml +++ b/receiver/windowsperfcountersreceiver/testdata/scraper/no_metric_def.yaml @@ -7,4 +7,5 @@ resourceMetrics: - asInt: "25089622016" timeUnixNano: "1647459021285009300" name: \Memory\Committed Bytes + unit: '1' scope: {} diff --git a/receiver/windowsperfcountersreceiver/testdata/scraper/standard.yaml b/receiver/windowsperfcountersreceiver/testdata/scraper/standard.yaml index 24d7f3d2b9f9b..520d6d5b189f4 100644 --- a/receiver/windowsperfcountersreceiver/testdata/scraper/standard.yaml +++ b/receiver/windowsperfcountersreceiver/testdata/scraper/standard.yaml @@ -11,48 +11,6 @@ resourceMetrics: value: stringValue: "0" timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "1" - timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "2" - timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "3" - timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "4" - timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "5" - timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "6" - timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "7" - timeUnixNano: "1646857199239674900" name: cpu.idle unit: '%' - description: number of bytes committed to memory @@ -71,12 +29,6 @@ resourceMetrics: value: stringValue: "1" timeUnixNano: "1646857199239674900" - - asDouble: 0 - attributes: - - key: instance - value: - stringValue: "2" - timeUnixNano: "1646857199239674900" name: processor.time unit: '%' scope: {} diff --git a/receiver/windowsperfcountersreceiver/testdata/scraper/sum_metric.yaml b/receiver/windowsperfcountersreceiver/testdata/scraper/sum_metric.yaml index 293f51c1b8f4f..f297d0e00af88 100644 --- a/receiver/windowsperfcountersreceiver/testdata/scraper/sum_metric.yaml +++ b/receiver/windowsperfcountersreceiver/testdata/scraper/sum_metric.yaml @@ -6,6 +6,7 @@ resourceMetrics: name: bytes.committed sum: aggregationTemporality: 2 + isMonotonic: true dataPoints: - asDouble: 1.94461696e+10 timeUnixNano: "1646862225775600200" diff --git a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go index fd6bf52f07823..b93d96b0f0939 100644 --- a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go +++ b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go @@ -112,7 +112,7 @@ func Test_WindowsPerfCounterScraper(t *testing.T) { "bytes.committed": { Description: "number of bytes committed to memory", Unit: "By", - Sum: SumMetric{}, + Sum: SumMetric{Aggregation: "cumulative", Monotonic: true}, }, }, PerfCounters: []ObjectConfig{ @@ -150,6 +150,28 @@ func Test_WindowsPerfCounterScraper(t *testing.T) { startMessage: "some performance counters could not be initialized", startErr: "failed to create perf counter with path \\Invalid Object\\Invalid Counter: The specified object was not found on the computer.\r\n", }, + { + name: "MetricDefinedButNoScrapedValue", + cfg: &Config{ + MetricMetaData: map[string]MetricConfig{ + "cpu.idle": { + Description: "percentage of time CPU is idle.", + Unit: "%", + Gauge: GaugeMetric{}, + }, + "no.counter": { + Description: "there is no counter or data for this metric", + Unit: "By", + Gauge: GaugeMetric{}, + }, + }, + PerfCounters: []ObjectConfig{ + {Object: "Processor", Instances: []string{"_Total"}, Counters: []CounterConfig{{Name: "% Idle Time", MetricRep: MetricRep{Name: "cpu.idle"}}}}, + }, + ControllerConfig: scraperhelper.ControllerConfig{CollectionInterval: time.Minute, InitialDelay: time.Second}, + }, + expectedMetricPath: filepath.Join("testdata", "scraper", "metric_not_scraped.yaml"), + }, } for _, test := range testCases { @@ -186,8 +208,16 @@ func Test_WindowsPerfCounterScraper(t *testing.T) { expectedMetrics, err := golden.ReadMetrics(test.expectedMetricPath) require.NoError(t, err) - // TODO: Metrics comparison is failing, not verifying the result until that is fixed. - _ = pmetrictest.CompareMetrics(expectedMetrics, actualMetrics, pmetrictest.IgnoreMetricValues()) + require.NoError(t, pmetrictest.CompareMetrics(expectedMetrics, actualMetrics, + // Scraping test host means static values, timestamps and instance counts are unreliable. ScopeMetrics order is also unpredictable. + // The check only takes the first instance of multi-instance counters and assumes that the other instances would be included. + pmetrictest.IgnoreSubsequentDataPoints("cpu.idle"), + pmetrictest.IgnoreSubsequentDataPoints("processor.time"), + pmetrictest.IgnoreScopeMetricsOrder(), + pmetrictest.IgnoreResourceMetricsOrder(), + pmetrictest.IgnoreMetricValues(), + pmetrictest.IgnoreTimestamp(), + )) }) } } From 2abd91dbab1751e409b56aea46f046720c75e6d4 Mon Sep 17 00:00:00 2001 From: Alexandre Beaulieu Date: Thu, 18 Apr 2024 14:09:26 -0400 Subject: [PATCH 4/7] [receiver/windowsperfcounters] chore: add changlog entry. --- .chloggen/windowsperfcounters-empty-datapoints.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .chloggen/windowsperfcounters-empty-datapoints.yaml diff --git a/.chloggen/windowsperfcounters-empty-datapoints.yaml b/.chloggen/windowsperfcounters-empty-datapoints.yaml new file mode 100644 index 0000000000000..fbbbf0c811faa --- /dev/null +++ b/.chloggen/windowsperfcounters-empty-datapoints.yaml @@ -0,0 +1,6 @@ +change_type: bug_fix +component: windowsperfcountersreceiver +note: Metric definitions with no matching performance counter are no longer included as metrics with zero datapoints in the scrape output. +issues: [4972] +subtext: +change_logs: [user] \ No newline at end of file From 428840de372cb6d0059980782b1792e7bf74c78a Mon Sep 17 00:00:00 2001 From: Alexandre Beaulieu Date: Thu, 18 Apr 2024 14:33:08 -0400 Subject: [PATCH 5/7] fix: auto-merge mishap when rebasing branch --- receiver/windowsperfcountersreceiver/factory_windows.go | 1 - 1 file changed, 1 deletion(-) diff --git a/receiver/windowsperfcountersreceiver/factory_windows.go b/receiver/windowsperfcountersreceiver/factory_windows.go index 207fd71f97343..10eaba63c3eae 100644 --- a/receiver/windowsperfcountersreceiver/factory_windows.go +++ b/receiver/windowsperfcountersreceiver/factory_windows.go @@ -8,7 +8,6 @@ package windowsperfcountersreceiver // import "github.com/open-telemetry/opentel import ( "context" - "github.com/open-telemetry/opentelemetry-collector-contrib/receiver/windowsperfcountersreceiver/internal/metadata" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/consumer" "go.opentelemetry.io/collector/receiver" From 0b1408518642acf8d90a0487cb6954cd12d87f7e Mon Sep 17 00:00:00 2001 From: Alexandre Beaulieu Date: Thu, 2 May 2024 08:42:53 -0400 Subject: [PATCH 6/7] fix: broken unit test after rebase --- .../windowsperfcounters_scraper_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go index b93d96b0f0939..f6ae51c77b0b7 100644 --- a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go +++ b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper_test.go @@ -455,10 +455,21 @@ func TestScrape(t *testing.T) { metrics.Sort(func(a, b pmetric.Metric) bool { return a.Name() < b.Name() }) + + assert.Equal(t, len(test.mockPerfCounters)-len(expectedErrors), metrics.Len()) + curMetricsNum := 0 for _, pc := range test.cfg.PerfCounters { for counterIdx, counterCfg := range pc.Counters { + counterValues := test.mockPerfCounters[counterIdx].counterValues + scrapeErr := test.mockPerfCounters[counterIdx].scrapeErr + + if scrapeErr != nil { + require.Empty(t, counterValues, "Invalid test case. Scrape error and counter values simultaneously.") + continue // no data for this counter. + } + metric := metrics.At(curMetricsNum) assert.Equal(t, counterCfg.MetricRep.Name, metric.Name()) metricData := test.cfg.MetricMetaData[counterCfg.MetricRep.Name] @@ -466,7 +477,6 @@ func TestScrape(t *testing.T) { assert.Equal(t, metricData.Unit, metric.Unit()) dps := metric.Gauge().DataPoints() - counterValues := test.mockPerfCounters[counterIdx].counterValues assert.Equal(t, len(counterValues), dps.Len()) for dpIdx, val := range counterValues { assert.Equal(t, val.Value, dps.At(dpIdx).DoubleValue()) From 667cf55f81a26812d1ac90c5b7b176d9dac213da Mon Sep 17 00:00:00 2001 From: Alexandre Beaulieu Date: Thu, 4 Jul 2024 07:47:59 -0400 Subject: [PATCH 7/7] code review --- .../windowsperfcounters_scraper.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go index 16b4b75b652fa..3162c7a8ed30f 100644 --- a/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go +++ b/receiver/windowsperfcountersreceiver/windowsperfcounters_scraper.go @@ -161,10 +161,13 @@ func (s *scraper) scrape(context.Context) (pmetric.Metrics, error) { // Drop metrics with no datapoints. This happens when configured counters don't exist on the host. // This may result in a Metrics message with no metrics if all counters are missing. metricSlice.RemoveIf(func(m pmetric.Metric) bool { - if m.Type() == pmetric.MetricTypeGauge { + switch m.Type() { + case pmetric.MetricTypeGauge: return m.Gauge().DataPoints().Len() == 0 - } else { + case pmetric.MetricTypeSum: return m.Sum().DataPoints().Len() == 0 + default: + return false } })