dsmiley commented on code in PR #3430:
URL: https://github.com/apache/solr/pull/3430#discussion_r2217361565
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1305,92 +1321,129 @@ private SolrCoreMetricManager
initCoreMetricManager(SolrConfig config) {
return coreMetricManager;
}
- // TODO SOLR-17458: Migrate to Otel
@Override
public void initializeMetrics(
SolrMetricsContext parentContext, Attributes attributes, String scope) {
- newSearcherCounter = parentContext.counter("new",
Category.SEARCHER.toString());
- newSearcherTimer = parentContext.timer("time",
Category.SEARCHER.toString(), "new");
- newSearcherWarmupTimer = parentContext.timer("warmup",
Category.SEARCHER.toString(), "new");
+
+ var searcherAttributesBuilder =
+ MetricUtils.baseAttributesSupplier(
+ attributes.toBuilder().put(CATEGORY_ATTR,
Category.SEARCHER.toString()).build());
+ var gaugeCoreAttributesBuilder =
+ MetricUtils.baseAttributesSupplier(
+ attributes.toBuilder().put(CATEGORY_ATTR,
Category.CORE.toString()).build());
+
+ Attributes baseSearcherAttributes =
searcherAttributesBuilder.get().build();
+ Attributes baseGaugeCoreAttributes =
gaugeCoreAttributesBuilder.get().build();
+
+ var baseSearcherTimerMetric =
+ parentContext.longHistogram("solr_searcher_timer", "Timer for opening
new searchers", "ms");
+
+ newSearcherCounter =
+ new AttributedLongCounter(
+ parentContext.longCounter(
+ "solr_core_new_searcher", "Total number of new searchers
opened"),
Review Comment:
I see you are putting adjectives first (english) but I feel the "searcher"
is a cateogry that should come first
##########
solr/core/src/java/org/apache/solr/metrics/reporters/SolrSlf4jReporter.java:
##########
@@ -49,6 +49,8 @@
* <code>solr.jvm</code>, <code>solr.core</code>, etc
* </ul>
*/
+// NOCOMMIT: Will we still be supporting this? Need to understand how Slf4j
works and if it is even
Review Comment:
I'm very suspicious anyone is using this. IMO, it's a better trade-off to
slim down Solr then retain this.
We'll need to remember to be clear to users on what we are dropping and to
solicit feedback.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -2586,7 +2639,7 @@ public RefCounted<SolrIndexSearcher> getSearcher(
boolean success = false;
openSearcherLock.lock();
- Timer.Context timerContext = newSearcherTimer.time();
+ AttributedLongTimer.MetricTimer timerContext = newSearcherTimer.start();
Review Comment:
I wish RTimer was AutoCloseable so we could do a nice try-finally pattern.
Any way; for another time.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1305,92 +1321,129 @@ private SolrCoreMetricManager
initCoreMetricManager(SolrConfig config) {
return coreMetricManager;
}
- // TODO SOLR-17458: Migrate to Otel
@Override
public void initializeMetrics(
SolrMetricsContext parentContext, Attributes attributes, String scope) {
- newSearcherCounter = parentContext.counter("new",
Category.SEARCHER.toString());
- newSearcherTimer = parentContext.timer("time",
Category.SEARCHER.toString(), "new");
- newSearcherWarmupTimer = parentContext.timer("warmup",
Category.SEARCHER.toString(), "new");
+
+ var searcherAttributesBuilder =
+ MetricUtils.baseAttributesSupplier(
+ attributes.toBuilder().put(CATEGORY_ATTR,
Category.SEARCHER.toString()).build());
+ var gaugeCoreAttributesBuilder =
+ MetricUtils.baseAttributesSupplier(
+ attributes.toBuilder().put(CATEGORY_ATTR,
Category.CORE.toString()).build());
+
+ Attributes baseSearcherAttributes =
searcherAttributesBuilder.get().build();
+ Attributes baseGaugeCoreAttributes =
gaugeCoreAttributesBuilder.get().build();
+
+ var baseSearcherTimerMetric =
+ parentContext.longHistogram("solr_searcher_timer", "Timer for opening
new searchers", "ms");
+
+ newSearcherCounter =
+ new AttributedLongCounter(
+ parentContext.longCounter(
+ "solr_core_new_searcher", "Total number of new searchers
opened"),
+ baseSearcherAttributes);
+
newSearcherMaxReachedCounter =
- parentContext.counter("maxReached", Category.SEARCHER.toString(),
"new");
+ new AttributedLongCounter(
+ parentContext.longCounter(
+ "solr_core_max_reached_searcher",
+ "Total number of maximum number of concurrent warming
searchers reached"),
+ baseSearcherAttributes);
+
newSearcherOtherErrorsCounter =
- parentContext.counter("errors", Category.SEARCHER.toString(), "new");
-
- parentContext.gauge(
- () -> name == null ? parentContext.nullString() : name,
- true,
- "coreName",
- Category.CORE.toString());
- parentContext.gauge(() -> startTime, true, "startTime",
Category.CORE.toString());
+ new AttributedLongCounter(
+ parentContext.longCounter(
+ "solr_core_searcher_errors", "Total number of searcher
errors"),
+ baseSearcherAttributes);
+
+ newSearcherTimer =
+ new AttributedLongTimer(
+ baseSearcherTimerMetric,
searcherAttributesBuilder.get().put(TYPE_ATTR, "new").build());
+
+ newSearcherWarmupTimer =
+ new AttributedLongTimer(
+ baseSearcherTimerMetric,
+ searcherAttributesBuilder.get().put(TYPE_ATTR, "warmup").build());
+
parentContext.gauge(() -> getOpenCount(), true, "refCount",
Category.CORE.toString());
- parentContext.gauge(
- () -> getInstancePath().toString(), true, "instanceDir",
Category.CORE.toString());
- parentContext.gauge(
- () -> isClosed() ? parentContext.nullString() : getIndexDir(),
- true,
- "indexDir",
- Category.CORE.toString());
- parentContext.gauge(
- () -> isClosed() ? parentContext.nullNumber() : getIndexSize(),
- true,
- "sizeInBytes",
- Category.INDEX.toString());
- parentContext.gauge(
- () -> isClosed() ? parentContext.nullString() :
NumberUtils.readableSize(getIndexSize()),
- true,
- "size",
- Category.INDEX.toString());
- parentContext.gauge(
- () -> isReady() ? getSegmentCount() : parentContext.nullNumber(),
- true,
- "segments",
- Category.INDEX.toString());
-
- final CloudDescriptor cd = getCoreDescriptor().getCloudDescriptor();
- if (cd != null) {
- parentContext.gauge(cd::getCollectionName, true, "collection",
Category.CORE.toString());
- parentContext.gauge(cd::getShardId, true, "shard",
Category.CORE.toString());
- parentContext.gauge(cd::isLeader, true, "isLeader",
Category.CORE.toString());
- parentContext.gauge(
- () -> String.valueOf(cd.getLastPublished()),
- true,
- "replicaState",
- Category.CORE.toString());
- }
-
- // initialize disk total / free metrics
- Path dataDirPath = Path.of(dataDir);
- parentContext.gauge(
- () -> {
+
+ parentContext.observableLongGauge(
+ "solr_core_ref_count",
+ "The current number of active references to a Solr core",
+ (observableLongMeasurement -> {
+ observableLongMeasurement.record(getOpenCount(),
baseGaugeCoreAttributes);
+ }));
+
+ parentContext.observableLongGauge(
+ "solr_core_disk_space",
+ "Solr core disk space metrics",
+ (observableLongMeasurement -> {
+
+ // initialize disk total / free metrics
+ Path dataDirPath = Path.of(dataDir);
+ var totalSpaceAttributes =
+ gaugeCoreAttributesBuilder.get().put(TYPE_ATTR,
"total_space").build();
+ var usableSpaceAttributes =
+ gaugeCoreAttributesBuilder.get().put(TYPE_ATTR,
"usable_space").build();
try {
- return Files.getFileStore(dataDirPath).getTotalSpace();
+ observableLongMeasurement.record(
+ Files.getFileStore(dataDirPath).getTotalSpace(),
totalSpaceAttributes);
} catch (IOException e) {
- return 0L;
+ observableLongMeasurement.record(0L, totalSpaceAttributes);
}
- },
- true,
- "totalSpace",
- Category.CORE.toString(),
- "fs");
- parentContext.gauge(
- () -> {
try {
- return Files.getFileStore(dataDirPath).getUsableSpace();
+ observableLongMeasurement.record(
+ Files.getFileStore(dataDirPath).getUsableSpace(),
usableSpaceAttributes);
} catch (IOException e) {
- return 0L;
+ observableLongMeasurement.record(0L, usableSpaceAttributes);
}
- },
- true,
- "usableSpace",
- Category.CORE.toString(),
- "fs");
- parentContext.gauge(
- () -> dataDirPath.toAbsolutePath().toString(),
- true,
- "path",
- Category.CORE.toString(),
- "fs");
+ }),
+ "By");
+
+ parentContext.observableLongGauge(
+ "solr_core_index_size",
+ "Index size for a Solr core",
+ (observableLongMeasurement -> {
+ if (!isClosed())
+ observableLongMeasurement.record(getIndexSize(),
baseGaugeCoreAttributes);
+ }),
+ "By");
+
+ parentContext.observableLongGauge(
+ "solr_core_segment_count",
+ "Number of segments in a Solr core",
+ (observableLongMeasurement -> {
+ if (isReady())
+ observableLongMeasurement.record(getSegmentCount(),
baseGaugeCoreAttributes);
+ }));
+
+ // NOCOMMIT: Do we need these start_time metrics? I think at minimum it
should be optional
Review Comment:
Start time of Solr, a core, and a searcher, are all useful IMO, but not
created times of other stuff in Solr (e.g. not of a handler). Does a timestamp
get exposed in a standard way with OTEL?
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1305,92 +1321,129 @@ private SolrCoreMetricManager
initCoreMetricManager(SolrConfig config) {
return coreMetricManager;
}
- // TODO SOLR-17458: Migrate to Otel
@Override
public void initializeMetrics(
SolrMetricsContext parentContext, Attributes attributes, String scope) {
- newSearcherCounter = parentContext.counter("new",
Category.SEARCHER.toString());
- newSearcherTimer = parentContext.timer("time",
Category.SEARCHER.toString(), "new");
- newSearcherWarmupTimer = parentContext.timer("warmup",
Category.SEARCHER.toString(), "new");
+
+ var searcherAttributesBuilder =
+ MetricUtils.baseAttributesSupplier(
+ attributes.toBuilder().put(CATEGORY_ATTR,
Category.SEARCHER.toString()).build());
+ var gaugeCoreAttributesBuilder =
+ MetricUtils.baseAttributesSupplier(
+ attributes.toBuilder().put(CATEGORY_ATTR,
Category.CORE.toString()).build());
+
+ Attributes baseSearcherAttributes =
searcherAttributesBuilder.get().build();
+ Attributes baseGaugeCoreAttributes =
gaugeCoreAttributesBuilder.get().build();
+
+ var baseSearcherTimerMetric =
+ parentContext.longHistogram("solr_searcher_timer", "Timer for opening
new searchers", "ms");
+
+ newSearcherCounter =
+ new AttributedLongCounter(
+ parentContext.longCounter(
+ "solr_core_new_searcher", "Total number of new searchers
opened"),
+ baseSearcherAttributes);
+
newSearcherMaxReachedCounter =
- parentContext.counter("maxReached", Category.SEARCHER.toString(),
"new");
+ new AttributedLongCounter(
+ parentContext.longCounter(
+ "solr_core_max_reached_searcher",
+ "Total number of maximum number of concurrent warming
searchers reached"),
Review Comment:
"number of" repeats.
IMO the name of this should be "solr_core_searcher_warming_max"
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1632,7 +1685,7 @@ public StatsCache createStatsCache() {
}
/** Load the request processors */
- private Map<String, UpdateRequestProcessorChain> loadUpdateProcessorChains()
{
+ private Map<String, UpdateRequestProcessorChain> loadUpdateProcessorChains()
throws IOException {
Review Comment:
unexpected change
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricsIntegrationTest.java:
##########
@@ -49,6 +50,10 @@
import org.junit.Before;
import org.junit.Test;
+// NOCOMMIT: Test fails because of the @After assert on index path. Was going
to migrate to just
+// check the registry for the core is deleted but this test does a rename
operation which otel has
+// not addressed yet. Need to migrate the rename operation to otel first.
[email protected](bugUrl =
"https://issues.apache.org/jira/browse/SOLR-17458";)
public class SolrMetricsIntegrationTest extends SolrTestCaseJ4 {
Review Comment:
I'm not aware of us dropping support for that. You might raise this
suggestion if you find supporting it too onerous.
##########
solr/core/src/test/org/apache/solr/core/SolrCoreTest.java:
##########
@@ -354,15 +353,19 @@ public void testCoreInitDeadlockMetrics() throws
Exception {
executor.execute(
() -> {
while (!created.get()) {
- var metrics =
- metricManager.getMetrics(
- "solr.core." + coreName,
-
MetricFilter.startsWith(SolrInfoBean.Category.INDEX.toString()));
- for (var m : metrics.values()) {
- if (m instanceof Gauge) {
- var v = ((Gauge<?>) m).getValue();
- atLeastOnePoll.compareAndSet(false, v != null);
- }
+ var reader =
SolrMetricTestUtils.getPrometheusMetricReader(h.getCore());
+
+ var datapoint =
+ SolrMetricTestUtils.getGaugeOpDatapoint(
+ reader,
+ "solr_core_index_size_bytes",
+ SolrMetricTestUtils.getStandaloneLabelsBase(h.getCore())
+ .get()
Review Comment:
looks like a supplier wrapper... not sure why getGaugeOpDatapoint wants a
Supplier
##########
solr/core/src/java/org/apache/solr/metrics/reporters/SolrJmxReporter.java:
##########
@@ -37,6 +37,8 @@
* <p>NOTE: {@link com.codahale.metrics.jmx.JmxReporter} that this class uses
exports only newly
* added metrics (it doesn't process already existing metrics in a registry)
*/
+// NOCOMMIT: This JMX reporter looks to be wrapped over a Dropwizard registry.
We need to migrate
+// this to OTEL. Maybe we can look at available otel shims for JMX?
public class SolrJmxReporter extends FilteringSolrMetricReporter {
Review Comment:
Hmmm; that sounds awkward. I'm unsure how relevant JMX for metrics is
nowadays. I'm more inclined to drop it.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1305,92 +1321,129 @@ private SolrCoreMetricManager
initCoreMetricManager(SolrConfig config) {
return coreMetricManager;
}
- // TODO SOLR-17458: Migrate to Otel
@Override
public void initializeMetrics(
SolrMetricsContext parentContext, Attributes attributes, String scope) {
- newSearcherCounter = parentContext.counter("new",
Category.SEARCHER.toString());
- newSearcherTimer = parentContext.timer("time",
Category.SEARCHER.toString(), "new");
- newSearcherWarmupTimer = parentContext.timer("warmup",
Category.SEARCHER.toString(), "new");
+
+ var searcherAttributesBuilder =
Review Comment:
These 2 vars are suppliers of a builder. What's the point of the Supplier
wrapper?
--
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]
