code review

Signed-off-by: Jay DeLuca <jaydeluca4@gmail.com>

Author: jaydeluca

prometheus-metrics-exposition-formats/src/test/java/io/prometheus/metrics/expositionformats/DuplicateNamesProtobufTest.java

@@ -41,7 +41,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 
@@ -63,7 +63,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
     return registry;
@@ -140,7 +140,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 
@@ -168,7 +168,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 
@@ -258,7 +258,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "http_requests_total";
+            return "http_requests";
           }
         });
 

prometheus-metrics-exposition-textformats/src/main/java/io/prometheus/metrics/expositionformats/TextFormatUtil.java

@@ -21,6 +21,7 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import javax.annotation.Nullable;
 
 /**
@@ -235,6 +236,21 @@ private static MetricSnapshot mergeSnapshots(List<MetricSnapshot> snapshots) {
               "Cannot merge histograms: gauge histogram and classic histogram");
         }
       }
+      // Validate metadata consistency so we don't silently pick one help/unit when they differ.
+      if (!Objects.equals(
+          first.getMetadata().getPrometheusName(), snapshot.getMetadata().getPrometheusName())) {
+        throw new IllegalArgumentException("Cannot merge snapshots: inconsistent metric name");
+      }
+      if (!Objects.equals(first.getMetadata().getHelp(), snapshot.getMetadata().getHelp())) {
+        throw new IllegalArgumentException(
+            "Cannot merge snapshots: conflicting help for metric "
+                + first.getMetadata().getPrometheusName());
+      }
+      if (!Objects.equals(first.getMetadata().getUnit(), snapshot.getMetadata().getUnit())) {
+        throw new IllegalArgumentException(
+            "Cannot merge snapshots: conflicting unit for metric "
+                + first.getMetadata().getPrometheusName());
+      }
       totalDataPoints += snapshot.getDataPoints().size();
     }
 

prometheus-metrics-exposition-textformats/src/test/java/io/prometheus/metrics/expositionformats/DuplicateNamesExpositionTest.java

@@ -35,7 +35,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 
@@ -57,7 +57,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
     return registry;
@@ -110,7 +110,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 
@@ -138,7 +138,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 
@@ -205,7 +205,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 
@@ -228,7 +228,7 @@ public MetricSnapshot collect() {
 
           @Override
           public String getPrometheusName() {
-            return "api_responses_total";
+            return "api_responses";
           }
         });
 

prometheus-metrics-model/src/main/java/io/prometheus/metrics/model/registry/PrometheusRegistry.java

@@ -18,7 +18,6 @@ public class PrometheusRegistry {
 
   public static final PrometheusRegistry defaultRegistry = new PrometheusRegistry();
 
-  private final Set<String> prometheusNames = ConcurrentHashMap.newKeySet();
   private final Set<Collector> collectors = ConcurrentHashMap.newKeySet();
   private final Set<MultiCollector> multiCollectors = ConcurrentHashMap.newKeySet();
   private final ConcurrentHashMap<String, RegistrationInfo> registered = new ConcurrentHashMap<>();
@@ -187,22 +186,19 @@ public void register(Collector collector) {
                           + ", new: "
                           + type);
                 }
-                existingInfo.validateMetadata(helpForValidation, unitForValidation);
+                // Check label set first; only mutate help/unit after validation passes.
                 if (!existingInfo.addLabelSet(names)) {
                   throw new IllegalArgumentException(
                       name + ": duplicate metric name with identical label schema " + names);
                 }
+                existingInfo.validateMetadata(helpForValidation, unitForValidation);
                 return existingInfo;
               }
             });
 
         collectorMetadata.put(
             collector, new CollectorRegistration(prometheusName, normalizedLabels));
       }
-
-      if (prometheusName != null) {
-        prometheusNames.add(prometheusName);
-      }
     } catch (Exception e) {
       collectors.remove(collector);
       CollectorRegistration reg = collectorMetadata.remove(collector);
@@ -219,7 +215,6 @@ public void register(MultiCollector collector) {
     }
     List<String> prometheusNamesList = collector.getPrometheusNames();
     List<MultiCollectorRegistration> registrations = new ArrayList<>();
-    Set<String> namesOnlyInPrometheusNames = new HashSet<>();
 
     try {
       for (String prometheusName : prometheusNamesList) {
@@ -249,24 +244,20 @@ public void register(MultiCollector collector) {
                             + ", new: "
                             + type);
                   }
-                  existingInfo.validateMetadata(helpForValidation, unitForValidation);
+                  // Check label set first; only mutate help/unit after validation passes.
                   if (!existingInfo.addLabelSet(labelNamesForValidation)) {
                     throw new IllegalArgumentException(
                         prometheusName
                             + ": duplicate metric name with identical label schema "
                             + labelNamesForValidation);
                   }
+                  existingInfo.validateMetadata(helpForValidation, unitForValidation);
                   return existingInfo;
                 }
               });
 
           registrations.add(new MultiCollectorRegistration(prometheusName, normalizedLabels));
         }
-
-        boolean addedToPrometheusNames = prometheusNames.add(prometheusName);
-        if (addedToPrometheusNames && metricType == null) {
-          namesOnlyInPrometheusNames.add(prometheusName);
-        }
       }
 
       multiCollectorMetadata.put(collector, registrations);
@@ -275,9 +266,6 @@ public void register(MultiCollector collector) {
       for (MultiCollectorRegistration registration : registrations) {
         unregisterLabelSchema(registration.prometheusName, registration.labelNames);
       }
-      for (String name : namesOnlyInPrometheusNames) {
-        prometheusNames.remove(name);
-      }
       throw e;
     }
   }
@@ -303,27 +291,24 @@ public void unregister(MultiCollector collector) {
   }
 
   /**
-   * Decrements the reference count for a label schema and removes the metric name entirely if no
-   * schemas remain.
+   * Removes the label schema for the given metric name. If no label schemas remain for that name,
+   * removes the metric name entirely from the registry.
    */
   private void unregisterLabelSchema(String prometheusName, Set<String> labelNames) {
     registered.computeIfPresent(
         prometheusName,
         (name, info) -> {
           info.removeLabelSet(labelNames);
           if (info.isEmpty()) {
-            // No more label schemas for this name, remove it entirely
-            prometheusNames.remove(prometheusName);
             return null; // remove from registered map
           }
-          return info; // keep the RegistrationInfo, just with decremented count
+          return info;
         });
   }
 
   public void clear() {
     collectors.clear();
     multiCollectors.clear();
-    prometheusNames.clear();
     registered.clear();
     collectorMetadata.clear();
     multiCollectorMetadata.clear();