Re: [PR] SOLR-17628: Add query quantiles metrics to prometheus endpoint [solr]
mlbiscoc merged PR #3164: URL: https://github.com/apache/solr/pull/3164 -- 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]
Re: [PR] SOLR-17628: Add query quantiles metrics to prometheus endpoint [solr]
dsmiley commented on PR #3164: URL: https://github.com/apache/solr/pull/3164#issuecomment-3023974817 Until there is a release branch for release X, no PR is "too late" for release X. Once a release branch exists (e.g. `branch_9_9`), you can take up the question with whoever volunteered to be the release manager (Houston volunteered for 9.9). -- 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]
Re: [PR] SOLR-17628: Add query quantiles metrics to prometheus endpoint [solr]
github-actions[bot] closed pull request #3164: SOLR-17628: Add query quantiles metrics to prometheus endpoint URL: https://github.com/apache/solr/pull/3164 -- 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]
Re: [PR] SOLR-17628: Add query quantiles metrics to prometheus endpoint [solr]
github-actions[bot] commented on PR #3164: URL: https://github.com/apache/solr/pull/3164#issuecomment-2968484617 This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. -- 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]
Re: [PR] SOLR-17628: Add query quantiles metrics to prometheus endpoint [solr]
github-actions[bot] commented on PR #3164: URL: https://github.com/apache/solr/pull/3164#issuecomment-2799284922 This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! -- 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]
Re: [PR] SOLR-17628: Add query quantiles metrics to prometheus endpoint [solr]
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]
Re: [PR] SOLR-17628: Add query quantiles metrics to prometheus endpoint [solr]
mlbiscoc commented on code in PR #3164:
URL: https://github.com/apache/solr/pull/3164#discussion_r1946662321
##
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";
Review Comment:
To me this feels like it doesn't belong anymore. Your dropwizard timer ->
prometheus summary exporting now gives you not only the quantile data but also
sum and count in a sliding window. Not sure if the total time as a gauge over
the lifetime of the core is useful but someone might disagree. I think this can
even be calculated with `promQL` with the new metric you made below. Also the
naming is pretty much identical making it confusing
`solr_metrics_core_requests_time` and `solr_metrics_core_request_time`
##
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 think the unit of time should exist in its name so people don't need to
figure it out and this was missed previously.
```suggestion
public static final String CORE_REQUEST_TIMES =
"solr_metrics_core_request_time_ms";
```
##
solr/core/src/java/org/apache/solr/metrics/prometheus/core/SolrCoreSearcherMetric.java:
##
@@ -27,7 +27,7 @@
/** Dropwizard metrics of name SEARCHER.* */
public class SolrCoreSearcherMetric extends SolrCoreMetric {
public static final String CORE_SEARCHER_METRICS =
"solr_metrics_core_searcher_documents";
- public static final String CORE_SEARCHER_TIMES =
"solr_metrics_core_average_searcher_warmup_time";
+ public static final String CORE_SEARCHER_TIMES =
"solr_metrics_core_searcher_warmup_time";
Review Comment:
```suggestion
public static final String CORE_SEARCHER_TIMES =
"solr_metrics_core_searcher_warmup_time_ms";
```
Same as above assuming this is in `ms`
--
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]
