mlbiscoc commented on code in PR #3417:
URL: https://github.com/apache/solr/pull/3417#discussion_r2195661683
##########
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:
Hmmm maybe that is better. Maybe I'll reduce this to only docs_pending for
this gauge instead then we'll move to submitted and committed increasing
counters. I'll refactor this around and see how it looks.
##########
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:
They go down from a rollback command.
```
addCommandsCumulative.add(-addCommands.sumThenReset());
deleteByIdCommandsCumulative.add(-deleteByIdCommands.sumThenReset());
deleteByQueryCommandsCumulative.add(-deleteByQueryCommands.sumThenReset());
```
Although the maintenance operations below only go up, so we can move that to
an normal counter actually.
##########
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:
Yeah but mostly kept them as it was the way the prometheus exporter prefixed
all it's metrics. Definitely no need to keep them if we don't want. I removed
them in the latest commit but will remove it from the other metrics in a
separate PR.
--
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]
