From d85cbaa17ca78a5d1076c6fcf5a811a1b2c690bc Mon Sep 17 00:00:00 2001
From: Benjamin Drung <benjamin.drung@ionos.com>
Date: Mon, 15 Nov 2021 11:22:36 +0100
Subject: [PATCH] ethtool: Prevent duplicate metric names (#2187)

Sanitizing the metric names can lead to duplicate metric names:

```
caller=level.go:63 level=error caller="error gathering metrics: [from Gatherer #2] collected metric \"node_ethtool_giant_hdr\" { label:<name:\"device\" value:\"ens192\" > untyped:<value:0" msg=" > } was collected before with the same name and label values"
```

Generate a map from the sanitized metric names to the metric names from
ethtool. In case of duplicate sanitized metric names drop both metrics,
because it is unknown which one to take.

Fixes: https://github.com/prometheus/node_exporter/issues/2185
Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
---
 collector/ethtool_linux.go                 | 30 ++++++++++++++++++----
 collector/fixtures/ethtool/eth0/statistics |  2 ++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/collector/ethtool_linux.go b/collector/ethtool_linux.go
index 233cbd8f..c68b3e9f 100644
--- a/collector/ethtool_linux.go
+++ b/collector/ethtool_linux.go
@@ -385,19 +385,39 @@ func (c *ethtoolCollector) Update(ch chan<- prometheus.Metric) error {
 			continue
 		}
 
+		// Sanitizing the metric names can lead to duplicate metric names. Therefore check for clashes beforehand.
+		metricFQNames := make(map[string]string)
+		for metric := range stats {
+			if !c.metricsPattern.MatchString(metric) {
+				continue
+			}
+			metricFQName := buildEthtoolFQName(metric)
+			existingMetric, exists := metricFQNames[metricFQName]
+			if exists {
+				level.Debug(c.logger).Log("msg", "dropping duplicate metric name", "device", device,
+					"metricFQName", metricFQName, "metric1", existingMetric, "metric2", metric)
+				// Keep the metric as "deleted" in the dict in case there are 3 duplicates.
+				metricFQNames[metricFQName] = ""
+			} else {
+				metricFQNames[metricFQName] = metric
+			}
+		}
+
 		// Sort metric names so that the test fixtures will match up
-		keys := make([]string, 0, len(stats))
-		for k := range stats {
+		keys := make([]string, 0, len(metricFQNames))
+		for k := range metricFQNames {
 			keys = append(keys, k)
 		}
 		sort.Strings(keys)
 
-		for _, metric := range keys {
-			if !c.metricsPattern.MatchString(metric) {
+		for _, metricFQName := range keys {
+			metric := metricFQNames[metricFQName]
+			if metric == "" {
+				// Skip the "deleted" duplicate metrics
 				continue
 			}
+
 			val := stats[metric]
-			metricFQName := buildEthtoolFQName(metric)
 
 			// Check to see if this metric exists; if not then create it and store it in c.entries.
 			entry, exists := c.entries[metric]
diff --git a/collector/fixtures/ethtool/eth0/statistics b/collector/fixtures/ethtool/eth0/statistics
index 9ce7e015..80423bd6 100644
--- a/collector/fixtures/ethtool/eth0/statistics
+++ b/collector/fixtures/ethtool/eth0/statistics
@@ -13,3 +13,5 @@ NIC statistics:
      rx_multicast: 23973
      tx_aborted: 0
      tx_underrun: 0
+     duplicate metric: 1
+     duplicate_metric: 2