dsmiley commented on code in PR #3417:
URL: https://github.com/apache/solr/pull/3417#discussion_r2184425972
##########
solr/core/src/test/org/apache/solr/update/DirectUpdateHandlerTest.java:
##########
@@ -136,22 +118,39 @@ public void testBasics() {
assertQ(req("q", "id:5"), "//*[@numFound='0']");
assertQ(req("q", "id:6"), "//*[@numFound='0']");
- long newAdds = ((Gauge<Number>)
metrics.get(addsName)).getValue().longValue();
- long newCumulativeAdds = ((Meter)
metrics.get(cumulativeAddsName)).getCount();
- assertEquals("new adds", 2, newAdds - adds);
- assertEquals("new cumulative adds", 2, newCumulativeAdds - cumulativeAdds);
+ assertEquals(
+ "Did not have the expected number of pending adds",
Review Comment:
as you went through the effort of writng these messages -- fine. But I
wouldn't waste your time; it's optional.
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -89,20 +94,28 @@ public class DirectUpdateHandler2 extends UpdateHandler
// stats
LongAdder addCommands = new LongAdder();
- Meter addCommandsCumulative;
LongAdder deleteByIdCommands = new LongAdder();
- Meter deleteByIdCommandsCumulative;
LongAdder deleteByQueryCommands = new LongAdder();
- Meter deleteByQueryCommandsCumulative;
- Meter expungeDeleteCommands;
- Meter mergeIndexesCommands;
- Meter commitCommands;
- Meter splitCommands;
- Meter optimizeCommands;
- Meter rollbackCommands;
LongAdder numDocsPending = new LongAdder();
- LongAdder numErrors = new LongAdder();
- Meter numErrorsCumulative;
+
+ // Cumulative commands
+ AttributedLongUpDownCounter deleteByQueryCommandsCumulative;
Review Comment:
why are these "cumulative" things having a type that includes "Down";
wouldn't they never go down?
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricTestUtils.java:
##########
@@ -121,4 +127,36 @@ public String toString() {
}
};
}
+
+ public static DataPointSnapshot getDataPointSnapshot(
+ PrometheusMetricReader reader, String metricName, Labels labels) {
+ var met = reader.collect();
+ return reader.collect().stream()
+ .filter(ms -> ms.getMetadata().getPrometheusName().equals(metricName))
+ .findFirst()
+ .flatMap(
+ ms ->
+ ms.getDataPoints().stream()
+ .filter(dp -> dp.getLabels().hasSameValues(labels))
+ .findFirst())
+ .orElse(null);
+ }
+
+ public static Supplier<Labels.Builder> getCloudLabelsBase(SolrCore core) {
+ return () ->
+ Labels.builder()
+ .label("collection", "tlog_replica_test_only_leader_indexes")
+ .label("shard", "shard1")
Review Comment:
likewise. Look at the CloudDescriptor
##########
solr/webapp/web/js/angular/controllers/plugins.js:
##########
@@ -15,6 +15,8 @@
limitations under the License.
*/
+//NOCOMMIT: This plugin seems tied to the Admin UIs plugin management but is
tied to dropwizard metrics failing some tests.
+// This needs to change how it gets these metrics or we need to make a shim to
the /admin/plugins handler for this to support it
Review Comment:
Looking at PluginInfoHandler, I suppose we can continue to support that.
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -220,84 +232,139 @@ public void initializeMetrics(
} else {
this.solrMetricsContext = parentContext.getChildContext(this);
}
- commitCommands = solrMetricsContext.meter("commits",
getCategory().toString(), scope);
- solrMetricsContext.gauge(
- () -> commitTracker.getCommitCount(), true, "autoCommits",
getCategory().toString(), scope);
- solrMetricsContext.gauge(
- () -> softCommitTracker.getCommitCount(),
- true,
- "softAutoCommits",
- getCategory().toString(),
- scope);
- if (commitTracker.getDocsUpperBound() > 0) {
- solrMetricsContext.gauge(
- () -> commitTracker.getDocsUpperBound(),
- true,
- "autoCommitMaxDocs",
- getCategory().toString(),
- scope);
- }
- if (commitTracker.getTimeUpperBound() > 0) {
- solrMetricsContext.gauge(
- () -> "" + commitTracker.getTimeUpperBound() + "ms",
- true,
- "autoCommitMaxTime",
- getCategory().toString(),
- scope);
- }
- if (commitTracker.getTLogFileSizeUpperBound() > 0) {
- solrMetricsContext.gauge(
- () -> commitTracker.getTLogFileSizeUpperBound(),
- true,
- "autoCommitMaxSize",
- getCategory().toString(),
- scope);
- }
- if (softCommitTracker.getDocsUpperBound() > 0) {
- solrMetricsContext.gauge(
- () -> softCommitTracker.getDocsUpperBound(),
- true,
- "softAutoCommitMaxDocs",
- getCategory().toString(),
- scope);
- }
- if (softCommitTracker.getTimeUpperBound() > 0) {
- solrMetricsContext.gauge(
- () -> "" + softCommitTracker.getTimeUpperBound() + "ms",
- true,
- "softAutoCommitMaxTime",
- getCategory().toString(),
- scope);
- }
- optimizeCommands = solrMetricsContext.meter("optimizes",
getCategory().toString(), scope);
- rollbackCommands = solrMetricsContext.meter("rollbacks",
getCategory().toString(), scope);
- splitCommands = solrMetricsContext.meter("splits",
getCategory().toString(), scope);
- mergeIndexesCommands = solrMetricsContext.meter("merges",
getCategory().toString(), scope);
- expungeDeleteCommands =
- solrMetricsContext.meter("expungeDeletes", getCategory().toString(),
scope);
- solrMetricsContext.gauge(
- () -> numDocsPending.longValue(), true, "docsPending",
getCategory().toString(), scope);
- solrMetricsContext.gauge(
- () -> addCommands.longValue(), true, "adds", getCategory().toString(),
scope);
- solrMetricsContext.gauge(
- () -> deleteByIdCommands.longValue(), true, "deletesById",
getCategory().toString(), scope);
- solrMetricsContext.gauge(
- () -> deleteByQueryCommands.longValue(),
- true,
- "deletesByQuery",
- getCategory().toString(),
- scope);
- solrMetricsContext.gauge(
- () -> numErrors.longValue(), true, "errors", getCategory().toString(),
scope);
+
+ // NOCOMMIT: We do not need a scope attribute that just appends
scope="updateHandler" on all of
+ // these. The metric name
+ // provides this context already with _update_ in the metric name already.
We should see if we
+ // can omit this from SolrCoreMetricManager instead of directly removing
it from builder.
+ var baseAttributes =
+ MetricUtils.baseAttributesSupplier(
+ attributes.toBuilder()
+ .remove(AttributeKey.stringKey("scope"))
+ .put(AttributeKey.stringKey("category"),
getCategory().toString())
+ .build());
+
+ var baseCommandsMetric =
+ solrMetricsContext.longUpDownCounter(
+ "solr_metrics_core_update_operations_cumulative",
+ "Cumulative number of update commands processed. Metric can go
down from rollback command");
addCommandsCumulative =
- solrMetricsContext.meter("cumulativeAdds", getCategory().toString(),
scope);
+ new AttributedLongUpDownCounter(
+ baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"adds").build());
+
deleteByIdCommandsCumulative =
- solrMetricsContext.meter("cumulativeDeletesById",
getCategory().toString(), scope);
+ new AttributedLongUpDownCounter(
+ baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"deletes_by_id").build());
+
deleteByQueryCommandsCumulative =
- solrMetricsContext.meter("cumulativeDeletesByQuery",
getCategory().toString(), scope);
- numErrorsCumulative =
- solrMetricsContext.meter("cumulativeErrors", getCategory().toString(),
scope);
+ new AttributedLongUpDownCounter(
+ baseCommandsMetric,
+ baseAttributes.get().put(OPERATION_ATTR,
"deletes_by_query").build());
+
+ commitCommands =
+ new AttributedLongUpDownCounter(
+ baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"commits").build());
+
+ optimizeCommands =
+ new AttributedLongUpDownCounter(
+ baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"optimize").build());
+
+ mergeIndexesCommands =
+ new AttributedLongUpDownCounter(
+ baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"merges").build());
+
+ expungeDeleteCommands =
+ new AttributedLongUpDownCounter(
+ baseCommandsMetric,
+ baseAttributes.get().put(OPERATION_ATTR,
"expunge_deletes").build());
+
+ var baseMaintenanceMetric =
+ solrMetricsContext.longCounter(
+ "solr_metrics_core_update_maintenance_operations",
+ "Total number of maintenance operations");
+
+ rollbackCommands =
+ new AttributedLongCounter(
+ baseMaintenanceMetric, baseAttributes.get().put(OPERATION_ATTR,
"rollback").build());
+
+ splitCommands =
+ new AttributedLongCounter(
+ baseMaintenanceMetric, baseAttributes.get().put(OPERATION_ATTR,
"split").build());
+
+ var baseErrorsMetric =
+ solrMetricsContext.longCounter(
+ "solr_metrics_core_update_errors", "Total number of update
errors");
+
+ numErrorsCumulative = new AttributedLongCounter(baseErrorsMetric,
baseAttributes.get().build());
+
+ softAutoCommits =
+ solrMetricsContext.observableLongCounter(
+ "solr_metrics_core_update_auto_commits",
+ "Total number of auto commits",
+ (observableLongMeasurement -> {
+ observableLongMeasurement.record(
+ commitTracker.getCommitCount(),
+ baseAttributes.get().put(TYPE_ATTR, "auto_commits").build());
+ observableLongMeasurement.record(
+ softCommitTracker.getCommitCount(),
+ baseAttributes.get().put(TYPE_ATTR,
"soft_auto_commits").build());
+ }));
+
+ // NOCOMMIT: This might not need to be an obseravableLongGauge, but a
simple long gauge. Seems
+ // like a waste to constantly call this callback to get the latest value
if the upper bounds
+ // rarely change.
+ commitStats =
+ solrMetricsContext.observableLongGauge(
+ "solr_metrics_core_update_commit_stats",
+ "Metrics around commits",
+ (observableLongMeasurement -> {
+ if (commitTracker.getDocsUpperBound() > 0) {
+ observableLongMeasurement.record(
+ commitTracker.getDocsUpperBound(),
+ baseAttributes.get().put(TYPE_ATTR,
"auto_commit_max_docs").build());
+ }
+
+ if (commitTracker.getTLogFileSizeUpperBound() > 0) {
+ observableLongMeasurement.record(
+ commitTracker.getTLogFileSizeUpperBound(),
+ baseAttributes.get().put(TYPE_ATTR,
"auto_commit_max_size").build());
+ }
+ if (softCommitTracker.getDocsUpperBound() > 0) {
+ observableLongMeasurement.record(
+ softCommitTracker.getDocsUpperBound(),
+ baseAttributes.get().put(TYPE_ATTR,
"soft_auto_commit_max_docs").build());
+ }
+ if (commitTracker.getTimeUpperBound() > 0) {
+ observableLongMeasurement.record(
+ commitTracker.getTimeUpperBound(),
+ baseAttributes.get().put(TYPE_ATTR,
"auto_commit_max_time").build());
+ }
+ if (softCommitTracker.getTimeUpperBound() > 0) {
+ observableLongMeasurement.record(
+ softCommitTracker.getTimeUpperBound(),
+ baseAttributes.get().put(TYPE_ATTR,
"soft_auto_commit_max_time").build());
+ }
+ }));
+
+ updateStats =
+ solrMetricsContext.observableLongGauge(
+ "solr_metrics_core_update_pending_operations",
+ "Operations pending commit. Values get reset after commit",
Review Comment:
I wonder if it would be better to instead track "submitted", as separate
from "committed" operations? Much like the started and completed merges -- the
PR Kevin Liang is working on -- #3416
##########
solr/core/src/test/org/apache/solr/cloud/TestTlogReplica.java:
##########
@@ -570,32 +575,48 @@ public void testOnlyLeaderIndexes() throws Exception {
.process(cloudClient, collectionName);
{
- long docsPending =
- (long)
- getSolrCore(true)
- .get(0)
- .getSolrMetricsContext()
- .getMetricRegistry()
- .getGauges()
- .get("UPDATE.updateHandler.docsPending")
- .getValue();
+ SolrCore core = getSolrCore(true).getFirst();
+ var reader =
+ core.getSolrMetricsContext()
+ .getMetricManager()
+ .getPrometheusMetricReader(
+
getSolrCore(true).getFirst().getCoreMetricManager().getRegistryName());
+ var actual =
+ (GaugeSnapshot.GaugeDataPointSnapshot)
+ SolrMetricTestUtils.getDataPointSnapshot(
+ reader,
+ "solr_metrics_core_update_pending_operations",
+ SolrMetricTestUtils.getCloudLabelsBase(core)
+ .get()
+ .label("category", "UPDATE")
+ .label("operation", "docs_pending")
+ .build());
assertEquals(
"Expected 4 docs are pending in core " +
getSolrCore(true).get(0).getCoreDescriptor(),
4,
- docsPending);
+ (long) actual.getValue());
}
for (SolrCore solrCore : getSolrCore(false)) {
- long docsPending =
- (long)
- solrCore
- .getSolrMetricsContext()
- .getMetricRegistry()
- .getGauges()
- .get("UPDATE.updateHandler.docsPending")
- .getValue();
+ var reader =
+ solrCore
+ .getSolrMetricsContext()
+ .getMetricManager()
+
.getPrometheusMetricReader(solrCore.getCoreMetricManager().getRegistryName());
+ var actual =
Review Comment:
this statement is lacking some elegance. If this pattern repeats itself; we
should seek an improvement
##########
solr/core/src/test/org/apache/solr/metrics/SolrMetricTestUtils.java:
##########
@@ -121,4 +127,36 @@ public String toString() {
}
};
}
+
+ public static DataPointSnapshot getDataPointSnapshot(
+ PrometheusMetricReader reader, String metricName, Labels labels) {
+ var met = reader.collect();
+ return reader.collect().stream()
+ .filter(ms -> ms.getMetadata().getPrometheusName().equals(metricName))
+ .findFirst()
+ .flatMap(
+ ms ->
+ ms.getDataPoints().stream()
+ .filter(dp -> dp.getLabels().hasSameValues(labels))
+ .findFirst())
+ .orElse(null);
+ }
+
+ public static Supplier<Labels.Builder> getCloudLabelsBase(SolrCore core) {
+ return () ->
+ Labels.builder()
+ .label("collection", "tlog_replica_test_only_leader_indexes")
Review Comment:
Look at the CloudDescriptor
##########
solr/core/src/test/org/apache/solr/update/DirectUpdateHandlerTest.java:
##########
@@ -160,20 +159,38 @@ public void testBasics() {
// now delete one
assertU(delI("5"));
- long newDelsI = ((Gauge<Number>)
metrics.get(delsIName)).getValue().longValue();
- long newCumulativeDelsI = ((Meter)
metrics.get(cumulativeDelsIName)).getCount();
- assertEquals("new delsI", 1, newDelsI);
- assertEquals("new cumulative delsI", 1, newCumulativeDelsI -
cumulativeDelsI);
+ assertEquals(
+ "Did not have the expected number of pending deletes_by_id",
+ 1,
+ getGaugeOpDatapoint(reader,
"solr_metrics_core_update_pending_operations", "deletes_by_id")
+ .getValue(),
+ 0.0);
+ assertEquals(
+ "Did not have the expected number of cumulative deletes_by_id",
+ 1,
+ getGaugeOpDatapoint(
+ reader, "solr_metrics_core_update_operations_cumulative",
"deletes_by_id")
Review Comment:
just a quick observation -- the "metrics" part of these names seems
redundant; no?
The "solr" part is I know needed to differentiate the JVM & Jetty.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
