dsmiley commented on code in PR #3826:
URL: https://github.com/apache/solr/pull/3826#discussion_r2483853497
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1370,13 +1366,19 @@ public void initializeMetrics(SolrMetricsContext
parentContext, Attributes attri
newSearcherTimer =
new AttributedLongTimer(
- baseSearcherTimerMetric,
- Attributes.builder().putAll(baseSearcherAttributes).put(TYPE_ATTR,
"new").build());
Review Comment:
To me, it seemed less organized to differentiate "warmup" from "open" by an
attribute, so I separated them.
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java:
##########
@@ -36,16 +34,19 @@
* Factory for creating metrics instruments that can write to either single or
dual registries (core
* and node).
*/
+// TODO consider making this a base abstraction with a simple impl and another
"Dual" one.
public class AttributedInstrumentFactory {
+ // These attributes are on a core but don't want to aggregate them.
private static final Set<AttributeKey<?>> FILTER_ATTRS_SET =
- Set.of(COLLECTION_ATTR, CORE_ATTR, SHARD_ATTR, REPLICA_TYPE_ATTR,
INTERNAL_ATTR);
Review Comment:
There were too many here IMO; we just want to exclude the ones identifying
in nature. Now we can differentiate aggregate between internal vs not -- very
useful.
##########
solr/core/src/java/org/apache/solr/util/stats/MetricUtils.java:
##########
@@ -20,16 +20,14 @@
import com.codahale.metrics.Timer;
import java.lang.invoke.MethodHandles;
import java.lang.management.OperatingSystemMXBean;
-import java.util.concurrent.ExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;
import org.apache.solr.common.util.NamedList;
-import org.apache.solr.core.SolrInfoBean;
-import org.apache.solr.metrics.SolrMetricsContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/** Metrics specific utility functions. */
+@Deprecated
Review Comment:
All that remains are a couple things specific to single callers that can
just as well adopt this stuff.
##########
solr/core/src/java/org/apache/solr/metrics/otel/instruments/AttributedInstrumentFactory.java:
##########
@@ -36,16 +34,19 @@
* Factory for creating metrics instruments that can write to either single or
dual registries (core
* and node).
*/
+// TODO consider making this a base abstraction with a simple impl and another
"Dual" one.
public class AttributedInstrumentFactory {
+ // These attributes are on a core but don't want to aggregate them.
private static final Set<AttributeKey<?>> FILTER_ATTRS_SET =
- Set.of(COLLECTION_ATTR, CORE_ATTR, SHARD_ATTR, REPLICA_TYPE_ATTR,
INTERNAL_ATTR);
+ Set.of(COLLECTION_ATTR, CORE_ATTR, SHARD_ATTR);
+
private final SolrMetricsContext primaryMetricsContext;
private final Attributes primaryAttributes;
private final boolean aggregateToNodeRegistry;
private final boolean primaryIsNodeRegistry;
- private SolrMetricsContext nodeMetricsContext = null;
- private Attributes nodeAttributes = null;
+ private final SolrMetricsContext nodeMetricsContext;
+ private final Attributes nodeAttributes;
Review Comment:
just making fields final; I like final fields
##########
solr/core/src/java/org/apache/solr/metrics/SolrMetricsContext.java:
##########
@@ -253,6 +256,16 @@ public BatchCallback batchCallback(
return batchCallback;
}
+ /** Returns an instrumented wrapper over the given executor service. */
+ public ExecutorService instrumentedExecutorService(
Review Comment:
Moved this here from MetricUtils. Feels like a good home to produce metrics
stuff including wrappers like this. Also will be a great place to eventually
check no-op scenario and return the input directly :-) (no overhead whatsoever)
--
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]
