mlbiscoc commented on code in PR #3417:
URL: https://github.com/apache/solr/pull/3417#discussion_r2216918815
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -262,27 +262,29 @@ public void initializeMetrics(
baseCommandsMetric,
baseAttributes.get().put(OPERATION_ATTR,
"deletes_by_query").build());
+ var baseCommitMetric =
+ solrMetricsContext.longCounter(
+ "solr_core_update_commit_operations", "Total number of commit
operations");
+
commitCommands =
- new AttributedLongUpDownCounter(
- baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"commits").build());
+ new AttributedLongCounter(
+ baseCommitMetric, baseAttributes.get().put(OPERATION_ATTR,
"commits").build());
optimizeCommands =
- new AttributedLongUpDownCounter(
- baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"optimize").build());
+ new AttributedLongCounter(
+ baseCommitMetric, baseAttributes.get().put(OPERATION_ATTR,
"optimize").build());
mergeIndexesCommands =
- new AttributedLongUpDownCounter(
- baseCommandsMetric, baseAttributes.get().put(OPERATION_ATTR,
"merges").build());
+ new AttributedLongCounter(
+ baseCommitMetric, baseAttributes.get().put(OPERATION_ATTR,
"merges").build());
Review Comment:
Changed to merge_indexes
##########
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:
Ok so I overhauled the util method a bit for getting metrics in
`SolrMetricTestUtils`. Maybe this is a bit better? If so, I will adopt this
paradigm on the other SolrCore PR I did and other PRs going forward.
##########
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 went ahead an created submitted and committed and removed the pending
metric. If you think this implementation is good, I'll fix up the tests
accordingly for the metric. Here is what it looks like. WDTY?
```
# HELP solr_core_update_committed_ops_total Total number of committed update
operations
# TYPE solr_core_update_committed_ops_total counter
solr_core_update_committed_ops_total{category="UPDATE",collection="gettingstarted",core="gettingstarted_shard1_replica_n1",ops="adds",otel_scope_name="org.apache.solr",replica="replica_n1",shard="shard1"}
1.0
solr_core_update_committed_ops_total{category="UPDATE",collection="gettingstarted",core="gettingstarted_shard1_replica_n1",ops="deletes_by_id",otel_scope_name="org.apache.solr",replica="replica_n1",shard="shard1"}
1.0
solr_core_update_committed_ops_total{category="UPDATE",collection="gettingstarted",core="gettingstarted_shard1_replica_n1",ops="deletes_by_query",otel_scope_name="org.apache.solr",replica="replica_n1",shard="shard1"}
1.0
# HELP solr_core_update_submitted_ops_total Total number of submitted update
operations
# TYPE solr_core_update_submitted_ops_total counter
solr_core_update_submitted_ops_total{category="UPDATE",collection="gettingstarted",core="gettingstarted_shard1_replica_n1",ops="adds",otel_scope_name="org.apache.solr",replica="replica_n1",shard="shard1"}
1.0
solr_core_update_submitted_ops_total{category="UPDATE",collection="gettingstarted",core="gettingstarted_shard1_replica_n1",ops="deletes_by_id",otel_scope_name="org.apache.solr",replica="replica_n1",shard="shard1"}
1.0
solr_core_update_submitted_ops_total{category="UPDATE",collection="gettingstarted",core="gettingstarted_shard1_replica_n1",ops="deletes_by_query",otel_scope_name="org.apache.solr",replica="replica_n1",shard="shard1"}
1.0
```
--
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]
