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


##########
solr/core/src/java/org/apache/solr/cluster/placement/impl/MetricImpl.java:
##########
@@ -120,22 +158,27 @@ public boolean equals(Object o) {
     }
     return name.equals(that.getName())
         && internalName.equals(that.getInternalName())
-        && converter.equals(that.converter);
+        && converter.equals(that.converter)

Review Comment:
   There's a subtle problem here I've encountered in the past.  It's 
problematic to involve a field in Object.equals/hashCode when that field is 
likely to be implemented by a lambda.  Instances of a lambda will never return 
`true` from `Object.equals` as they are always separate instances, even if, 
say, two point to the same method reference.



##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -185,10 +218,153 @@ public void getTags(Set<String> requestedTags, 
SolrClientNodeStateProvider.Remot
         return;
       }
 
-      getRemotePropertiesAndMetrics(requestedTags, ctx);
-      getRemoteTags(requestedTags, ctx);
+      // Categorize requested system properties or metrics
+      requestedTags.forEach(
+          tag -> {
+            if (tag.startsWith(SYSPROP)) {
+              requestsProperties.add(tag);
+            } else if (tag.startsWith(METRICS_PREFIX)) {
+              requestedMetrics.add(tag);
+            } else {
+              requestedMetricTags.add(tag);
+            }
+          });
+
+      getRemoteSystemProps(requestsProperties, ctx);
+      getRemoteMetrics(requestedMetrics, ctx);
+      getRemoteMetricsFromTags(requestedMetricTags, ctx);

Review Comment:
   Ideally we would get these things in parallel... but its maybe fine for now



##########
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:
   it's hard to tell in GitHub but if I recall the metrics fetching was 
somewhat resilient to at least some types of errors(?).  At least, if we can't 
reach a host at all, it is probably not a failure situation.



##########
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:
   Can we be sure there will be no extra info following the metric number?  You 
showed us recently something I hadn't seen before in which there was something 
afterwards.  I forget what it was called... a sample or something.



##########
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:
   Should we be doing the long conversion _here_?



##########
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");

Review Comment:
   I assume, refer to InputStreamResponseParser constant



##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -185,10 +218,153 @@ public void getTags(Set<String> requestedTags, 
SolrClientNodeStateProvider.Remot
         return;
       }
 
-      getRemotePropertiesAndMetrics(requestedTags, ctx);
-      getRemoteTags(requestedTags, ctx);
+      // Categorize requested system properties or metrics
+      requestedTags.forEach(
+          tag -> {
+            if (tag.startsWith(SYSPROP)) {
+              requestsProperties.add(tag);
+            } else if (tag.startsWith(METRICS_PREFIX)) {
+              requestedMetrics.add(tag);
+            } else {
+              requestedMetricTags.add(tag);
+            }
+          });
+
+      getRemoteSystemProps(requestsProperties, ctx);
+      getRemoteMetrics(requestedMetrics, ctx);
+      getRemoteMetricsFromTags(requestedMetricTags, ctx);
+
     } catch (Exception e) {
       throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
     }
   }
+
+  private void getRemoteSystemProps(
+      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+    ModifiableSolrParams params = new ModifiableSolrParams();
+    try {
+      SimpleSolrResponse rsp = ctx.invokeWithRetry(ctx.getNode(), 
"/admin/info/properties", params);
+      NamedList<?> systemPropsRsp = (NamedList<?>) 
rsp.getResponse().get("system.properties");
+      for (String requestedProperty : requestedTagNames) {
+        Object property = 
systemPropsRsp.get(requestedProperty.substring(SYSPROP.length()));
+        if (property != null) ctx.tags.put(requestedProperty, 
property.toString());
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error 
getting remote info", e);
+    }
+  }
+
+  /**
+   * Retrieve values that match metrics. Metrics names are structured like 
below:
+   *
+   * <p>"metrics:solr_cores_filesystem_disk_space_bytes:type=usable_space" or
+   * "metrics:jvm_cpu_count" Metrics are fetched from /admin/metrics and 
parsed using shared utility
+   * methods.
+   */
+  private void getRemoteMetrics(
+      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+    Set<MetricRequest> metricRequests = new HashSet<>();
+    Set<String> requestedMetricNames = new HashSet<>();
+    Set<String> labelsFilter = new HashSet<>();
+
+    // Parse metric tags into structured MetricRequest objects
+    for (String tag : requestedTagNames) {
+      try {
+        MetricRequest request = MetricRequest.fromTag(tag);
+        metricRequests.add(request);
+        requestedMetricNames.add(request.metricName());
+
+        if (request.hasLabelFilter()) {
+          labelsFilter.add(request.kvLabel());
+        }
+
+        // Pre-populate the map tag key to match its corresponding prometheus 
metrics
+        ctx.tags.put(tag, null);
+      } catch (IllegalArgumentException e) {
+        // Skip invalid metric tags
+        continue;
+      }
+    }
+
+    if (requestedMetricNames.isEmpty()) {
+      return;
+    }
+
+    // Fetch all prometheus metrics from requested prometheus metric names
+    String[] lines =
+        SolrClientNodeStateProvider.fetchBatchedMetric(ctx.getNode(), ctx, 
requestedMetricNames);
+
+    // Process prometheus response using structured MetricRequest objects
+    for (String line : lines) {
+      if (shouldSkipPrometheusLine(line)) continue;
+
+      String lineMetricName = extractMetricNameFromLine(line);
+      Long value = Metrics.extractPrometheusValue(line);
+
+      // Find matching MetricRequest(s) for this line
+      for (MetricRequest request : metricRequests) {
+        if (!request.metricName().equals(lineMetricName)) {
+          continue; // Metric name doesn't match
+        }
+
+        // Skip metric if it does not contain requested label
+        if (request.hasLabelFilter() && !line.contains(request.kvLabel())) {
+          continue;
+        }
+
+        // Found a match - store the value using the original tag
+        ctx.tags.put(request.originalTag(), value);
+        break; // Move to next line since we found our match
+      }
+    }
+  }
+
+  public static String extractMetricNameFromLine(String line) {
+    if (line.contains("{")) {
+      return line.substring(0, line.indexOf("{"));
+    } else {
+      return line.split(" ")[0];
+    }
+  }
+
+  public static String extractLabelValueFromLine(String line, String labelKey) 
{
+    String labelPattern = labelKey + "=\"";
+    if (!line.contains(labelPattern)) return null;
+
+    int startIdx = line.indexOf(labelPattern) + labelPattern.length();
+    int endIdx = line.indexOf("\"", startIdx);
+    return endIdx > startIdx ? line.substring(startIdx, endIdx) : null;
+  }
+
+  /** Helper method to check if a Prometheus line should be skipped (comments 
or empty lines). */
+  public static boolean shouldSkipPrometheusLine(String line) {
+    return line.startsWith("#") || line.trim().isEmpty();

Review Comment:
   use isBlank 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");

Review Comment:
   again



##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -185,10 +218,153 @@ public void getTags(Set<String> requestedTags, 
SolrClientNodeStateProvider.Remot
         return;
       }
 
-      getRemotePropertiesAndMetrics(requestedTags, ctx);
-      getRemoteTags(requestedTags, ctx);
+      // Categorize requested system properties or metrics
+      requestedTags.forEach(
+          tag -> {
+            if (tag.startsWith(SYSPROP)) {
+              requestsProperties.add(tag);
+            } else if (tag.startsWith(METRICS_PREFIX)) {
+              requestedMetrics.add(tag);
+            } else {
+              requestedMetricTags.add(tag);
+            }
+          });
+
+      getRemoteSystemProps(requestsProperties, ctx);
+      getRemoteMetrics(requestedMetrics, ctx);
+      getRemoteMetricsFromTags(requestedMetricTags, ctx);
+
     } catch (Exception e) {
       throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
     }
   }
+
+  private void getRemoteSystemProps(
+      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+    ModifiableSolrParams params = new ModifiableSolrParams();
+    try {
+      SimpleSolrResponse rsp = ctx.invokeWithRetry(ctx.getNode(), 
"/admin/info/properties", params);
+      NamedList<?> systemPropsRsp = (NamedList<?>) 
rsp.getResponse().get("system.properties");
+      for (String requestedProperty : requestedTagNames) {
+        Object property = 
systemPropsRsp.get(requestedProperty.substring(SYSPROP.length()));
+        if (property != null) ctx.tags.put(requestedProperty, 
property.toString());
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error 
getting remote info", e);
+    }
+  }
+
+  /**
+   * Retrieve values that match metrics. Metrics names are structured like 
below:
+   *
+   * <p>"metrics:solr_cores_filesystem_disk_space_bytes:type=usable_space" or
+   * "metrics:jvm_cpu_count" Metrics are fetched from /admin/metrics and 
parsed using shared utility

Review Comment:
   ```suggestion
      * "metrics:jvm_cpu_count". Metrics are fetched from /admin/metrics and 
parsed using shared utility
   ```



##########
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");
     }
   }
 
-  /** Retrieve values of well known tags, as defined in {@link Tags}. */
-  private void getRemoteTags(
+  /** Retrieve values of well known tags, as defined in {@link Metrics}. */
+  private void getRemoteMetricsFromTags(
       Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
 
     // First resolve names into actual Tags instances
-    EnumSet<Tags> requestedTags = EnumSet.noneOf(Tags.class);
-    for (Tags t : Tags.values()) {
+    EnumSet<Metrics> requestedMetricNames = EnumSet.noneOf(Metrics.class);
+    for (Metrics t : Metrics.values()) {
       if (requestedTagNames.contains(t.tagName)) {
-        requestedTags.add(t);
+        requestedMetricNames.add(t);
       }
     }
-    if (requestedTags.isEmpty()) {
+
+    if (requestedMetricNames.isEmpty()) {
       return;
     }
 
-    Set<String> groups = new HashSet<>();
-    List<String> prefixes = new ArrayList<>();
-    for (Tags t : requestedTags) {
-      groups.add(t.group);
-      prefixes.add(t.prefix);
+    ModifiableSolrParams params = new ModifiableSolrParams();
+    params.add("wt", "prometheus");
+
+    // Collect unique metric names
+    Set<String> uniqueMetricNames = new HashSet<>();
+    for (Metrics t : requestedMetricNames) {
+      uniqueMetricNames.add(t.metricName);
     }
 
-    ModifiableSolrParams params = new ModifiableSolrParams();
-    params.add("group", StrUtils.join(groups, ','));
-    params.add("prefix", StrUtils.join(prefixes, ','));
+    // Use metric name filtering to get only the metrics we need
+    params.add("name", StrUtils.join(uniqueMetricNames, ','));
 
     try {
+      var req = new GenericSolrRequest(METHOD.GET, "/admin/metrics", 
SolrRequestType.ADMIN, params);
+      req.setResponseParser(new InputStreamResponseParser("prometheus"));
+
+      String baseUrl =
+          
ctx.zkClientClusterStateProvider.getZkStateReader().getBaseUrlForNodeName(ctx.getNode());
       SimpleSolrResponse rsp =
-          ctx.invokeWithRetry(ctx.getNode(), CommonParams.METRICS_PATH, 
params);
-      NamedList<?> metrics = (NamedList<?>) rsp.getResponse().get("metrics");
-      if (metrics != null) {
-        for (Tags t : requestedTags) {
-          ctx.tags.put(t.tagName, t.extractResult(metrics));
+          ctx.cloudSolrClient.getHttpClient().requestWithBaseUrl(baseUrl, 
req::process);

Review Comment:
   thanks for using our new API for this



##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -185,10 +218,153 @@ public void getTags(Set<String> requestedTags, 
SolrClientNodeStateProvider.Remot
         return;
       }
 
-      getRemotePropertiesAndMetrics(requestedTags, ctx);
-      getRemoteTags(requestedTags, ctx);
+      // Categorize requested system properties or metrics
+      requestedTags.forEach(
+          tag -> {
+            if (tag.startsWith(SYSPROP)) {
+              requestsProperties.add(tag);
+            } else if (tag.startsWith(METRICS_PREFIX)) {
+              requestedMetrics.add(tag);
+            } else {
+              requestedMetricTags.add(tag);
+            }
+          });
+
+      getRemoteSystemProps(requestsProperties, ctx);
+      getRemoteMetrics(requestedMetrics, ctx);
+      getRemoteMetricsFromTags(requestedMetricTags, ctx);
+
     } catch (Exception e) {
       throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e);
     }
   }
+
+  private void getRemoteSystemProps(
+      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+    ModifiableSolrParams params = new ModifiableSolrParams();
+    try {
+      SimpleSolrResponse rsp = ctx.invokeWithRetry(ctx.getNode(), 
"/admin/info/properties", params);
+      NamedList<?> systemPropsRsp = (NamedList<?>) 
rsp.getResponse().get("system.properties");
+      for (String requestedProperty : requestedTagNames) {
+        Object property = 
systemPropsRsp.get(requestedProperty.substring(SYSPROP.length()));
+        if (property != null) ctx.tags.put(requestedProperty, 
property.toString());
+      }
+    } catch (Exception e) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error 
getting remote info", e);
+    }
+  }
+
+  /**
+   * Retrieve values that match metrics. Metrics names are structured like 
below:
+   *
+   * <p>"metrics:solr_cores_filesystem_disk_space_bytes:type=usable_space" or
+   * "metrics:jvm_cpu_count" Metrics are fetched from /admin/metrics and 
parsed using shared utility
+   * methods.
+   */
+  private void getRemoteMetrics(
+      Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx 
ctx) {
+    Set<MetricRequest> metricRequests = new HashSet<>();
+    Set<String> requestedMetricNames = new HashSet<>();
+    Set<String> labelsFilter = new HashSet<>();
+
+    // Parse metric tags into structured MetricRequest objects
+    for (String tag : requestedTagNames) {
+      try {
+        MetricRequest request = MetricRequest.fromTag(tag);
+        metricRequests.add(request);
+        requestedMetricNames.add(request.metricName());
+
+        if (request.hasLabelFilter()) {
+          labelsFilter.add(request.kvLabel());
+        }
+
+        // Pre-populate the map tag key to match its corresponding prometheus 
metrics
+        ctx.tags.put(tag, null);
+      } catch (IllegalArgumentException e) {
+        // Skip invalid metric tags
+        continue;
+      }
+    }
+
+    if (requestedMetricNames.isEmpty()) {
+      return;
+    }
+
+    // Fetch all prometheus metrics from requested prometheus metric names
+    String[] lines =
+        SolrClientNodeStateProvider.fetchBatchedMetric(ctx.getNode(), ctx, 
requestedMetricNames);
+
+    // Process prometheus response using structured MetricRequest objects
+    for (String line : lines) {
+      if (shouldSkipPrometheusLine(line)) continue;
+
+      String lineMetricName = extractMetricNameFromLine(line);
+      Long value = Metrics.extractPrometheusValue(line);
+
+      // Find matching MetricRequest(s) for this line
+      for (MetricRequest request : metricRequests) {
+        if (!request.metricName().equals(lineMetricName)) {
+          continue; // Metric name doesn't match
+        }
+
+        // Skip metric if it does not contain requested label
+        if (request.hasLabelFilter() && !line.contains(request.kvLabel())) {
+          continue;
+        }
+
+        // Found a match - store the value using the original tag
+        ctx.tags.put(request.originalTag(), value);
+        break; // Move to next line since we found our match
+      }
+    }
+  }
+
+  public static String extractMetricNameFromLine(String line) {

Review Comment:
   when writing parsing code, I find it immensely helpful to include a comment 
with a sample value



##########
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")) {

Review Comment:
   that constant again



##########
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")) {

Review Comment:
   CC @epugh is looking to make this more graceful



-- 
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