dsmiley commented on code in PR #3713:
URL: https://github.com/apache/solr/pull/3713#discussion_r2402213683
##########
solr/core/src/java/org/apache/solr/cluster/placement/impl/NodeMetricImpl.java:
##########
@@ -17,108 +17,51 @@
package org.apache.solr.cluster.placement.impl;
-import java.util.Objects;
import java.util.function.Function;
-import org.apache.solr.client.solrj.impl.NodeValueFetcher;
import org.apache.solr.cluster.placement.NodeMetric;
-/**
- * Node metric identifier, corresponding to a node-level metric registry and
the internal metric
- * name.
- */
+/** Node metric identifier, corresponding to a node-level metric name with
labels */
Review Comment:
As there aren't any constants on this class anymore (you removed them)...
the least we could do is share an example or two in the javadoc here.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -17,157 +17,201 @@
package org.apache.solr.client.solrj.impl;
-import java.util.ArrayList;
+import static
org.apache.solr.client.solrj.impl.InputStreamResponseParser.STREAM_KEY;
+
+import java.io.BufferedReader;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+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 java.util.stream.Stream;
+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_KEY);
+ if (metrics == null || metricName == null) return null;
+
+ try (InputStream in = (InputStream) metrics) {
+ return prometheusMetricStream(in)
+ .filter(line ->
extractMetricNameFromLine(line).equals(metricName))
+ .mapToInt((value) -> extractPrometheusValue(value).intValue())
+ .sum();
+ } 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. This
method assumes 1 metric,
+ * so will get the first metricName it sees with associated label and
value.
+ */
+ private static Double extractFromPrometheusResponse(
+ NamedList<Object> root, String metricName, String labelKey, String
labelValue) {
+ Object metrics = root.get(STREAM_KEY);
- } else {
- throw new IllegalArgumentException("Unknown type : " + val);
+ if (metrics == null || metricName == null) {
+ return null;
+ }
+
+ try (InputStream in = (InputStream) metrics) {
+ return prometheusMetricStream(in)
+ .filter(line -> line.startsWith(metricName))
+ .filter(
+ line -> {
+ // If metric with specific labels were requested, filter by
those labels
+ if (labelKey != null && labelValue != null) {
+ String expectedLabel = labelKey + "=\"" + labelValue +
"\"";
+ return line.contains(expectedLabel);
+ }
+ return true;
+ })
+ .findFirst()
+ .map(Metrics::extractPrometheusValue)
+ .orElse(null);
+ } catch (Exception e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unable to read prometheus
metrics output", e);
}
}
- }
- /** Retrieve values that match JVM system properties and metrics. */
- private void getRemotePropertiesAndMetrics(
- Set<String> requestedTagNames, SolrClientNodeStateProvider.RemoteCallCtx
ctx) {
+ /**
+ * Extracts the numeric value from a Prometheus metric line. Sample
inputs: - With labels:
+ * solr_metrics_core_requests_total{core="demo",...} 123.0 - Without
labels:
+ * solr_metrics_core_requests_total 123.0 - With exemplars:
+ * solr_metrics_core_requests_total{core="demo"} 123.0 #
{trace_id="abc123"} 2.0 1234567890
+ */
+ public static Double extractPrometheusValue(String line) {
+ String s = line.trim();
- 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);
- }
+ // Get the position after the labels if they exist.
+ int afterLabelsPos = s.indexOf('}');
+ String tail = (afterLabelsPos >= 0) ? s.substring(afterLabelsPos +
1).trim() : s;
+
+ // Get the metric value after the first white space and chop off
anything after such as
+ // exemplars from Open Metrics Format
+ int whiteSpacePos = tail.indexOf(' ');
+ String firstToken = (whiteSpacePos >= 0) ? tail.substring(0,
whiteSpacePos) : tail;
+
+ return Double.parseDouble(firstToken);
}
- if (!metricsKeyVsTag.isEmpty()) {
- SolrClientNodeStateProvider.fetchReplicaMetrics(ctx.getNode(), ctx,
metricsKeyVsTag);
+
+ /** Returns a Stream of Prometheus lines for processing with filtered out
comment lines */
+ public static Stream<String> prometheusMetricStream(InputStream
inputStream) {
+ BufferedReader reader =
+ new BufferedReader(new InputStreamReader(inputStream,
StandardCharsets.UTF_8));
+
+ return reader.lines().filter(line -> !isPrometheusCommentLine(line));
}
}
- /** 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);
+
+ for (Metrics t : requestedMetricNames) {
+ Object value = t.extractResult(rsp.getResponse());
Review Comment:
This is the only place extractResult is invoked, and I see it's always
prometheus and a named list with the STREAM key. IMO the code here should take
responsibility of passing the InputStream into extractResult, since it's this
place that chose InputStreamResponseParser. You have the implementation of
extractResult making assumptions that ideally it would not with a generic name
of extractResult and a NamedList (which is a generic bucket of anything).
At your discretion, you could pass a Stream<String> of prometheus metric
lines, or have extractResult do that.
I think extractResult should have a more specific name indicative of the
format. Maybe extractFromPrometheus
--
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]
