mlbiscoc commented on code in PR #4106:
URL: https://github.com/apache/solr/pull/4106#discussion_r2788486096
##########
solr/modules/cross-dc/src/java/org/apache/solr/crossdc/update/processor/ProducerMetrics.java:
##########
@@ -45,6 +53,22 @@ public ProducerMetrics(SolrMetricsContext
solrMetricsContext, SolrCore solrCore)
solrMetricsContext.longCounter(
"solr_core_crossdc_producer_submitted",
"The number of documents submitted to the Kafka topic (success or
error)");
+ var localSubmittedAdd =
+ solrMetricsContext.longCounter(
+ "solr_core_crossdc_producer_submitted_add",
+ "The number of add requests submitted to the Kafka topic (success
or error)");
+ var localSubmittedDbi =
+ solrMetricsContext.longCounter(
+ "solr_core_crossdc_producer_submitted_delete_by_id",
+ "The number of Delete-By-Id requests submitted to the Kafka topic
(success or error)");
+ var localSubmittedDbq =
+ solrMetricsContext.longCounter(
+ "solr_core_crossdc_producer_submitted_delete_by_query",
+ "The number of Delete-By-Query requests submitted to the Kafka
topic (success or error)");
+ var localSubmittedCommit =
+ solrMetricsContext.longCounter(
+ "solr_core_crossdc_producer_submitted_commit",
+ "The number of standalone Commit requests submitted to the Kafka
topic (success or error)");
Review Comment:
This seems wrong based on descriptions.
`solr_core_crossdc_producer_submitted` says it is the number of documents. Then
the name should probably be `solr_core_crossdc_producer_submitted_docs`. Then
the rest of these should be `solr_core_crossdc_producer_submitted_requests` and
instead of appending the suffix of different metric names that are redundant
the `add`/`deletebyid` etc should be a label of `type` or `operation`. Then
success or error should be attribute key of `result`. This would be much easier
for aggregation query languages like promql in my opinion and looks like so:
```
# HELP
# TYP
solr_core_crossdc_producer_submitted_requests{operation="add",result="success"}
123
solr_core_crossdc_producer_submitted_requests{operation="add",result="error"} 1
solr_core_crossdc_producer_submitted_requests{operation="delete_by_id",result="success"}
1
solr_core_crossdc_producer_submitted_requests{operation="delete_by_id",result="error"}
0
solr_core_crossdc_producer_submitted_requests{operation="delete_by_query",result="success"}
10
solr_core_crossdc_producer_submitted_requests{operation="delete_by_query",result="error"}
1
solr_core_crossdc_producer_submitted_requests{operation="commit",result="success"}
100
solr_core_crossdc_producer_submitted_requests{operation="commit",result="error"}
0
# HELP
# TYPE
solr_core_crossdc_producer_submitted_docs{result="success"} 999
solr_core_crossdc_producer_submitted_docs{result="error"} 123
```
##########
solr/modules/cross-dc/src/java/org/apache/solr/crossdc/update/processor/MirroringUpdateProcessor.java:
##########
@@ -271,8 +274,11 @@ public void processDelete(final DeleteUpdateCommand cmd)
throws IOException {
try {
requestMirroringHandler.mirror(mirrorRequest);
+ producerMetrics.getSubmitted().inc();
+ producerMetrics.getSubmittedDeleteById().inc();
} catch (Exception e) {
log.error("mirror submit failed", e);
Review Comment:
Should there be a `producerMetrics.getSubmitError().inc();` here? Looks like
above that is the pattern. Also the best practice of the repo looks to be log
or throw but not both. So I guess remove these log.errrors?
--
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]
