mlbiscoc commented on code in PR #3384:
URL: https://github.com/apache/solr/pull/3384#discussion_r2141160310
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -193,14 +203,79 @@ public static class HandlerMetrics {
public final Timer requestTimes;
public final Counter totalTime;
- public HandlerMetrics(SolrMetricsContext solrMetricsContext, String...
metricPath) {
+ public AttributedLongCounter otelRequests;
+ public AttributedLongCounter otelNumErrors;
+ public AttributedLongCounter otelNumServerErrors;
+ public AttributedLongCounter otelNumClientErrors;
+ public AttributedLongCounter otelNumTimeouts;
+ public AttributedLongTimer otelRequestTimes;
+
+ public HandlerMetrics(
+ SolrMetricsContext solrMetricsContext, Attributes attributes,
String... metricPath) {
+
+ // TODO SOLR-17458: To be removed
numErrors = solrMetricsContext.meter("errors", metricPath);
numServerErrors = solrMetricsContext.meter("serverErrors", metricPath);
numClientErrors = solrMetricsContext.meter("clientErrors", metricPath);
numTimeouts = solrMetricsContext.meter("timeouts", metricPath);
requests = solrMetricsContext.counter("requests", metricPath);
requestTimes = solrMetricsContext.timer("requestTimes", metricPath);
totalTime = solrMetricsContext.counter("totalTime", metricPath);
+
+ var baseRequestMetric =
+ solrMetricsContext.longCounter("solr_metrics_core_requests", "HTTP
Solr request counts");
+
+ var baseRequestTimeMetric =
+ solrMetricsContext.longHistogram(
+ "solr_metrics_core_requests_times", "HTTP Solr request times",
"ms");
+
+ otelRequests =
+ new AttributedLongCounter(
+ baseRequestMetric,
+ Attributes.builder()
+ .putAll(attributes)
+ .put(AttributeKey.stringKey("type"), "requests")
+ .build());
Review Comment:
These are the base metrics. Then you bind them to attributes so we don't
need to keep rebuilding them every time we record an event.
##########
solr/core/src/java/org/apache/solr/handler/admin/MetricsHandler.java:
##########
@@ -127,11 +126,12 @@ private void handleRequest(SolrParams params,
BiConsumer<String, Object> consume
return;
}
+ // TODO SOLR-17458: Make this the default option after dropwizard removal
if (PROMETHEUS_METRICS_WT.equals(params.get(CommonParams.WT))) {
- response = handlePrometheusExport(params);
- consumer.accept("metrics", response);
+ consumer.accept("metrics", metricManager.getMetricReader().collect());
Review Comment:
Instead of going through the whole dropwizard -> prometheus formatter logic.
Very simply the metric reader does the OTEL -> prometheus for us through the
SDK.
##########
solr/core/src/test/org/apache/solr/response/TestPrometheusResponseWriter.java:
##########
@@ -127,54 +131,6 @@ public void testPrometheusStructureOutput() throws
Exception {
}
}
- public void testPrometheusDummyOutput() throws Exception {
Review Comment:
Pretty much all Prometheus Formmater tests go away with the exception of
`testPrometheusStructureOutput` which tests for prometheus exposition format
from the OTEL sdk.
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -193,14 +203,79 @@ public static class HandlerMetrics {
public final Timer requestTimes;
public final Counter totalTime;
- public HandlerMetrics(SolrMetricsContext solrMetricsContext, String...
metricPath) {
+ public AttributedLongCounter otelRequests;
+ public AttributedLongCounter otelNumErrors;
+ public AttributedLongCounter otelNumServerErrors;
+ public AttributedLongCounter otelNumClientErrors;
+ public AttributedLongCounter otelNumTimeouts;
+ public AttributedLongTimer otelRequestTimes;
Review Comment:
These are essentially equivalent to Dropwizards metrics above. Once we do
deprecations, we can just change the name to the ones above and remove them.
##########
solr/core/src/java/org/apache/solr/handler/component/SearchHandler.java:
##########
@@ -152,18 +153,28 @@ public void init(PluginInfo info) {
}
}
+ // TODO SOLR-17458: Fix metric Attributes
@Override
- public void initializeMetrics(SolrMetricsContext parentContext, String
scope) {
- super.initializeMetrics(parentContext, scope);
+ public void initializeMetrics(
+ SolrMetricsContext parentContext, Attributes attributes, String scope) {
+ super.initializeMetrics(
+ parentContext,
+ Attributes.builder()
+ .putAll(attributes)
+ .put(AttributeKey.stringKey("category"), getCategory().toString())
+ .put(AttributeKey.stringKey("internal"), "false")
Review Comment:
Instead of having the handler do something like `/select[shard]`, we use a
tag saying `internal` true/false
##########
solr/core/src/java/org/apache/solr/handler/RequestHandlerBase.java:
##########
@@ -193,14 +203,79 @@ public static class HandlerMetrics {
public final Timer requestTimes;
public final Counter totalTime;
- public HandlerMetrics(SolrMetricsContext solrMetricsContext, String...
metricPath) {
+ public AttributedLongCounter otelRequests;
+ public AttributedLongCounter otelNumErrors;
+ public AttributedLongCounter otelNumServerErrors;
+ public AttributedLongCounter otelNumClientErrors;
+ public AttributedLongCounter otelNumTimeouts;
+ public AttributedLongTimer otelRequestTimes;
+
+ public HandlerMetrics(
+ SolrMetricsContext solrMetricsContext, Attributes attributes,
String... metricPath) {
+
+ // TODO SOLR-17458: To be removed
numErrors = solrMetricsContext.meter("errors", metricPath);
numServerErrors = solrMetricsContext.meter("serverErrors", metricPath);
numClientErrors = solrMetricsContext.meter("clientErrors", metricPath);
numTimeouts = solrMetricsContext.meter("timeouts", metricPath);
requests = solrMetricsContext.counter("requests", metricPath);
requestTimes = solrMetricsContext.timer("requestTimes", metricPath);
totalTime = solrMetricsContext.counter("totalTime", metricPath);
+
+ var baseRequestMetric =
+ solrMetricsContext.longCounter("solr_metrics_core_requests", "HTTP
Solr request counts");
+
+ var baseRequestTimeMetric =
+ solrMetricsContext.longHistogram(
+ "solr_metrics_core_requests_times", "HTTP Solr request times",
"ms");
+
+ otelRequests =
+ new AttributedLongCounter(
+ baseRequestMetric,
+ Attributes.builder()
+ .putAll(attributes)
+ .put(AttributeKey.stringKey("type"), "requests")
+ .build());
+
+ otelNumErrors =
+ new AttributedLongCounter(
+ baseRequestMetric,
+ Attributes.builder()
+ .putAll(attributes)
+ .put(AttributeKey.stringKey("type"), "errors")
+ .build());
+
+ otelNumServerErrors =
+ new AttributedLongCounter(
+ baseRequestMetric,
+ Attributes.builder()
+ .putAll(attributes)
+ .put(AttributeKey.stringKey("type"), "serverErrors")
+ .build());
+
+ otelNumClientErrors =
+ new AttributedLongCounter(
+ baseRequestMetric,
+ Attributes.builder()
+ .putAll(attributes)
+ .put(AttributeKey.stringKey("type"), "clientErrors")
+ .build());
+
+ otelNumTimeouts =
+ new AttributedLongCounter(
+ baseRequestMetric,
+ Attributes.builder()
+ .putAll(attributes)
+ .put(AttributeKey.stringKey("type"), "timeouts")
+ .build());
+
+ otelRequestTimes = new AttributedLongTimer(baseRequestTimeMetric,
attributes);
+
+ otelRequests.record(0L);
+ otelNumErrors.record(0L);
+ otelNumServerErrors.record(0L);
+ otelNumClientErrors.record(0L);
+ otelNumTimeouts.record(0L);
Review Comment:
I record 0's so after initializing so it creates the metrics otherwise, if a
handler like `export` is never called, you'll never actually see the metric.
Its debatable if this should actually be here and I am thinking it shouldn't
now. Metrics should probably only exist if a handler is actually being used,
otherwise you bloat the response with 0's.
--
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]
