Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]

["dsmiley (via GitHub)" <gi...@apache.org>]

dsmiley commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2312787312


##########
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:
   Ah I see.  It's not an obvious implementation.  The _appearance_ of LongUpDownCounter or whatever suggests to me it actually has a value.  But kinda sorta... the wrapped Attributed thing _over_ it provides the attributes to the recording method on LongUpDownCounter (e.g. inc by 1) call and then internally it does what you say.  It's not obvious at all.  Did you see javadocs/sample code that suggest to you that the LongUpDownCounter instance (or similar) could be reused?



-- 
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-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org