mlbiscoc commented on code in PR #3568:
URL: https://github.com/apache/solr/pull/3568#discussion_r2311262088
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -177,66 +189,93 @@ private SolrIndexWriter(
} else {
mergeTotals = false;
}
+ String coreName = core.getCoreDescriptor().getName();
+ var baseAttributesBuilder =
+ Attributes.builder()
+ .put(CATEGORY_ATTR, SolrInfoBean.Category.INDEX.toString())
+ .put(CORE_ATTR, coreName);
+ if (core.getCoreContainer().isZooKeeperAware()) {
+ String collectionName = core.getCoreDescriptor().getCollectionName();
+ baseAttributesBuilder
+ .put(COLLECTION_ATTR, collectionName)
+ .put(SHARD_ATTR,
core.getCoreDescriptor().getCloudDescriptor().getShardId())
+ .put(REPLICA_ATTR, Utils.parseMetricsReplicaName(collectionName,
coreName));
+ }
+ var baseAttributes = baseAttributesBuilder.build();
if (mergeDetails) {
mergeTotals = true; // override
majorMergedDocs =
- solrMetricsContext.meter(
- "docs", SolrInfoBean.Category.INDEX.toString(), "merge",
"major");
+ new AttributedLongCounter(
+ solrMetricsContext.longCounter(
+ "solr_indexriter_major_merged_docs",
+ "Number of documents merged while merging segments above
the majorMergeDocs threshold"),
+ baseAttributes);
majorDeletedDocs =
- solrMetricsContext.meter(
- "deletedDocs", SolrInfoBean.Category.INDEX.toString(),
"merge", "major");
+ new AttributedLongCounter(
+ solrMetricsContext.longCounter(
+ "solr_indexriter_major_deleted_docs",
+ "Number of deleted documents that were expunged while
merging segments above the majorMergeDocs threshold"),
+ baseAttributes);
}
if (mergeTotals) {
minorMerge =
- solrMetricsContext.timer("minor",
SolrInfoBean.Category.INDEX.toString(), "merge");
+ new AttributedLongTimer(
+ solrMetricsContext.longHistogram(
+ "solr_indexwriter_minor_merge",
+ "Time spent merging segments below the majorMergeDocs
threshold",
+ OtelUnit.MILLISECONDS),
+ baseAttributes);
majorMerge =
- solrMetricsContext.timer("major",
SolrInfoBean.Category.INDEX.toString(), "merge");
+ new AttributedLongTimer(
+ solrMetricsContext.longHistogram(
+ "solr_indexwriter_major_merge",
+ "Time spent merging segments above the majorMergeDocs
threshold",
+ OtelUnit.MILLISECONDS),
+ baseAttributes);
mergeErrors =
- solrMetricsContext.counter("errors",
SolrInfoBean.Category.INDEX.toString(), "merge");
+ new AttributedLongCounter(
+ solrMetricsContext.longCounter(
+ "solr_indexwriter_merge_errors",
+ "Number of merge errors"),
+ baseAttributes);
String tag = core.getMetricTag();
- solrMetricsContext.gauge(
- () -> runningMajorMerges.get(),
- true,
- "running",
- SolrInfoBean.Category.INDEX.toString(),
- "merge",
- "major");
- solrMetricsContext.gauge(
- () -> runningMinorMerges.get(),
- true,
- "running",
- SolrInfoBean.Category.INDEX.toString(),
- "merge",
- "minor");
- solrMetricsContext.gauge(
- () -> runningMajorMergesDocs.get(),
- true,
- "running.docs",
- SolrInfoBean.Category.INDEX.toString(),
- "merge",
- "major");
- solrMetricsContext.gauge(
- () -> runningMinorMergesDocs.get(),
- true,
- "running.docs",
- SolrInfoBean.Category.INDEX.toString(),
- "merge",
- "minor");
- solrMetricsContext.gauge(
- () -> runningMajorMergesSegments.get(),
- true,
- "running.segments",
- SolrInfoBean.Category.INDEX.toString(),
- "merge",
- "major");
- solrMetricsContext.gauge(
- () -> runningMinorMergesSegments.get(),
- true,
- "running.segments",
- SolrInfoBean.Category.INDEX.toString(),
- "merge",
- "minor");
- flushMeter = solrMetricsContext.meter("flush",
SolrInfoBean.Category.INDEX.toString());
+ majorMergeStats =
+ solrMetricsContext.observableLongGauge(
+ "solr_indexwriter_major_merge_stats",
+ "Metrics around currently running segment merges above the
majorMergeDocs threshold",
+ (observableLongMeasurement -> {
+ observableLongMeasurement.record(
+ runningMajorMerges.get(),
+ baseAttributes.toBuilder().put(TYPE_ATTR,
"running").build());
+ observableLongMeasurement.record(
+ runningMajorMergesDocs.get(),
+ baseAttributes.toBuilder().put(TYPE_ATTR,
"running_docs").build());
+ observableLongMeasurement.record(
+ runningMajorMergesSegments.get(),
+ baseAttributes.toBuilder().put(TYPE_ATTR,
"running_segments").build());
+ }));
+ minorMergeStats =
+ solrMetricsContext.observableLongGauge(
+ "solr_indexwriter_minor_merge_stats",
+ "Metrics around currently running segment merges below the
majorMergeDocs threshold",
+ (observableLongMeasurement -> {
+ observableLongMeasurement.record(
+ runningMinorMerges.get(),
+ baseAttributes.toBuilder().put(TYPE_ATTR,
"running").build());
+ observableLongMeasurement.record(
+ runningMinorMergesDocs.get(),
+ baseAttributes.toBuilder().put(TYPE_ATTR,
"running_docs").build());
+ observableLongMeasurement.record(
+ runningMinorMergesSegments.get(),
+ baseAttributes.toBuilder().put(TYPE_ATTR,
"running_segments").build());
+ }));
Review Comment:
I'm thinking that both of these can just be `solr_indexwriter_merge_stats`
then add `major/minor` as an attribute. Also I know the number of documents for
a major and minor merge threshold is decided by the configuration. Would we
able to initialize those numbers in the metric description so users can see?
Then we can also wrap these all under 1 single gauge instead of making 2.
##########
solr/core/src/java/org/apache/solr/update/SolrIndexWriter.java:
##########
@@ -87,6 +97,8 @@ public class SolrIndexWriter extends IndexWriter {
private final AtomicInteger runningMinorMergesSegments = new AtomicInteger();
private final AtomicLong runningMajorMergesDocs = new AtomicLong();
private final AtomicLong runningMinorMergesDocs = new AtomicLong();
+ private ObservableLongGauge majorMergeStats;
+ private ObservableLongGauge minorMergeStats;
Review Comment:
Lets close the observables after with
[IOUtils](https://github.com/apache/solr/blob/4472522e7cf2ffbd820b2c307c80dad81055f0be/solr/solrj/src/java/org/apache/solr/common/util/IOUtils.java#L26)
##########
solr/core/src/test/org/apache/solr/update/SolrIndexMetricsTest.java:
##########
@@ -119,27 +136,50 @@ public void testIndexMetricsWithDetails() throws
Exception {
addDocs();
- MetricRegistry registry =
- h.getCoreContainer()
- .getMetricManager()
- .registry(h.getCore().getCoreMetricManager().getRegistryName());
- assertNotNull(registry);
-
- Map<String, Metric> metrics = registry.getMetrics();
-
- assertTrue(
- metrics.entrySet().stream().filter(e ->
e.getKey().startsWith("INDEX")).count() >= 12);
+ try (SolrCore core = h.getCoreContainer().getCore("collection1")) {
+ var prometheusMetricReader =
SolrMetricTestUtils.getPrometheusMetricReader(core);
+ assertNotNull(prometheusMetricReader);
+ MetricSnapshots otelMetrics = prometheusMetricReader.collect();
+ assertTrue("Metrics count: " + otelMetrics.size(), otelMetrics.size() >=
20);
+
+ // check basic index meters
+ var minorMergeTimer =
+ SolrMetricTestUtils.getHistogramDatapoint(
+ core,
+ "solr_indexwriter_minor_merge_milliseconds",
+ SolrMetricTestUtils.newStandaloneLabelsBuilder(core)
+ .label(CATEGORY_ATTR.toString(),
SolrInfoBean.Category.INDEX.toString())
+ .build());
+ assertTrue("minorMerge: " + minorMergeTimer.getCount(),
minorMergeTimer.getCount() >= 3);
+ var majorMergeTimer =
+ SolrMetricTestUtils.getHistogramDatapoint(
+ core,
+ "solr_indexwriter_major_merge_milliseconds",
+ SolrMetricTestUtils.newStandaloneLabelsBuilder(core)
+ .label(CATEGORY_ATTR.toString(),
SolrInfoBean.Category.INDEX.toString())
+ .build());
+ // major merge timer should have a value of 0, and because 0 values are
not reported, no datapoint is available
+ assertNull("majorMergeTimer", majorMergeTimer);
- // check basic index meters
- Timer timer = (Timer) metrics.get("INDEX.merge.minor");
- assertTrue("minorMerge: " + timer.getCount(), timer.getCount() >= 3);
- timer = (Timer) metrics.get("INDEX.merge.major");
- assertEquals("majorMerge: " + timer.getCount(), 0, timer.getCount());
- // check detailed meters
- Meter meter = (Meter) metrics.get("INDEX.merge.major.docs");
- assertEquals("majorMergeDocs: " + meter.getCount(), 0, meter.getCount());
Review Comment:
So I know these metric initially checked for 0 and we assert for null which
is good, but could we maybe add to the test to also trigger a major merge and
minor merge and assert the metric does exist after with a value > 0? Because
then if someone were to just delete all the initialize metrics on
SolrIndexWriter, this test would still technically pass.
--
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]
