dsmiley commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2308371965
##########
solr/core/src/java/org/apache/solr/util/stats/OtelInstrumentedExecutorService.java:
##########
@@ -0,0 +1,283 @@
+package org.apache.solr.util.stats;
+
+import static org.apache.solr.metrics.SolrMetricProducer.CATEGORY_ATTR;
+import static org.apache.solr.metrics.SolrMetricProducer.TYPE_ATTR;
+
+import io.opentelemetry.api.common.AttributeKey;
+import io.opentelemetry.api.common.Attributes;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ForkJoinPool;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.solr.core.SolrInfoBean;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.metrics.otel.OtelUnit;
+import org.apache.solr.metrics.otel.instruments.AttributedLongCounter;
+import org.apache.solr.metrics.otel.instruments.AttributedLongTimer;
+import
org.apache.solr.metrics.otel.instruments.AttributedLongTimer.MetricTimer;
+import org.apache.solr.metrics.otel.instruments.AttributedLongUpDownCounter;
+
+/**
+ * OTEL instrumentation wrapper around {@link ExecutorService}. Based on {@link
+ * com.codahale.metrics.InstrumentedExecutorService}.
+ */
+public class OtelInstrumentedExecutorService implements ExecutorService {
+ public static final AttributeKey<String> NAME_ATTR =
AttributeKey.stringKey("executor_name");
+
+ private final ExecutorService delegate;
+ private final AttributedLongCounter submitted;
+ private final AttributedLongUpDownCounter running;
+ private final AttributedLongCounter completed;
+ private final AttributedLongTimer idle;
+ private final AttributedLongTimer duration;
+
+ public OtelInstrumentedExecutorService(
+ ExecutorService delegate,
+ SolrMetricsContext ctx,
+ SolrInfoBean.Category category,
+ String executorName) {
+ this.delegate = delegate;
+
+ Attributes attrs =
+ Attributes.builder()
+ .put(CATEGORY_ATTR, category.toString())
+ .put(NAME_ATTR, executorName)
+ .build();
+
+ // Each metric type needs a separate name to avoid obscuring other types
+ this.submitted =
+ new AttributedLongCounter(
+ ctx.longCounter("solr_executor_tasks", "Number of ExecutorService
tasks"),
+ attrs.toBuilder().put(TYPE_ATTR, "submitted").build());
+ this.completed =
+ new AttributedLongCounter(
+ ctx.longCounter("solr_executor_tasks", "Number of ExecutorService
tasks"),
+ attrs.toBuilder().put(TYPE_ATTR, "completed").build());
+ this.running =
+ new AttributedLongUpDownCounter(
+ ctx.longUpDownCounter(
+ "solr_executor_tasks_active", "Number of running
ExecutorService tasks"),
+ attrs.toBuilder().put(TYPE_ATTR, "running").build());
+ this.idle =
+ new AttributedLongTimer(
+ ctx.longHistogram(
+ "solr_executor_task_times",
+ "Timing of ExecutorService tasks",
+ OtelUnit.MILLISECONDS),
+ attrs.toBuilder().put(TYPE_ATTR, "idle").build());
+ this.duration =
+ new AttributedLongTimer(
+ ctx.longHistogram(
+ "solr_executor_task_times",
+ "Timing of ExecutorService tasks",
+ OtelUnit.MILLISECONDS),
+ attrs.toBuilder().put(TYPE_ATTR, "duration").build());
+
+ if (delegate instanceof ThreadPoolExecutor) {
+ ThreadPoolExecutor threadPool = (ThreadPoolExecutor) delegate;
+ ctx.observableLongGauge(
+ "solr_executor_thread_pool_size",
+ "Thread pool size",
+ measurement -> {
+ measurement.record(
+ threadPool.getPoolSize(), attrs.toBuilder().put(TYPE_ATTR,
"size").build());
+ measurement.record(
+ threadPool.getCorePoolSize(), attrs.toBuilder().put(TYPE_ATTR,
"core").build());
+ measurement.record(
+ threadPool.getMaximumPoolSize(),
attrs.toBuilder().put(TYPE_ATTR, "max").build());
+ });
+
+ final BlockingQueue<Runnable> taskQueue = threadPool.getQueue();
+ ctx.observableLongGauge(
+ "solr_executor_thread_pool_tasks",
+ "Thread pool task counts",
+ measurement -> {
+ measurement.record(
+ threadPool.getActiveCount(), attrs.toBuilder().put(TYPE_ATTR,
"active").build());
+ measurement.record(
+ threadPool.getCompletedTaskCount(),
+ attrs.toBuilder().put(TYPE_ATTR, "completed").build());
+ measurement.record(
+ taskQueue.size(), attrs.toBuilder().put(TYPE_ATTR,
"queued").build());
+ measurement.record(
+ taskQueue.remainingCapacity(),
+ attrs.toBuilder().put(TYPE_ATTR, "capacity").build());
+ });
+ } else if (delegate instanceof ForkJoinPool) {
+ ForkJoinPool forkJoinPool = (ForkJoinPool) delegate;
+ ctx.observableLongGauge(
+ "solr_executor_fork_join_pool_tasks",
+ "Fork join pool task counts",
+ measurement -> {
+ measurement.record(
+ forkJoinPool.getStealCount(), attrs.toBuilder().put(TYPE_ATTR,
"stolen").build());
+ measurement.record(
+ forkJoinPool.getQueuedTaskCount(),
+ attrs.toBuilder().put(TYPE_ATTR, "queued").build());
+ });
+ ctx.observableLongGauge(
+ "solr_executor_fork_join_pool_threads",
+ "Fork join pool thread counts",
+ measurement -> {
+ measurement.record(
+ forkJoinPool.getActiveThreadCount(),
+ attrs.toBuilder().put(TYPE_ATTR, "active").build());
+ measurement.record(
+ forkJoinPool.getRunningThreadCount(),
+ attrs.toBuilder().put(TYPE_ATTR, "running").build());
+ });
+ }
+ }
+
+ /** {@inheritDoc} */
Review Comment:
This is always pointless. `@inheritDoc` is useful if you want to _add_
information beforehand as a precursor to the inherited/boilerplate information.
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -1132,14 +1129,13 @@ private void loadInternal() {
// setup executor to load cores in parallel
ExecutorService coreLoadExecutor =
- MetricUtils.instrumentedExecutorService(
+ new OtelInstrumentedExecutorService(
Review Comment:
I think we shouldn't be wrapping the ExecutorService if metrics are
disabled. For example they should be disabled by default in tests. Having a
factory method allows the conditional disablement to exist there.
##########
solr/core/src/java/org/apache/solr/util/stats/OtelInstrumentedExecutorService.java:
##########
@@ -0,0 +1,283 @@
+package org.apache.solr.util.stats;
+
+import static org.apache.solr.metrics.SolrMetricProducer.CATEGORY_ATTR;
+import static org.apache.solr.metrics.SolrMetricProducer.TYPE_ATTR;
+
+import io.opentelemetry.api.common.AttributeKey;
+import io.opentelemetry.api.common.Attributes;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.ForkJoinPool;
+import java.util.concurrent.Future;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.solr.core.SolrInfoBean;
+import org.apache.solr.metrics.SolrMetricsContext;
+import org.apache.solr.metrics.otel.OtelUnit;
+import org.apache.solr.metrics.otel.instruments.AttributedLongCounter;
+import org.apache.solr.metrics.otel.instruments.AttributedLongTimer;
+import
org.apache.solr.metrics.otel.instruments.AttributedLongTimer.MetricTimer;
+import org.apache.solr.metrics.otel.instruments.AttributedLongUpDownCounter;
+
+/**
+ * OTEL instrumentation wrapper around {@link ExecutorService}. Based on {@link
+ * com.codahale.metrics.InstrumentedExecutorService}.
+ */
+public class OtelInstrumentedExecutorService implements ExecutorService {
+ public static final AttributeKey<String> NAME_ATTR =
AttributeKey.stringKey("executor_name");
+
+ private final ExecutorService delegate;
+ private final AttributedLongCounter submitted;
+ private final AttributedLongUpDownCounter running;
+ private final AttributedLongCounter completed;
+ private final AttributedLongTimer idle;
+ private final AttributedLongTimer duration;
+
+ public OtelInstrumentedExecutorService(
+ ExecutorService delegate,
+ SolrMetricsContext ctx,
+ SolrInfoBean.Category category,
+ String executorName) {
+ this.delegate = delegate;
+
+ Attributes attrs =
+ Attributes.builder()
+ .put(CATEGORY_ATTR, category.toString())
+ .put(NAME_ATTR, executorName)
+ .build();
+
+ // Each metric type needs a separate name to avoid obscuring other types
+ this.submitted =
+ new AttributedLongCounter(
+ ctx.longCounter("solr_executor_tasks", "Number of ExecutorService
tasks"),
+ attrs.toBuilder().put(TYPE_ATTR, "submitted").build());
+ this.completed =
+ new AttributedLongCounter(
+ ctx.longCounter("solr_executor_tasks", "Number of ExecutorService
tasks"),
+ attrs.toBuilder().put(TYPE_ATTR, "completed").build());
Review Comment:
I'm confused by this feedback: shouldn't each AttributedLongCounter have its
own longCounter since they are counting different things?
--
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]
