dsmiley commented on code in PR #3713:
URL: https://github.com/apache/solr/pull/3713#discussion_r2398572932
##########
solr/core/src/java/org/apache/solr/cluster/placement/impl/MetricImpl.java:
##########
Review Comment:
never remove from equals and leave in hashCode! I imagine maybe Error-Prone
might even detect this common error
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -55,18 +60,14 @@ public enum Metrics {
CORES("cores", "solr_cores_loaded") {
@Override
public Object extractResult(NamedList<Object> root) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
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;
+ return prometheusMetricStream(in)
+ .filter(line -> line.startsWith(metricName))
+ .mapToInt((value) -> extractPrometheusValue(value).intValue())
+ .sum();
Review Comment:
freaking beautiful
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -99,53 +100,66 @@ public Object extractResult(NamedList<Object> root) {
* Extract metric value from Prometheus response, optionally filtering by
label. This
* consolidated method handles both labeled and unlabeled metrics.
*/
- private static Long extractFromPrometheusResponse(
+ private static Double extractFromPrometheusResponse(
NamedList<Object> root, String metricName, String labelKey, String
labelValue) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
if (metrics == null || metricName == null) {
return null;
}
try (InputStream in = (InputStream) metrics) {
- String[] lines = parsePrometheusOutput(in);
-
- 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);
- }
+ 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);
}
-
- 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);
+ public static Double extractPrometheusValue(String line) {
+ String s = line.trim();
+
+ // 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);
}
- public static String[] parsePrometheusOutput(InputStream inputStream)
throws Exception {
- String output = new String(inputStream.readAllBytes(),
StandardCharsets.UTF_8);
- return output.split("\n");
+ /** Returns a Stream of Prometheus lines for processing with filtered out
comment lines */
+ public static java.util.stream.Stream<String>
prometheusMetricStream(InputStream inputStream) {
Review Comment:
No FQN necessary
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -99,53 +100,66 @@ public Object extractResult(NamedList<Object> root) {
* Extract metric value from Prometheus response, optionally filtering by
label. This
* consolidated method handles both labeled and unlabeled metrics.
*/
- private static Long extractFromPrometheusResponse(
+ private static Double extractFromPrometheusResponse(
NamedList<Object> root, String metricName, String labelKey, String
labelValue) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
if (metrics == null || metricName == null) {
return null;
}
try (InputStream in = (InputStream) metrics) {
- String[] lines = parsePrometheusOutput(in);
-
- 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);
- }
+ return prometheusMetricStream(in)
Review Comment:
beautiful
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -258,73 +272,75 @@ private void getRemoteSystemProps(
* 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
+ * "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());
+ 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;
- }
+ // Pre-populate the map tag key to match its corresponding prometheus
metrics
+ ctx.tags.put(tag, null);
}
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
- }
+ // Process prometheus stream response using structured MetricRequest
objects
+ try (java.util.stream.Stream<String> stream =
+ SolrClientNodeStateProvider.fetchMetricStream(ctx.getNode(), ctx,
requestedMetricNames)) {
+
+ stream.forEach(
+ line -> {
+ String lineMetricName = extractMetricNameFromLine(line);
+ Double value = Metrics.extractPrometheusValue(line);
+
+ // Find matching MetricRequest(s) for this line
+ for (MetricRequest request : metricRequests) {
Review Comment:
in practice, maybe we assume a small number of requests? If not, a
`Set<String>` of metric names could filter out anything not needed efficiently.
We probably also filter to what we need... so we don't need additional
complexity I guess.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -337,7 +353,7 @@ public static String extractLabelValueFromLine(String line,
String labelKey) {
}
/** Helper method to check if a Prometheus line should be skipped (comments
or empty lines). */
- public static boolean shouldSkipPrometheusLine(String line) {
+ public static boolean isPrometheusCommentLine(String line) {
return line.startsWith("#") || line.trim().isEmpty();
Review Comment:
```suggestion
return line.startsWith("#") || line.isBlank();
```
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -55,18 +60,14 @@ public enum Metrics {
CORES("cores", "solr_cores_loaded") {
@Override
public Object extractResult(NamedList<Object> root) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
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;
+ return prometheusMetricStream(in)
+ .filter(line -> line.startsWith(metricName))
Review Comment:
wow... we assume that all metrics that start with this can be summed, and
regardless of attributes. That's a big fat assumption that should be stated
explicitly because I suspect it might not be true or might subtly become
inappropriate some day as metrics/attributes are added
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -99,53 +100,66 @@ public Object extractResult(NamedList<Object> root) {
* Extract metric value from Prometheus response, optionally filtering by
label. This
* consolidated method handles both labeled and unlabeled metrics.
*/
- private static Long extractFromPrometheusResponse(
+ private static Double extractFromPrometheusResponse(
NamedList<Object> root, String metricName, String labelKey, String
labelValue) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
if (metrics == null || metricName == null) {
return null;
}
try (InputStream in = (InputStream) metrics) {
- String[] lines = parsePrometheusOutput(in);
-
- 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);
- }
+ 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);
}
-
- 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);
+ public static Double extractPrometheusValue(String line) {
Review Comment:
please add a comment showing a sample value
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -99,53 +100,66 @@ public Object extractResult(NamedList<Object> root) {
* Extract metric value from Prometheus response, optionally filtering by
label. This
* consolidated method handles both labeled and unlabeled metrics.
*/
- private static Long extractFromPrometheusResponse(
+ private static Double extractFromPrometheusResponse(
NamedList<Object> root, String metricName, String labelKey, String
labelValue) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
if (metrics == null || metricName == null) {
return null;
}
try (InputStream in = (InputStream) metrics) {
- String[] lines = parsePrometheusOutput(in);
-
- 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);
- }
+ 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);
}
-
- 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);
+ public static Double extractPrometheusValue(String line) {
+ String s = line.trim();
+
+ // 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);
}
- public static String[] parsePrometheusOutput(InputStream inputStream)
throws Exception {
- String output = new String(inputStream.readAllBytes(),
StandardCharsets.UTF_8);
- return output.split("\n");
+ /** Returns a Stream of Prometheus lines for processing with filtered out
comment lines */
+ public static java.util.stream.Stream<String>
prometheusMetricStream(InputStream inputStream) {
+ BufferedReader reader =
+ new BufferedReader(new InputStreamReader(inputStream,
StandardCharsets.UTF_8));
+
+ return reader
+ .lines()
+ .filter(line -> !isPrometheusCommentLine(line))
+ .onClose(
+ () -> {
+ try {
+ reader.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ });
Review Comment:
this looks suspicious; I've never seen this before. The caller of
prometheusMetricStream obtains the inputStream; it has the responsibility of
closing the stream, which I see will happen via try-with-resources. The reader
doesn't add anything that must be closed.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -99,53 +100,66 @@ public Object extractResult(NamedList<Object> root) {
* Extract metric value from Prometheus response, optionally filtering by
label. This
* consolidated method handles both labeled and unlabeled metrics.
*/
- private static Long extractFromPrometheusResponse(
+ private static Double extractFromPrometheusResponse(
NamedList<Object> root, String metricName, String labelKey, String
labelValue) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
if (metrics == null || metricName == null) {
return null;
}
try (InputStream in = (InputStream) metrics) {
- String[] lines = parsePrometheusOutput(in);
-
- 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);
- }
+ 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);
}
-
- 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);
+ public static Double extractPrometheusValue(String line) {
+ String s = line.trim();
+
+ // 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);
}
- public static String[] parsePrometheusOutput(InputStream inputStream)
throws Exception {
- String output = new String(inputStream.readAllBytes(),
StandardCharsets.UTF_8);
- return output.split("\n");
+ /** Returns a Stream of Prometheus lines for processing with filtered out
comment lines */
+ public static java.util.stream.Stream<String>
prometheusMetricStream(InputStream inputStream) {
+ BufferedReader reader =
+ new BufferedReader(new InputStreamReader(inputStream,
StandardCharsets.UTF_8));
+
+ return reader
+ .lines()
+ .filter(line -> !isPrometheusCommentLine(line))
+ .onClose(
Review Comment:
again; questionable
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -99,53 +100,66 @@ public Object extractResult(NamedList<Object> root) {
* Extract metric value from Prometheus response, optionally filtering by
label. This
* consolidated method handles both labeled and unlabeled metrics.
Review Comment:
could mention the behavior if the criteria matches multiple metric lines
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/SolrClientNodeStateProvider.java:
##########
@@ -213,14 +214,17 @@ static String[] fetchBatchedMetric(String solrNode,
RemoteCallCtx ctx, Set<Strin
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);
+
+ try {
+ InputStream in =
Review Comment:
Should use try-with-resources as the code here owns the lifecycle of that
InputStream. It's a weird case because of "stream" from
InputStreamRequestParser providing us the stream indirectly but we are
considered the owner, not that thing. I suspect this will be clarified soon if
@epugh simplifies something. It's not the job of
`NodeValueFetcher.Metrics.parsePrometheusOutput` to close that InputStream,
since that method is being provided the stream. This is a standard Java
practice/convention/protocol so that a _correct_ closer (and not missing or
redundant closer) is identified.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/client/solrj/impl/NodeValueFetcher.java:
##########
@@ -55,18 +60,14 @@ public enum Metrics {
CORES("cores", "solr_cores_loaded") {
@Override
public Object extractResult(NamedList<Object> root) {
- Object metrics = root.get("stream");
+ Object metrics = root.get(STREAM_KEY);
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;
+ return prometheusMetricStream(in)
+ .filter(line -> line.startsWith(metricName))
Review Comment:
in this case, can we say the metric name *equals*, not startsWith?
--
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]
