mlbiscoc commented on code in PR #3713:
URL: https://github.com/apache/solr/pull/3713#discussion_r2399438000
##########
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:
The line looks like `solr_cores_loaded{type-...} 123` so I can't just do
equals. I changed it to `extractMetricNameFromLine(line).equals(metricName)` if
you want it more verbose?
##########
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:
I added a comment that this just gets the first line it sees as this is what
it assumes. If you need multiple or want to do a sum, you have to `Override`
extractResult which is what was happening above and what dropwizard did before.
##########
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:
Good point. Thats something I left out. I add a check to see if the line
contains the attributes.
##########
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:
Yeah there is an assumption of small number of requests here. I found there
wasn't a large number of hard coded metrics were wanted. It was somewhat bigger
with dropwizard when it mixed system properties and metrics together in a
single request.
--
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]
