mlbiscoc commented on code in PR #3713:
URL: https://github.com/apache/solr/pull/3713#discussion_r2394957411


##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -17,157 +17,190 @@
 
 package org.apache.solr.client.solrj.impl;
 
-import java.util.ArrayList;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import org.apache.solr.client.solrj.SolrRequest.METHOD;
+import org.apache.solr.client.solrj.SolrRequest.SolrRequestType;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
 import org.apache.solr.client.solrj.response.SimpleSolrResponse;
 import org.apache.solr.common.SolrException;
-import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.StrUtils;
-import org.apache.solr.common.util.Utils;
 
 /**
  * This class is responsible for fetching metrics and other attributes from a 
given node in Solr
  * cluster. This is a helper class that is used by {@link 
SolrClientNodeStateProvider}
  */
-// NOCOMMIT: Need to removed hardcoded references to Dropwizard metrics for 
OTEL conversion, and
-// probably change enum structure to be more compatible with OTEL naming
 public class NodeValueFetcher {
   // well known tags
   public static final String NODE = "node";
   public static final String PORT = "port";
   public static final String HOST = "host";
   public static final String CORES = "cores";
   public static final String SYSPROP = "sysprop.";
-  public static final Set<String> tags =
-      Set.of(NODE, PORT, HOST, CORES, Tags.FREEDISK.tagName, 
Tags.HEAPUSAGE.tagName);
+  public static final Set<String> tags = Set.of(NODE, PORT, HOST, CORES);
   public static final Pattern hostAndPortPattern = 
Pattern.compile("(?:https?://)?([^:]+):(\\d+)");
   public static final String METRICS_PREFIX = "metrics:";
 
   /** Various well known tags that can be fetched from a node */
-  public enum Tags {
-    FREEDISK(
-        "freedisk", "solr.node", "CONTAINER.fs.usableSpace", 
"solr.node/CONTAINER.fs.usableSpace"),
-    TOTALDISK(
-        "totaldisk", "solr.node", "CONTAINER.fs.totalSpace", 
"solr.node/CONTAINER.fs.totalSpace"),
-    CORES("cores", "solr.node", "CONTAINER.cores", null) {
+  public enum Metrics {
+    FREEDISK("freedisk", "solr_cores_filesystem_disk_space", "type", 
"usable_space"),
+    TOTALDISK("totaldisk", "solr_cores_filesystem_disk_space", "type", 
"total_space"),
+    CORES("cores", "solr_cores_loaded") {
       @Override
-      public Object extractResult(Object root) {
-        NamedList<?> node = (NamedList<?>) Utils.getObjectByPath(root, false, 
"solr.node");
-        int count = 0;
-        for (String leafCoreMetricName : new String[] {"lazy", "loaded", 
"unloaded"}) {
-          Number n = (Number) node.get("CONTAINER.cores." + 
leafCoreMetricName);
-          if (n != null) count += n.intValue();
+      public Object extractResult(NamedList<Object> root) {
+        Object metrics = root.get("stream");
+        if (metrics == null || metricName == null) return null;
+
+        try (InputStream in = (InputStream) metrics) {
+          String[] lines = parsePrometheusOutput(in);
+          int count = 0;
+
+          for (String line : lines) {
+            if (shouldSkipPrometheusLine(line) || 
!line.startsWith(metricName)) continue;
+            count += (int) extractPrometheusValue(line);
+          }
+          return count;
+        } catch (Exception e) {
+          throw new SolrException(
+              SolrException.ErrorCode.SERVER_ERROR, "Unable to read prometheus 
metrics output", e);
         }
-        return count;
       }
     },
-    SYSLOADAVG("sysLoadAvg", "solr.jvm", "os.systemLoadAverage", 
"solr.jvm/os.systemLoadAverage"),
-    HEAPUSAGE("heapUsage", "solr.jvm", "memory.heap.usage", 
"solr.jvm/memory.heap.usage");
-    // the metrics group
-    public final String group;
-    // the metrics prefix
-    public final String prefix;
+    SYSLOADAVG("sysLoadAvg", "jvm_system_cpu_utilization_ratio");
+
     public final String tagName;
-    // the json path in the response
-    public final String path;
+    public final String metricName;
+    public final String labelKey;
+    public final String labelValue;
+
+    Metrics(String name, String metricName) {
+      this(name, metricName, null, null);
+    }
 
-    Tags(String name, String group, String prefix, String path) {
-      this.group = group;
-      this.prefix = prefix;
+    Metrics(String name, String metricName, String labelKey, String 
labelValue) {
       this.tagName = name;
-      this.path = path;
+      this.metricName = metricName;
+      this.labelKey = labelKey;
+      this.labelValue = labelValue;
     }
 
-    public Object extractResult(Object root) {
-      Object v = Utils.getObjectByPath(root, true, path);
-      return v == null ? null : convertVal(v);
+    public Object extractResult(NamedList<Object> root) {
+      return extractFromPrometheusResponse(root, metricName, labelKey, 
labelValue);
     }
 
-    public Object convertVal(Object val) {
-      if (val instanceof String) {
-        return Double.valueOf((String) val);
-      } else if (val instanceof Number num) {
-        return num.doubleValue();
+    /**
+     * Extract metric value from Prometheus response, optionally filtering by 
label. This
+     * consolidated method handles both labeled and unlabeled metrics.
+     */
+    private static Long extractFromPrometheusResponse(
+        NamedList<Object> root, String metricName, String labelKey, String 
labelValue) {
+      Object metrics = root.get("stream");
 
-      } else {
-        throw new IllegalArgumentException("Unknown type : " + val);
+      if (metrics == null || metricName == null) {
+        return null;
       }
-    }
-  }
 
-  /** Retrieve values that match JVM system properties and metrics. */
-  private void getRemotePropertiesAndMetrics(
-      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+      try (InputStream in = (InputStream) metrics) {
+        String[] lines = parsePrometheusOutput(in);
 
-    Map<String, Set<Object>> metricsKeyVsTag = new HashMap<>();
-    for (String tag : requestedTagNames) {
-      if (tag.startsWith(SYSPROP)) {
-        metricsKeyVsTag
-            .computeIfAbsent(
-                "solr.jvm:system.properties:" + 
tag.substring(SYSPROP.length()),
-                k -> new HashSet<>())
-            .add(tag);
-      } else if (tag.startsWith(METRICS_PREFIX)) {
-        metricsKeyVsTag
-            .computeIfAbsent(tag.substring(METRICS_PREFIX.length()), k -> new 
HashSet<>())
-            .add(tag);
+        for (String line : lines) {
+          if (shouldSkipPrometheusLine(line) || !line.startsWith(metricName)) 
continue;
+
+          // If metric with specific labels were requested, then return the 
metric with that label
+          // and skip others
+          if (labelKey != null && labelValue != null) {
+            String expectedLabel = labelKey + "=\"" + labelValue + "\"";
+            if (!line.contains(expectedLabel)) {
+              continue;
+            }
+          }
+
+          return extractPrometheusValue(line);
+        }
+      } catch (Exception e) {
+        throw new SolrException(
+            SolrException.ErrorCode.SERVER_ERROR, "Unable to read prometheus 
metrics output", e);
+      }
+
+      return null;
+    }
+
+    public static long extractPrometheusValue(String line) {

Review Comment:
   Yeah something called Open Metric format with exemplars and additional 
metadata. We default prometheus format and unless this calls with 
`wt=openmetrics` then it would fail. But maybe down the line Open Metrics 
format becomes the default. I tried chainging it up here to maybe be a bit 
better at just getting the value in case there is that additional metadata 
after or somehow it does get Open Metrics.



##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -17,157 +17,190 @@
 
 package org.apache.solr.client.solrj.impl;
 
-import java.util.ArrayList;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import org.apache.solr.client.solrj.SolrRequest.METHOD;
+import org.apache.solr.client.solrj.SolrRequest.SolrRequestType;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
 import org.apache.solr.client.solrj.response.SimpleSolrResponse;
 import org.apache.solr.common.SolrException;
-import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.StrUtils;
-import org.apache.solr.common.util.Utils;
 
 /**
  * This class is responsible for fetching metrics and other attributes from a 
given node in Solr
  * cluster. This is a helper class that is used by {@link 
SolrClientNodeStateProvider}
  */
-// NOCOMMIT: Need to removed hardcoded references to Dropwizard metrics for 
OTEL conversion, and
-// probably change enum structure to be more compatible with OTEL naming
 public class NodeValueFetcher {
   // well known tags
   public static final String NODE = "node";
   public static final String PORT = "port";
   public static final String HOST = "host";
   public static final String CORES = "cores";
   public static final String SYSPROP = "sysprop.";
-  public static final Set<String> tags =
-      Set.of(NODE, PORT, HOST, CORES, Tags.FREEDISK.tagName, 
Tags.HEAPUSAGE.tagName);
+  public static final Set<String> tags = Set.of(NODE, PORT, HOST, CORES);
   public static final Pattern hostAndPortPattern = 
Pattern.compile("(?:https?://)?([^:]+):(\\d+)");
   public static final String METRICS_PREFIX = "metrics:";
 
   /** Various well known tags that can be fetched from a node */
-  public enum Tags {
-    FREEDISK(
-        "freedisk", "solr.node", "CONTAINER.fs.usableSpace", 
"solr.node/CONTAINER.fs.usableSpace"),
-    TOTALDISK(
-        "totaldisk", "solr.node", "CONTAINER.fs.totalSpace", 
"solr.node/CONTAINER.fs.totalSpace"),
-    CORES("cores", "solr.node", "CONTAINER.cores", null) {
+  public enum Metrics {
+    FREEDISK("freedisk", "solr_cores_filesystem_disk_space", "type", 
"usable_space"),
+    TOTALDISK("totaldisk", "solr_cores_filesystem_disk_space", "type", 
"total_space"),
+    CORES("cores", "solr_cores_loaded") {
       @Override
-      public Object extractResult(Object root) {
-        NamedList<?> node = (NamedList<?>) Utils.getObjectByPath(root, false, 
"solr.node");
-        int count = 0;
-        for (String leafCoreMetricName : new String[] {"lazy", "loaded", 
"unloaded"}) {
-          Number n = (Number) node.get("CONTAINER.cores." + 
leafCoreMetricName);
-          if (n != null) count += n.intValue();
+      public Object extractResult(NamedList<Object> root) {
+        Object metrics = root.get("stream");
+        if (metrics == null || metricName == null) return null;
+
+        try (InputStream in = (InputStream) metrics) {
+          String[] lines = parsePrometheusOutput(in);
+          int count = 0;
+
+          for (String line : lines) {
+            if (shouldSkipPrometheusLine(line) || 
!line.startsWith(metricName)) continue;
+            count += (int) extractPrometheusValue(line);
+          }
+          return count;
+        } catch (Exception e) {
+          throw new SolrException(
+              SolrException.ErrorCode.SERVER_ERROR, "Unable to read prometheus 
metrics output", e);
         }
-        return count;
       }
     },
-    SYSLOADAVG("sysLoadAvg", "solr.jvm", "os.systemLoadAverage", 
"solr.jvm/os.systemLoadAverage"),
-    HEAPUSAGE("heapUsage", "solr.jvm", "memory.heap.usage", 
"solr.jvm/memory.heap.usage");
-    // the metrics group
-    public final String group;
-    // the metrics prefix
-    public final String prefix;
+    SYSLOADAVG("sysLoadAvg", "jvm_system_cpu_utilization_ratio");
+
     public final String tagName;
-    // the json path in the response
-    public final String path;
+    public final String metricName;
+    public final String labelKey;
+    public final String labelValue;
+
+    Metrics(String name, String metricName) {
+      this(name, metricName, null, null);
+    }
 
-    Tags(String name, String group, String prefix, String path) {
-      this.group = group;
-      this.prefix = prefix;
+    Metrics(String name, String metricName, String labelKey, String 
labelValue) {
       this.tagName = name;
-      this.path = path;
+      this.metricName = metricName;
+      this.labelKey = labelKey;
+      this.labelValue = labelValue;
     }
 
-    public Object extractResult(Object root) {
-      Object v = Utils.getObjectByPath(root, true, path);
-      return v == null ? null : convertVal(v);
+    public Object extractResult(NamedList<Object> root) {
+      return extractFromPrometheusResponse(root, metricName, labelKey, 
labelValue);
     }
 
-    public Object convertVal(Object val) {
-      if (val instanceof String) {
-        return Double.valueOf((String) val);
-      } else if (val instanceof Number num) {
-        return num.doubleValue();
+    /**
+     * Extract metric value from Prometheus response, optionally filtering by 
label. This
+     * consolidated method handles both labeled and unlabeled metrics.
+     */
+    private static Long extractFromPrometheusResponse(
+        NamedList<Object> root, String metricName, String labelKey, String 
labelValue) {
+      Object metrics = root.get("stream");
 
-      } else {
-        throw new IllegalArgumentException("Unknown type : " + val);
+      if (metrics == null || metricName == null) {
+        return null;
       }
-    }
-  }
 
-  /** Retrieve values that match JVM system properties and metrics. */
-  private void getRemotePropertiesAndMetrics(
-      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+      try (InputStream in = (InputStream) metrics) {
+        String[] lines = parsePrometheusOutput(in);
 
-    Map<String, Set<Object>> metricsKeyVsTag = new HashMap<>();
-    for (String tag : requestedTagNames) {
-      if (tag.startsWith(SYSPROP)) {
-        metricsKeyVsTag
-            .computeIfAbsent(
-                "solr.jvm:system.properties:" + 
tag.substring(SYSPROP.length()),
-                k -> new HashSet<>())
-            .add(tag);
-      } else if (tag.startsWith(METRICS_PREFIX)) {
-        metricsKeyVsTag
-            .computeIfAbsent(tag.substring(METRICS_PREFIX.length()), k -> new 
HashSet<>())
-            .add(tag);
+        for (String line : lines) {
+          if (shouldSkipPrometheusLine(line) || !line.startsWith(metricName)) 
continue;
+
+          // If metric with specific labels were requested, then return the 
metric with that label
+          // and skip others
+          if (labelKey != null && labelValue != null) {
+            String expectedLabel = labelKey + "=\"" + labelValue + "\"";
+            if (!line.contains(expectedLabel)) {
+              continue;
+            }
+          }
+
+          return extractPrometheusValue(line);
+        }
+      } catch (Exception e) {
+        throw new SolrException(
+            SolrException.ErrorCode.SERVER_ERROR, "Unable to read prometheus 
metrics output", e);
+      }
+
+      return null;
+    }
+
+    public static long extractPrometheusValue(String line) {
+      line = line.trim();
+      String actualValue;
+      if (line.contains("}")) {
+        actualValue = line.substring(line.lastIndexOf("} ") + 1);
+      } else {
+        actualValue = line.split(" ")[1];
       }
+      return (long) Double.parseDouble(actualValue);

Review Comment:
   Casted it higher up instead



##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -17,157 +17,190 @@
 
 package org.apache.solr.client.solrj.impl;
 
-import java.util.ArrayList;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
 import java.util.EnumSet;
-import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import org.apache.solr.client.solrj.SolrRequest.METHOD;
+import org.apache.solr.client.solrj.SolrRequest.SolrRequestType;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
 import org.apache.solr.client.solrj.response.SimpleSolrResponse;
 import org.apache.solr.common.SolrException;
-import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.StrUtils;
-import org.apache.solr.common.util.Utils;
 
 /**
  * This class is responsible for fetching metrics and other attributes from a 
given node in Solr
  * cluster. This is a helper class that is used by {@link 
SolrClientNodeStateProvider}
  */
-// NOCOMMIT: Need to removed hardcoded references to Dropwizard metrics for 
OTEL conversion, and
-// probably change enum structure to be more compatible with OTEL naming
 public class NodeValueFetcher {
   // well known tags
   public static final String NODE = "node";
   public static final String PORT = "port";
   public static final String HOST = "host";
   public static final String CORES = "cores";
   public static final String SYSPROP = "sysprop.";
-  public static final Set<String> tags =
-      Set.of(NODE, PORT, HOST, CORES, Tags.FREEDISK.tagName, 
Tags.HEAPUSAGE.tagName);
+  public static final Set<String> tags = Set.of(NODE, PORT, HOST, CORES);
   public static final Pattern hostAndPortPattern = 
Pattern.compile("(?:https?://)?([^:]+):(\\d+)");
   public static final String METRICS_PREFIX = "metrics:";
 
   /** Various well known tags that can be fetched from a node */
-  public enum Tags {
-    FREEDISK(
-        "freedisk", "solr.node", "CONTAINER.fs.usableSpace", 
"solr.node/CONTAINER.fs.usableSpace"),
-    TOTALDISK(
-        "totaldisk", "solr.node", "CONTAINER.fs.totalSpace", 
"solr.node/CONTAINER.fs.totalSpace"),
-    CORES("cores", "solr.node", "CONTAINER.cores", null) {
+  public enum Metrics {
+    FREEDISK("freedisk", "solr_cores_filesystem_disk_space", "type", 
"usable_space"),
+    TOTALDISK("totaldisk", "solr_cores_filesystem_disk_space", "type", 
"total_space"),
+    CORES("cores", "solr_cores_loaded") {
       @Override
-      public Object extractResult(Object root) {
-        NamedList<?> node = (NamedList<?>) Utils.getObjectByPath(root, false, 
"solr.node");
-        int count = 0;
-        for (String leafCoreMetricName : new String[] {"lazy", "loaded", 
"unloaded"}) {
-          Number n = (Number) node.get("CONTAINER.cores." + 
leafCoreMetricName);
-          if (n != null) count += n.intValue();
+      public Object extractResult(NamedList<Object> root) {
+        Object metrics = root.get("stream");
+        if (metrics == null || metricName == null) return null;
+
+        try (InputStream in = (InputStream) metrics) {
+          String[] lines = parsePrometheusOutput(in);
+          int count = 0;
+
+          for (String line : lines) {
+            if (shouldSkipPrometheusLine(line) || 
!line.startsWith(metricName)) continue;
+            count += (int) extractPrometheusValue(line);
+          }
+          return count;
+        } catch (Exception e) {
+          throw new SolrException(
+              SolrException.ErrorCode.SERVER_ERROR, "Unable to read prometheus 
metrics output", e);
         }
-        return count;
       }
     },
-    SYSLOADAVG("sysLoadAvg", "solr.jvm", "os.systemLoadAverage", 
"solr.jvm/os.systemLoadAverage"),
-    HEAPUSAGE("heapUsage", "solr.jvm", "memory.heap.usage", 
"solr.jvm/memory.heap.usage");
-    // the metrics group
-    public final String group;
-    // the metrics prefix
-    public final String prefix;
+    SYSLOADAVG("sysLoadAvg", "jvm_system_cpu_utilization_ratio");
+
     public final String tagName;
-    // the json path in the response
-    public final String path;
+    public final String metricName;
+    public final String labelKey;
+    public final String labelValue;
+
+    Metrics(String name, String metricName) {
+      this(name, metricName, null, null);
+    }
 
-    Tags(String name, String group, String prefix, String path) {
-      this.group = group;
-      this.prefix = prefix;
+    Metrics(String name, String metricName, String labelKey, String 
labelValue) {
       this.tagName = name;
-      this.path = path;
+      this.metricName = metricName;
+      this.labelKey = labelKey;
+      this.labelValue = labelValue;
     }
 
-    public Object extractResult(Object root) {
-      Object v = Utils.getObjectByPath(root, true, path);
-      return v == null ? null : convertVal(v);
+    public Object extractResult(NamedList<Object> root) {
+      return extractFromPrometheusResponse(root, metricName, labelKey, 
labelValue);
     }
 
-    public Object convertVal(Object val) {
-      if (val instanceof String) {
-        return Double.valueOf((String) val);
-      } else if (val instanceof Number num) {
-        return num.doubleValue();
+    /**
+     * Extract metric value from Prometheus response, optionally filtering by 
label. This
+     * consolidated method handles both labeled and unlabeled metrics.
+     */
+    private static Long extractFromPrometheusResponse(
+        NamedList<Object> root, String metricName, String labelKey, String 
labelValue) {
+      Object metrics = root.get("stream");
 
-      } else {
-        throw new IllegalArgumentException("Unknown type : " + val);
+      if (metrics == null || metricName == null) {
+        return null;
       }
-    }
-  }
 
-  /** Retrieve values that match JVM system properties and metrics. */
-  private void getRemotePropertiesAndMetrics(
-      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+      try (InputStream in = (InputStream) metrics) {
+        String[] lines = parsePrometheusOutput(in);
 
-    Map<String, Set<Object>> metricsKeyVsTag = new HashMap<>();
-    for (String tag : requestedTagNames) {
-      if (tag.startsWith(SYSPROP)) {
-        metricsKeyVsTag
-            .computeIfAbsent(
-                "solr.jvm:system.properties:" + 
tag.substring(SYSPROP.length()),
-                k -> new HashSet<>())
-            .add(tag);
-      } else if (tag.startsWith(METRICS_PREFIX)) {
-        metricsKeyVsTag
-            .computeIfAbsent(tag.substring(METRICS_PREFIX.length()), k -> new 
HashSet<>())
-            .add(tag);
+        for (String line : lines) {
+          if (shouldSkipPrometheusLine(line) || !line.startsWith(metricName)) 
continue;
+
+          // If metric with specific labels were requested, then return the 
metric with that label
+          // and skip others
+          if (labelKey != null && labelValue != null) {
+            String expectedLabel = labelKey + "=\"" + labelValue + "\"";
+            if (!line.contains(expectedLabel)) {
+              continue;
+            }
+          }
+
+          return extractPrometheusValue(line);
+        }
+      } catch (Exception e) {
+        throw new SolrException(
+            SolrException.ErrorCode.SERVER_ERROR, "Unable to read prometheus 
metrics output", e);
+      }
+
+      return null;
+    }
+
+    public static long extractPrometheusValue(String line) {
+      line = line.trim();
+      String actualValue;
+      if (line.contains("}")) {
+        actualValue = line.substring(line.lastIndexOf("} ") + 1);
+      } else {
+        actualValue = line.split(" ")[1];
       }
+      return (long) Double.parseDouble(actualValue);
     }
-    if (!metricsKeyVsTag.isEmpty()) {
-      SolrClientNodeStateProvider.fetchReplicaMetrics(ctx.getNode(), ctx, 
metricsKeyVsTag);
+
+    public static String[] parsePrometheusOutput(InputStream inputStream) 
throws Exception {
+      String output = new String(inputStream.readAllBytes(), 
StandardCharsets.UTF_8);
+      return output.split("\n");

Review Comment:
   Yeah you're right, this is very inefficient. Rewrote with streams which look 
a bit cleaner as well.



##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java:
##########
@@ -138,74 +135,95 @@ public Map<String, Map<String, List<Replica>>> 
getReplicaInfo(
       String node, Collection<String> keys) {
     Map<String, Map<String, List<Replica>>> result =
         nodeVsCollectionVsShardVsReplicaInfo.computeIfAbsent(node, o -> new 
HashMap<>());
-    if (!keys.isEmpty()) {
-      Map<String, Pair<String, Replica>> metricsKeyVsTagReplica = new 
HashMap<>();
-      forEachReplica(
-          result,
-          r -> {
-            for (String key : keys) {
-              if (r.getProperties().containsKey(key)) continue; // it's 
already collected
-              String perReplicaMetricsKey =
-                  "solr.core."
-                      + r.getCollection()
-                      + "."
-                      + r.getShard()
-                      + "."
-                      + Utils.parseMetricsReplicaName(r.getCollection(), 
r.getCoreName())
-                      + ":";
-              String perReplicaValue = key;
-              perReplicaMetricsKey += perReplicaValue;
-              metricsKeyVsTagReplica.put(perReplicaMetricsKey, new Pair<>(key, 
r));
-            }
-          });
-
-      if (!metricsKeyVsTagReplica.isEmpty()) {
-        Map<String, Object> tagValues = fetchReplicaMetrics(node, 
metricsKeyVsTagReplica);
-        tagValues.forEach(
-            (k, o) -> {
-              Pair<String, Replica> p = metricsKeyVsTagReplica.get(k);
-              if (p.second() != null) 
p.second().getProperties().put(p.first(), o);
-            });
+
+    if (keys.isEmpty()) {
+      return result;
+    }
+
+    // Build mapping from core name to (replica, metric) {coreName: <Replica, 
Prometheus Metric
+    // Name>}
+    Map<String, List<Pair<Replica, String>>> coreToReplicaProps = new 
HashMap<>();
+    Set<String> requestedMetricNames = new HashSet<>();
+
+    forEachReplica(
+        result,
+        replica -> {
+          for (String key : keys) {
+            if (replica.getProperties().containsKey(key)) continue;
+
+            // Build core name as the key to the replica and the metric it 
needs
+            String coreName =
+                replica.getCollection()
+                    + "_"
+                    + replica.getShard()
+                    + "_"
+                    + Utils.parseMetricsReplicaName(replica.getCollection(), 
replica.getCoreName());
+
+            coreToReplicaProps
+                .computeIfAbsent(coreName, k -> new ArrayList<>())
+                .add(new Pair<>(replica, key));
+            requestedMetricNames.add(key);
+          }
+        });
+
+    if (coreToReplicaProps.isEmpty()) {
+      return result;
+    }
+
+    RemoteCallCtx ctx = new RemoteCallCtx(node, solrClient);
+    String[] lines = fetchBatchedMetric(node, ctx, requestedMetricNames);
+
+    // Parse through prometheus metrics and map the metric with its 
corresponding replica properties
+    for (String line : lines) {
+      if (NodeValueFetcher.shouldSkipPrometheusLine(line)) continue;
+
+      String prometheusMetricName = 
NodeValueFetcher.extractMetricNameFromLine(line);
+
+      // Extract core name from prometheus line and the core label
+      String coreParam = NodeValueFetcher.extractLabelValueFromLine(line, 
"core");
+      if (coreParam == null) continue;
+
+      // Find the matching core and set the metric value to its corresponding 
replica properties
+      List<Pair<Replica, String>> replicaProps = 
coreToReplicaProps.get(coreParam);
+      if (replicaProps != null) {
+        Long value = NodeValueFetcher.Metrics.extractPrometheusValue(line);
+        replicaProps.stream()
+            .filter(pair -> pair.second().equals(prometheusMetricName))
+            .forEach(pair -> pair.first().getProperties().put(pair.second(), 
value));
       }
     }
+
     return result;
   }
 
-  protected Map<String, Object> fetchReplicaMetrics(
-      String node, Map<String, Pair<String, Replica>> metricsKeyVsTagReplica) {
-    Map<String, Set<Object>> collect =
-        metricsKeyVsTagReplica.entrySet().stream()
-            .collect(Collectors.toMap(e -> e.getKey(), e -> 
Set.of(e.getKey())));
-    RemoteCallCtx ctx = new RemoteCallCtx(null, solrClient);
-    fetchReplicaMetrics(node, ctx, collect);
-    return ctx.tags;
-  }
+  static String[] fetchBatchedMetric(String solrNode, RemoteCallCtx ctx, 
Set<String> metricNames) {
+    if (!ctx.isNodeAlive(solrNode))
+      throw new SolrException(
+          SolrException.ErrorCode.SERVER_ERROR,
+          "Solr node is not healthy. Cannot request metrics: " + solrNode);
 
-  // NOCOMMIT: We need to change the /admin/metrics call here to work with
-  // Prometheus/OTEL telemetry
-  static void fetchReplicaMetrics(
-      String solrNode, RemoteCallCtx ctx, Map<String, Set<Object>> 
metricsKeyVsTag) {
-    if (!ctx.isNodeAlive(solrNode)) return;
     ModifiableSolrParams params = new ModifiableSolrParams();
-    params.add("key", metricsKeyVsTag.keySet().toArray(new String[0]));
-    try {
-      SimpleSolrResponse rsp = ctx.invokeWithRetry(solrNode, 
CommonParams.METRICS_PATH, params);
-      metricsKeyVsTag.forEach(
-          (key, tags) -> {
-            Object v =
-                Utils.getObjectByPath(rsp.getResponse(), true, 
Arrays.asList("metrics", key));
-            for (Object tag : tags) {
-              if (tag instanceof Function) {
-                @SuppressWarnings({"unchecked"})
-                Pair<String, Object> p = (Pair<String, Object>) ((Function) 
tag).apply(v);
-                ctx.tags.put(p.first(), p.second());
-              } else {
-                if (v != null) ctx.tags.put(tag.toString(), v);
-              }
-            }
-          });
+    params.add("wt", "prometheus");
+    params.add("name", String.join(",", metricNames));
+
+    var req =
+        new GenericSolrRequest(
+            SolrRequest.METHOD.GET, "/admin/metrics", 
SolrRequest.SolrRequestType.ADMIN, params);
+    req.setResponseParser(new InputStreamResponseParser("prometheus"));
+
+    String baseUrl =
+        
ctx.zkClientClusterStateProvider.getZkStateReader().getBaseUrlForNodeName(solrNode);
+    try (InputStream in =
+        (InputStream)
+            ctx.cloudSolrClient
+                .getHttpClient()
+                .requestWithBaseUrl(baseUrl, req::process)
+                .getResponse()
+                .get("stream")) {
+      return NodeValueFetcher.Metrics.parsePrometheusOutput(in);
     } catch (Exception e) {
-      log.warn("could not get tags from node {}", solrNode, e);
+      throw new SolrException(

Review Comment:
   Are you saying we don't want to exception catch here? It was here before so 
kind of just kept. There is an `isNodeAlive` check at the top that I threw an 
exception but shouldn't have so I changed that in the latest commit.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to