dsmiley commented on code in PR #3164:
URL: https://github.com/apache/solr/pull/3164#discussion_r1948435790
##########
solr/core/src/java/org/apache/solr/metrics/prometheus/SolrPrometheusFormatter.java:
##########
@@ -206,6 +251,22 @@ public void collectGaugeDatapoint(
metricGauges.get(metricName).add(dataPoint);
}
+ /**
+ * Collects {@link
io.prometheus.metrics.model.snapshots.SummarySnapshot.SummaryDataPointSnapshot}
+ * and appends to existing metric or create new metric if name does not exist
+ *
+ * @param metricName Name of metric
+ * @param dataPoint Gauge datapoint to be collected
+ */
+ public void collectSummaryDatapoint(
+ String metricName, SummarySnapshot.SummaryDataPointSnapshot dataPoint) {
+ if (!metricSummaries.containsKey(metricName)) {
+ metricSummaries.put(metricName, new ArrayList<>());
+ }
+
+ metricSummaries.get(metricName).add(dataPoint);
Review Comment:
```suggestion
metricSummaries.computeIfAbsent(metricName, k -> new
ArrayList<>()).add(dataPoint);
```
##########
solr/core/src/java/org/apache/solr/metrics/prometheus/SolrPrometheusFormatter.java:
##########
@@ -176,6 +202,25 @@ public GaugeSnapshot.GaugeDataPointSnapshot
createGaugeDatapoint(double value, L
return
GaugeSnapshot.GaugeDataPointSnapshot.builder().value(value).labels(labels).build();
}
+ /**
+ * Create a {@link
io.prometheus.metrics.model.snapshots.SummarySnapshot.SummaryDataPointSnapshot}
+ * with labels
+ *
+ * @param count number of observations
+ * @param sum sum of all values
+ * @param quantiles quantile information
+ * @param labels set of name/values labels
+ */
+ public SummarySnapshot.SummaryDataPointSnapshot
createSummaryDataPointSnapshot(
+ long count, double sum, Quantiles quantiles, Labels labels) {
+ return SummarySnapshot.SummaryDataPointSnapshot.builder()
+ .count(count)
+ .sum(sum)
+ .labels(labels)
+ .quantiles(quantiles)
+ .build();
+ }
Review Comment:
I don't see why this is its own method, as you now feel the need to have
quite a number of lines here (including documentation). Just inline it.
##########
solr/core/src/java/org/apache/solr/metrics/prometheus/core/SolrCoreHandlerMetric.java:
##########
@@ -28,7 +28,7 @@ public class SolrCoreHandlerMetric extends SolrCoreMetric {
public static final String CORE_REQUESTS_TOTAL =
"solr_metrics_core_requests";
public static final String CORE_REQUESTS_UPDATE_HANDLER =
"solr_metrics_core_update_handler";
public static final String CORE_REQUESTS_TOTAL_TIME =
"solr_metrics_core_requests_time";
- public static final String CORE_REQUEST_TIMES =
"solr_metrics_core_average_request_time";
+ public static final String CORE_REQUEST_TIMES =
"solr_metrics_core_request_time";
Review Comment:
I love that feedback in general but not sure if there is a consistency issue
(like do this not just here but basically everywhere)
##########
solr/core/src/java/org/apache/solr/metrics/prometheus/SolrPrometheusFormatter.java:
##########
@@ -220,6 +281,11 @@ public MetricSnapshots collect() {
snapshots.add(
new GaugeSnapshot(new MetricMetadata(metricName),
metricGauges.get(metricName)));
}
+ for (String metricName : metricSummaries.keySet()) {
Review Comment:
Always iterate the entrySet or call forEach so you don't have to lookup the
value of each one separately
--
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]
