You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "jkmuriithi (via GitHub)" <gi...@apache.org> on 2025/08/27 19:57:06 UTC
[PR] Migrate thread pool metrics to OTEL [solr]
jkmuriithi opened a new pull request, #3519:
URL: https://github.com/apache/solr/pull/3519
https://issues.apache.org/jira/browse/SOLR-17806
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
With Dropwizard, `ExecutorService`'s were instrumented via calls to `MetricUtils.instrumentedExecutorService`, which in turn created an instance of `com.codahale.metrics.InstrumentedExecutorService`. This gave us task/thread counts and task duration information for various Solr thread pools.
Screenshot example of the existing metrics:
<img width="613" height="978" alt="Screenshot 2025-08-27 at 3 45 55 PM" src="https://github.com/user-attachments/assets/ef1b2322-bcda-4c4f-8afa-08920f18c14d" />
# Solution
Create `OtelInstrumentedExecutorService` to act as a drop-in replacement for `InstrumentedExecutorService`. Replace the `MetricUtils` method calls.
Screenshot example of the new metrics:
<img width="1349" height="1047" alt="Screenshot 2025-08-27 at 3 55 03 PM" src="https://github.com/user-attachments/assets/a9e1a729-24ce-4854-b63c-816a413c3e2d" />
# Tests
The previous `MetricUtils` static method didn't seem to have any tests. Since this PR introduces a new class to replace the functionality of that method, I think it makes sense to add some unit tests. I'll come back to this PR to add those.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [x] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2312893354
##########
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:
So here were [some snippets](https://opentelemetry.io/docs/languages/java/api/#counter) from the OTEL java documentation. The point of attributed wrappers is so we are not populating the classes with shared instrument members and attribute initializations and figuring out how to link them together at recording. The attributed classes wrap them together for us and make it look very similar to Dropwizard. But I did some digging in the OTEL spec and found this:
> If the attribute names and types are provided during the [counter creation](https://opentelemetry.io/docs/specs/otel/metrics/api/#counter-creation), the [OpenTelemetry API](https://opentelemetry.io/docs/specs/otel/overview/#api) authors MAY allow attribute values to be passed in using a more efficient way (e.g. strong typed struct allocated on the callstack, tuple). The API MUST allow callers to provide flexible attributes at invocation time rather than having to register all the possible attribute names during the instrument creation.
Attributes MAY be passed but the API MUST have attributes passed at invocation time rather than insturment creation. Digging even further I found they actually did allow for whats called `Bound` insturments to link attributes at creation which looks close to what my attributed wrappers did until they removed it in 1.10. See this [PR](https://github.com/open-telemetry/opentelemetry-java/pull/3928). So attributes at invocation time is the only way you can do it right now.
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2312615038
##########
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:
No worries, my explanation there might have had too much fluff there but let me try one last time I swear :). The `LongCounter` came from an `SDKMeterProvider` under the hood and
my guess of the implementation of that `LongCounter` from the `SDKMeterProvider` in pseudocode might look something like this:
```
ConcurrentMap<Attributes, LongAdder> store = new ConcurrentHashMap<>();
void add(long n, Attributes attrs) {
store.computeIfAbsent(attrs, a -> new LongAdder()).add(n);
}
```
I also did it for the DirectUpdateHandler2 already [here](https://github.com/apache/solr/blob/b79d939983740020af496e622cec721e6674c89c/solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java#L247) with a bunch of tests making sure they [have their own values](https://github.com/apache/solr/blob/b79d939983740020af496e622cec721e6674c89c/solr/core/src/test/org/apache/solr/update/DirectUpdateHandlerTest.java#L228). But still, I agree we should have a test the different values for this PR.
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc merged PR #3519:
URL: https://github.com/apache/solr/pull/3519
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2308889173
##########
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 understand that the name & description is common for multiple fully attributed / distinguished metrics. But doesn't the "LongCounter" store the underlying value, and thus wouldn't sharing it with two different AttributedLongCounters thus share the same LongCounter instance (the backing value for the current _value_), and thus be totally invalid?
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #3519:
URL: https://github.com/apache/solr/pull/3519#issuecomment-3449022556
As I browse Solr 10 metrics endpoint for the first time, I'm seeing these metrics standout as seeming misplaced to me, from a naming standpoint. For metrics on a Solr core, I expect the metrics name to start with `solr_core_`. If not at a core level, I think `solr_node_ would be the right prefix. We do similarly already for caches. What follows the `solr_` prefix should be a very restricted list matching the enum `org.apache.solr.core.SolrInfoBean.Group`
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2305174004
##########
solr/core/src/java/org/apache/solr/util/stats/OtelInstrumentedExecutorService.java:
##########
@@ -0,0 +1,276 @@
+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.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");
Review Comment:
Maybe call it `EXECUTOR_NAME` since just `NAME` is too generic, I can imagine an actual `NAME` attribute existing one day.
##########
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:
Initialize the `ctx.longCounter` once then just pass it in
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -853,14 +853,11 @@ private void loadInternal() {
// NOCOMMIT: Do we need this for OTEL or is this specific for reporters?
Review Comment:
We can start removing these NOCOMMIT comments
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -445,10 +445,7 @@ public void initializeMetrics(
// TODO SOLR-17458: Add Otel
Review Comment:
Clean up this TODO
##########
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());
Review Comment:
I'm not sure if this running should be an up/down counter or a gauge but I think its ok. I only every used them for cumulative sums which can also be negative but I could be wrong here.
Regardless just put running in the name. No point in having the type attribute if its the only metric with the `type=running as there is nothing to aggregate on it for this metric.
##########
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());
Review Comment:
Same as the comment on initializing once.
##########
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());
+ });
Review Comment:
I just pushed a [commit](https://github.com/apache/solr/commit/5cc10d5b7241b1c1195a3088a8709c556e25f0b8) to start closing these observables. We should close these on `shutdown()`
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc closed pull request #3519: SOLR-17806: Migrate thread pool metrics to OTEL
URL: https://github.com/apache/solr/pull/3519
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2312602422
##########
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 admit I don't get it despite your attempts to explain it. But no matter; let's proceed with your recommendation anyway. Once the change you suggest is made, presumably a test here will show that the values between two such AttributedLongCounters are different despite sharing the same LongCounter (let's ensure this is so if not). I'd love to debug this test and dive in and see for myself.
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2308436157
##########
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:
The `submitted` and `completed` metric are sharing a single metric name `solr_executor_tasks` and description that is differentiated by attributes. Some have their own longCounter but some share names just with different attributes. The way it is done here technically works. But the reason I am saying initialize it before is so there is one instance of its creation and lets say we want to change the description of the metric, we only have to change the one instance.
```
var tasksCounter =
ctx.longCounter("solr_executor_tasks", "Number of ExecutorService tasks");
this.submitted =
new AttributedLongCounter(
tasksCounter,
attrs.toBuilder().put(TYPE_ATTR, "submitted").build());
this.completed =
new AttributedLongCounter(
tasksCounter,
attrs.toBuilder().put(TYPE_ATTR, "completed").build());
```
--
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
Re: [PR] SOLR-17806: Migrate thread pool metrics to OTEL [solr]
Posted by "mlbiscoc (via GitHub)" <gi...@apache.org>.
mlbiscoc commented on code in PR #3519:
URL: https://github.com/apache/solr/pull/3519#discussion_r2308935183
##########
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:
Yes BUT only if the metric name has no differing attributes on the metric so then it would just be a single time series. Counter instrument doesn’t hold a single backing value otherwise. In this case, there are 2 distinct metrics with 2 unique attributes (`type`) differentiated from each under under same metric name. The AttributedLongCounter is initialized with 2 different sets of attributes thus creating 2 separate time series on the same metric name.
Thats why in tests we get in a psuedo structure:
```
MetricSnapshot {
Snapshot {
Metric name: solr_executor_tasks <- Shared metric name
Datapoints: { <- Unique time series
solr_executor_tasks{type=submitted} 0
solr_executor_tasks{type=completed} 0
}
}
}
```
[See here](https://github.com/apache/solr/blob/7ffda2ce9418671eea008d5423b5d16c479c0c20/solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedLongCounter.java#L37) the counter adds not to just the metric itself but the unique sets of attributes as well that was binded to the AttributedLongCounter.
--
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