github-actions[bot] commented on code in PR #68506:
URL: https://github.com/apache/doris/pull/68506#discussion_r4110817586
##########
fe/fe-core/src/main/java/org/apache/doris/metric/MetricRepo.java:
##########
@@ -837,7 +837,7 @@ public Long getValue() {
HTTP_COUNTER_COPY_INFO_QUERY_REQUEST = new
LongCounterMetric("http_copy_into_query_request_total",
MetricUnit.REQUESTS, "http copy into total query request");
DORIS_METRIC_REGISTER.addMetrics(HTTP_COUNTER_COPY_INFO_QUERY_REQUEST);
- HTTP_COUNTER_COPY_INFO_QUERY_ERR = new
LongCounterMetric("http_copy_into_upload_err_total",
+ HTTP_COUNTER_COPY_INFO_QUERY_ERR = new
LongCounterMetric("http_copy_into_query_err_total",
Review Comment:
[P1] Count every failed query request in the new series
The new public query-error name still undercounts the outcomes it describes.
`loadQuery()` increments the request total for every call, but its validation
`BAD_REQUEST` returns bypass the error increment; `HttpUtils.getBody()` can
also fail before the try, and `executeQuery()` returns `COMMON_ERROR` for
interrupted/failed futures without updating this counter. Those requests now
appear as successes when consumers compare `query_err_total` with
`query_request_total`. Please update the error counter once at a common
failed-outcome boundary (including these direct returns).
##########
fe/fe-core/src/main/java/org/apache/doris/metric/MetricRepo.java:
##########
@@ -837,7 +837,7 @@ public Long getValue() {
HTTP_COUNTER_COPY_INFO_QUERY_REQUEST = new
LongCounterMetric("http_copy_into_query_request_total",
MetricUnit.REQUESTS, "http copy into total query request");
DORIS_METRIC_REGISTER.addMetrics(HTTP_COUNTER_COPY_INFO_QUERY_REQUEST);
- HTTP_COUNTER_COPY_INFO_QUERY_ERR = new
LongCounterMetric("http_copy_into_upload_err_total",
+ HTTP_COUNTER_COPY_INFO_QUERY_ERR = new
LongCounterMetric("http_copy_into_query_err_total",
Review Comment:
[P2] Add coverage for the registry identity collision
This bug was silent because metrics with the same name and empty label set
replace one another, but the change adds no test that would fail on that
collision. Please add a focused `MetricsTest` assertion that the upload and
query error names resolve to their respective objects and that independent
increments (relative to captured baselines) produce two distinct exported
series/values.
##########
fe/fe-core/src/main/java/org/apache/doris/metric/MetricRepo.java:
##########
@@ -837,7 +837,7 @@ public Long getValue() {
HTTP_COUNTER_COPY_INFO_QUERY_REQUEST = new
LongCounterMetric("http_copy_into_query_request_total",
MetricUnit.REQUESTS, "http copy into total query request");
DORIS_METRIC_REGISTER.addMetrics(HTTP_COUNTER_COPY_INFO_QUERY_REQUEST);
- HTTP_COUNTER_COPY_INFO_QUERY_ERR = new
LongCounterMetric("http_copy_into_upload_err_total",
+ HTTP_COUNTER_COPY_INFO_QUERY_ERR = new
LongCounterMetric("http_copy_into_query_err_total",
Review Comment:
[P1] Count each restored upload failure exactly once
Giving the query counter a distinct key also makes the upload counter
registered above externally visible again, but `CopyIntoAction.copy()` does not
maintain request cardinality: a missing `fileName` returns `BAD_REQUEST` with
no increment, while `DorisHttpException` and generic exceptions increment in
their catch blocks and then fall through to the second increment at line 211.
One failed upload is therefore exported as either 0 or 2 errors. Please
centralize the upload-error update (or make every failed exit update exactly
once) before restoring this series.
--
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]
