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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
