From 0d2637fd632eeda62cf444b0fd26b0f28643a368 Mon Sep 17 00:00:00 2001 From: "zhenshan.cao" Date: Fri, 17 Jul 2026 02:18:40 -0700 Subject: [PATCH] fix: keep proxy metric status label stable, split cause into its own label (#51495) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issue: #51493 ### What #50221 split the `status` label values of `milvus_proxy_req_count` / `milvus_proxy_grpc_latency` in place. Released in **v2.6.19**, that silently zeroes every existing dashboard panel and alert rule matching `status="fail"` / `status="rejected"` — an alert that stops firing rather than erroring. This keeps the classification but moves it onto its own dimension, restoring the status domain to what <= v2.6.18 emitted: ``` status: success | fail | rejected | retry | total | abandon (as before v2.6.19) cause: user | system | cancel | na (new) ``` | v2.6.19 / v2.6.20 | this PR | |---|---| | `status="fail_input"` | `status="fail", cause="user"` | | `status="fail_system"` | `status="fail", cause="system"` | | `status="rejected_user"` | `status="rejected", cause="user"` | | `status="rejected_system"` | `status="rejected", cause="system"` | | `status="cancel"` | `status="fail"` or `"rejected"`, `cause="cancel"` | | `success` / `retry` / `total` / `abandon` | unchanged, `cause="na"` | ### Why a label instead of new status values - **Old queries work unchanged and count each request once.** `status="fail"` is again every hard failure; Prometheus aggregates over `cause` for free. That rollup is what a label dimension *is* — the split forces every consumer who just wants "how many failures" to hand-write `status=~"fail_.*"`. - **Cardinality is unchanged.** `cause` is functionally dependent on the outcome, so the realized `(status, cause)` pairs are exactly the series the split already produces. Additive, not multiplicative. - **New consumers lose nothing**: alert on `status="fail", cause="system"`; route `cause="user"` to the owning application team. The alternative — emitting old and new `status` values side by side during a transition — was rejected: it defers the break rather than removing it (the old values must still be dropped eventually, forcing the same migration later), and meanwhile double-counts every request in unfiltered aggregations. `cancel` is folded into `cause` for the same reason: before v2.6.19 client cancellations were counted as `fail` (cancel in the response status) or `rejected` (cancel at the interceptor), so promoting it to a `status` value quietly makes `status="fail"` under-count versus older releases. As a cause it stays excludable via `cause!="cancel"` without redefining `status`. ### Also in this PR - The 7 `snapshot_impl` sites that emitted a bare `fail` with no classification now report their real cause via `failMetricLabel(err)` (e.g. `snapshot_metadata_uri is required` is `cause="user"`, not a system fault). Their `status` is unchanged. - `deployments/monitor/grafana/milvus-dashboard.json`: the "Faild Request Rate" panel goes back to `status="fail"` — a verbatim revert of what #50221 changed, which is the compatibility claim demonstrated. - Dev docs and stale comments that named the old label values. ### Verification - `TestParseMetricLabelStatusDomainIsStable` pins the status domain, so re-splitting it fails CI rather than shipping. - Adding a label makes every `WithLabelValues` site arity-sensitive **at runtime, not compile time** — a missed site panics in production. Unit tests do not reach all of them (coverage shows `GetRestoreSnapshotState`, `ListRestoreSnapshotJobs`, `PinSnapshotData`, `UnpinSnapshotData` at 0%), so all **58 emit sites were verified statically via AST**, not only the ones tests happen to hit. - Passing: `pkg/metrics`, `pkg/util/requestutil`, `pkg/util/merr`, `internal/distributed/proxy` (incl. `httpserver`, which covers the REST emit path), and the `internal/proxy` snapshot / metric-label tests. `golangci-lint` clean on every touched package. - Pre-existing and unrelated: `service_test.go` bind failures (`listen tcp :19529: address already in use`) reproduce identically on unmodified master — a local process holds the port. ### Rollout Should be cherry-picked to 2.6 so the window in which the fine-grained `status` values exist stays confined to v2.6.19–v2.6.20. Users who already adopted the new values on those two releases need a one-time query change (`status="fail_system"` -> `status="fail", cause="system"`); that is a known, bounded set, and preferable to leaving every pre-2.6.19 dashboard silently broken. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: zhenshan.cao Co-authored-by: Claude Opus 4.8 (1M context) --- .../monitor/grafana/milvus-dashboard.json | 2 +- docs/dev/error_handling_guide.md | 2 +- docs/dev/error_sentinel_convention.md | 4 +- .../datacoord/ddl_callbacks_import_test.go | 2 +- internal/datacoord/index_meta.go | 2 +- .../proxy/httpserver/handler_v2.go | 16 +-- .../distributed/proxy/request_interceptor.go | 17 +-- .../proxy/request_interceptor_test.go | 18 +-- internal/proxy/hook_interceptor.go | 5 +- internal/proxy/service_provider.go | 9 +- internal/proxy/snapshot_impl.go | 110 ++++++++++------- internal/proxy/util.go | 19 +-- internal/proxy/util_test.go | 23 ++-- internal/storage/insert_data.go | 2 +- internal/util/queryutil/helpers.go | 2 +- pkg/metrics/metrics.go | 29 +++-- pkg/metrics/proxy_metrics.go | 4 +- pkg/util/merr/error_classification_test.go | 2 +- pkg/util/merr/errors.go | 2 +- pkg/util/requestutil/getter.go | 55 ++++----- pkg/util/requestutil/getter_test.go | 116 +++++++++++++----- 21 files changed, 262 insertions(+), 179 deletions(-) diff --git a/deployments/monitor/grafana/milvus-dashboard.json b/deployments/monitor/grafana/milvus-dashboard.json index dbb352ff90..34a33c1899 100644 --- a/deployments/monitor/grafana/milvus-dashboard.json +++ b/deployments/monitor/grafana/milvus-dashboard.json @@ -4794,7 +4794,7 @@ "uid": "${datasource}" }, "exemplar": true, - "expr": "sum(increase(milvus_proxy_req_count{app_kubernetes_io_instance=~\"$instance\", app_kubernetes_io_name=\"$app_name\", namespace=\"$namespace\", status=~\"fail_.*\"}[2m])/120) by(function_name, pod, node_id)", + "expr": "sum(increase(milvus_proxy_req_count{app_kubernetes_io_instance=~\"$instance\", app_kubernetes_io_name=\"$app_name\", namespace=\"$namespace\", status=\"fail\"}[2m])/120) by(function_name, pod, node_id)", "interval": "", "legendFormat": "{{function_name}}-{{pod}}-{{node_id}}", "queryType": "randomWalk", diff --git a/docs/dev/error_handling_guide.md b/docs/dev/error_handling_guide.md index 5db1f2fac8..7cfd1fcd48 100644 --- a/docs/dev/error_handling_guide.md +++ b/docs/dev/error_handling_guide.md @@ -163,7 +163,7 @@ Two mechanisms, used in different situations: | Surface | InputError behavior | |---|---| | `commonpb.Status` | `ExtraInfo["is_input_error"]="true"`, `Retriable` forced `false` | -| Prometheus | request counted as `fail_input` / `rejected_user` (vs `fail_system` / `rejected_system`) | +| Prometheus | request counted with `cause="user"` (vs `cause="system"`) alongside the coarse `status="fail"` / `status="rejected"` | | Access log / failure log | `error_type` field set accordingly | | proxy lb_policy | **no cross-replica failover** — retrying a bad request elsewhere can't help | | `retry.Do` | aborts immediately instead of retrying | diff --git a/docs/dev/error_sentinel_convention.md b/docs/dev/error_sentinel_convention.md index ece73fe40b..a75c25ead3 100644 --- a/docs/dev/error_sentinel_convention.md +++ b/docs/dev/error_sentinel_convention.md @@ -105,8 +105,8 @@ component-to-component) must be `*merr.milvusError` defined in If an error needs to be visible to the wire, it lives here. No exceptions. Every sentinel also carries an Input-vs-System classification (who is to -blame) that drives `Status.Retriable`, the `fail_input`/`fail_system` metric -labels, lb_policy failover and `retry.Do`; see "Input vs System: who is to +blame) that drives `Status.Retriable`, the `cause` metric label, lb_policy +failover and `retry.Do`; see "Input vs System: who is to blame?" in [error_handling_guide.md](error_handling_guide.md). #### Code-range partition diff --git a/internal/datacoord/ddl_callbacks_import_test.go b/internal/datacoord/ddl_callbacks_import_test.go index 3667fce433..0cd4cc925d 100644 --- a/internal/datacoord/ddl_callbacks_import_test.go +++ b/internal/datacoord/ddl_callbacks_import_test.go @@ -108,7 +108,7 @@ func (s *ImportCallbacksSuite) TestValidateImportRequest_MaxJobsExceededReturnsE s.Error(err) // Job-count backpressure is a server-side condition -> ErrImportSysFailed - // (must not be bucketed as fail_input). + // (must not be bucketed as a user-caused failure). s.True(errors.Is(err, merr.ErrImportSysFailed)) s.Contains(err.Error(), "The number of jobs has reached the limit") } diff --git a/internal/datacoord/index_meta.go b/internal/datacoord/index_meta.go index 532cc214ea..edb38de910 100644 --- a/internal/datacoord/index_meta.go +++ b/internal/datacoord/index_meta.go @@ -486,7 +486,7 @@ func (m *indexMeta) canCreateIndex(req *indexpb.CreateIndexRequest, isJSON bool) // Client-caused conflict, same family as the "one distinct index per // field" case above: return an input-class error rather than // ServiceInternal (code 5, "never return out of Milvus") so the - // caller is not blamed with a system error / counted as fail_system. + // caller is not blamed with a system error / counted as a system-caused failure. return 0, merr.WrapErrParameterInvalidMsg("%s", errMsg) } } diff --git a/internal/distributed/proxy/httpserver/handler_v2.go b/internal/distributed/proxy/httpserver/handler_v2.go index cc06327380..4355d5ec94 100644 --- a/internal/distributed/proxy/httpserver/handler_v2.go +++ b/internal/distributed/proxy/httpserver/handler_v2.go @@ -410,24 +410,26 @@ func wrapperPost(newReq newReqFunc, v2 handlerFuncV2) gin.HandlerFunc { strconv.FormatInt(paramtable.GetNodeID(), 10), methodTag, metrics.TotalLabel, + metrics.CauseNA, dbName, collectionName, ).Inc() - label := requestutil.ParseMetricLabel(resp, err) + label, cause := requestutil.ParseMetricLabel(resp, err) // set metrics for state code metrics.ProxyFunctionCall.WithLabelValues( strconv.FormatInt(paramtable.GetNodeID(), 10), methodTag, label, + cause, dbName, collectionName, ).Inc() - // Mirror the fail_input/fail_system metric split into the logs so a - // failed REST request can be filtered by error_type. System failures are - // logged at Warn (actionable); input failures at Info (expected user - // mistakes — keeping them at Warn would spam the logs). - if label == metrics.FailSystemLabel || label == metrics.FailInputLabel { + // Mirror the metric's cause into the logs so a failed REST request can be + // filtered by error_type. System failures are logged at Warn (actionable); + // input failures at Info (expected user mistakes — keeping them at Warn + // would spam the logs). + if label == metrics.FailLabel && (cause == metrics.CauseSystem || cause == metrics.CauseUser) { var status *commonpb.Status switch r := resp.(type) { case interface{ GetStatus() *commonpb.Status }: @@ -436,7 +438,7 @@ func wrapperPost(newReq newReqFunc, v2 handlerFuncV2) gin.HandlerFunc { status = r } errType := merr.SystemError - if label == metrics.FailInputLabel { + if cause == metrics.CauseUser { errType = merr.InputError } logger := mlog.With( diff --git a/internal/distributed/proxy/request_interceptor.go b/internal/distributed/proxy/request_interceptor.go index a7357dd201..0cf314412c 100644 --- a/internal/distributed/proxy/request_interceptor.go +++ b/internal/distributed/proxy/request_interceptor.go @@ -60,28 +60,30 @@ func UnaryRequestStatsInterceptor(ctx context.Context, req any, rpcInfo *grpc.Un strconv.FormatInt(paramtable.GetNodeID(), 10), methodTag, metrics.TotalLabel, + metrics.CauseNA, dbName, collectionName, ).Inc() start := time.Now() resp, err := handler(ctx, req) - label := requestutil.ParseMetricLabel(resp, err) + label, cause := requestutil.ParseMetricLabel(resp, err) // set metrics for state code metrics.ProxyFunctionCall.WithLabelValues( strconv.FormatInt(paramtable.GetNodeID(), 10), methodTag, label, + cause, dbName, collectionName, ).Inc() - // Mirror the fail_input/fail_system metric split into the logs so a failed - // request can be filtered by error_type the same way the metric is. System - // failures are logged at Warn (actionable for SRE); input failures at Info - // (expected user mistakes — keeping them at Warn would spam the logs). - if label == metrics.FailSystemLabel || label == metrics.FailInputLabel { + // Mirror the metric's cause into the logs so a failed request can be + // filtered by error_type the same way the metric is. System failures are + // logged at Warn (actionable for SRE); input failures at Info (expected user + // mistakes — keeping them at Warn would spam the logs). + if label == metrics.FailLabel && (cause == metrics.CauseSystem || cause == metrics.CauseUser) { var status *commonpb.Status switch r := resp.(type) { case interface{ GetStatus() *commonpb.Status }: @@ -90,7 +92,7 @@ func UnaryRequestStatsInterceptor(ctx context.Context, req any, rpcInfo *grpc.Un status = r } errType := merr.SystemError - if label == metrics.FailInputLabel { + if cause == metrics.CauseUser { errType = merr.InputError } logger := mlog.With( @@ -111,6 +113,7 @@ func UnaryRequestStatsInterceptor(ctx context.Context, req any, rpcInfo *grpc.Un strconv.FormatInt(paramtable.GetNodeID(), 10), methodTag, label, + cause, ).Observe(float64(time.Since(start).Milliseconds())) return resp, err diff --git a/internal/distributed/proxy/request_interceptor_test.go b/internal/distributed/proxy/request_interceptor_test.go index e9bea9a5da..274647c8d3 100644 --- a/internal/distributed/proxy/request_interceptor_test.go +++ b/internal/distributed/proxy/request_interceptor_test.go @@ -66,8 +66,8 @@ func (suite *StatsInterceptorSuite) TestUnaryRequestStatsInterceptor() { return merr.Success(), nil }, expectLabels: [][]string{ - {paramtable.GetStringNodeID(), "CreateCollection", metrics.TotalLabel, dbName, collection}, - {paramtable.GetStringNodeID(), "CreateCollection", metrics.SuccessLabel, dbName, collection}, + {paramtable.GetStringNodeID(), "CreateCollection", metrics.TotalLabel, metrics.CauseNA, dbName, collection}, + {paramtable.GetStringNodeID(), "CreateCollection", metrics.SuccessLabel, metrics.CauseNA, dbName, collection}, }, }, { @@ -83,8 +83,8 @@ func (suite *StatsInterceptorSuite) TestUnaryRequestStatsInterceptor() { return merr.Status(merr.WrapErrServiceInternal("unexpcted")), nil }, expectLabels: [][]string{ - {paramtable.GetStringNodeID(), "CreateCollection", metrics.TotalLabel, dbName, collection}, - {paramtable.GetStringNodeID(), "CreateCollection", metrics.FailSystemLabel, dbName, collection}, + {paramtable.GetStringNodeID(), "CreateCollection", metrics.TotalLabel, metrics.CauseNA, dbName, collection}, + {paramtable.GetStringNodeID(), "CreateCollection", metrics.FailLabel, metrics.CauseSystem, dbName, collection}, }, }, { @@ -102,8 +102,8 @@ func (suite *StatsInterceptorSuite) TestUnaryRequestStatsInterceptor() { }, nil }, expectLabels: [][]string{ - {paramtable.GetStringNodeID(), "Insert", metrics.TotalLabel, dbName, collection}, - {paramtable.GetStringNodeID(), "Insert", metrics.RetryLabel, dbName, collection}, + {paramtable.GetStringNodeID(), "Insert", metrics.TotalLabel, metrics.CauseNA, dbName, collection}, + {paramtable.GetStringNodeID(), "Insert", metrics.RetryLabel, metrics.CauseNA, dbName, collection}, }, }, { @@ -119,9 +119,9 @@ func (suite *StatsInterceptorSuite) TestUnaryRequestStatsInterceptor() { return nil, status.Error(codes.Unauthenticated, "auth check failure, please check api key is correct") }, expectLabels: [][]string{ - {paramtable.GetStringNodeID(), "CreateCollection", metrics.TotalLabel, dbName, collection}, - // Unauthenticated is a caller-side rejection -> rejected_user (review §8). - {paramtable.GetStringNodeID(), "CreateCollection", metrics.RejectedUserLabel, dbName, collection}, + {paramtable.GetStringNodeID(), "CreateCollection", metrics.TotalLabel, metrics.CauseNA, dbName, collection}, + // Unauthenticated is the caller's fault -> rejected, cause=user. + {paramtable.GetStringNodeID(), "CreateCollection", metrics.RejectedLabel, metrics.CauseUser, dbName, collection}, }, }, } diff --git a/internal/proxy/hook_interceptor.go b/internal/proxy/hook_interceptor.go index 5f14bbb3cc..b6827a576f 100644 --- a/internal/proxy/hook_interceptor.go +++ b/internal/proxy/hook_interceptor.go @@ -70,6 +70,7 @@ func updateProxyFunctionCallMetric(fullMethod string, err error) { if method == "" { return } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, "", "").Inc() - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), "", "").Inc() + status, cause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, "", "").Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, status, cause, "", "").Inc() } diff --git a/internal/proxy/service_provider.go b/internal/proxy/service_provider.go index 1bc2912a4e..f66da4d599 100644 --- a/internal/proxy/service_provider.go +++ b/internal/proxy/service_provider.go @@ -78,7 +78,7 @@ func (i *InterceptorImpl[Req, Resp]) Call(ctx context.Context, request Req, defer sp.End() tr := timerecord.NewTimeRecorder(i.method) metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), i.method, - metrics.TotalLabel, request.GetDbName(), request.GetCollectionName()).Inc() + metrics.TotalLabel, metrics.CauseNA, request.GetDbName(), request.GetCollectionName()).Inc() resp, err := i.onCall(ctx, request) if err != nil { @@ -86,7 +86,7 @@ func (i *InterceptorImpl[Req, Resp]) Call(ctx context.Context, request Req, } metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), i.method, - metrics.SuccessLabel, request.GetDbName(), request.GetCollectionName()).Inc() + metrics.SuccessLabel, metrics.CauseNA, request.GetDbName(), request.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), i.method).Observe(float64(tr.ElapseSpan().Milliseconds())) return resp, err @@ -281,7 +281,7 @@ func (node *RemoteProxyServiceProvider) DescribeCollection(ctx context.Context, mlog.Err(err)) metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, - metrics.AbandonLabel, request.GetDbName(), request.GetCollectionName()).Inc() + metrics.AbandonLabel, metrics.CauseNA, request.GetDbName(), request.GetCollectionName()).Inc() return nil, err } @@ -295,8 +295,9 @@ func (node *RemoteProxyServiceProvider) DescribeCollection(ctx context.Context, mlog.Uint64("BeginTS", dct.BeginTs()), mlog.Uint64("EndTS", dct.EndTs())) + failStatus, failCause := failMetricLabel(err) metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, - failMetricLabel(err), request.GetDbName(), request.GetCollectionName()).Inc() + failStatus, failCause, request.GetDbName(), request.GetCollectionName()).Inc() return nil, err } diff --git a/internal/proxy/snapshot_impl.go b/internal/proxy/snapshot_impl.go index 142818f4ed..a833200a8d 100644 --- a/internal/proxy/snapshot_impl.go +++ b/internal/proxy/snapshot_impl.go @@ -47,7 +47,7 @@ func (node *Proxy) CreateSnapshot(ctx context.Context, req *milvuspb.CreateSnaps tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() t := &createSnapshotTask{ req: req, ctx: ctx, @@ -57,20 +57,21 @@ func (node *Proxy) CreateSnapshot(ctx context.Context, req *milvuspb.CreateSnaps err := node.sched.ddQueue.Enqueue(t) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "CreateSnapshot failed to Enqueue", mlog.Err(err)) return merr.Status(err), nil } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "CreateSnapshot failed to WaitToFinish", mlog.Err(err)) return merr.Status(err), nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -87,7 +88,7 @@ func (node *Proxy) DropSnapshot(ctx context.Context, req *milvuspb.DropSnapshotR tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() t := &dropSnapshotTask{ req: req, ctx: ctx, @@ -97,20 +98,21 @@ func (node *Proxy) DropSnapshot(ctx context.Context, req *milvuspb.DropSnapshotR err := node.sched.ddQueue.Enqueue(t) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "DropSnapshot failed to Enqueue", mlog.Err(err)) return merr.Status(err), nil } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "DropSnapshot failed to WaitToFinish", mlog.Err(err)) return merr.Status(err), nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -127,7 +129,7 @@ func (node *Proxy) DescribeSnapshot(ctx context.Context, req *milvuspb.DescribeS tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() t := &describeSnapshotTask{ req: req, ctx: ctx, @@ -137,7 +139,7 @@ func (node *Proxy) DescribeSnapshot(ctx context.Context, req *milvuspb.DescribeS err := node.sched.ddQueue.Enqueue(t) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "DescribeSnapshot failed to Enqueue", mlog.Err(err)) return &milvuspb.DescribeSnapshotResponse{ @@ -146,7 +148,8 @@ func (node *Proxy) DescribeSnapshot(ctx context.Context, req *milvuspb.DescribeS } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "DescribeSnapshot failed to WaitToFinish", mlog.Err(err)) return &milvuspb.DescribeSnapshotResponse{ @@ -154,7 +157,7 @@ func (node *Proxy) DescribeSnapshot(ctx context.Context, req *milvuspb.DescribeS }, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -170,7 +173,7 @@ func (node *Proxy) ListSnapshots(ctx context.Context, req *milvuspb.ListSnapshot tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() t := &listSnapshotsTask{ req: req, ctx: ctx, @@ -180,7 +183,7 @@ func (node *Proxy) ListSnapshots(ctx context.Context, req *milvuspb.ListSnapshot err := node.sched.ddQueue.Enqueue(t) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "ListSnapshots failed to Enqueue", mlog.Err(err)) return &milvuspb.ListSnapshotsResponse{ @@ -189,7 +192,8 @@ func (node *Proxy) ListSnapshots(ctx context.Context, req *milvuspb.ListSnapshot } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "ListSnapshots failed to WaitToFinish", mlog.Err(err)) return &milvuspb.ListSnapshotsResponse{ @@ -197,7 +201,7 @@ func (node *Proxy) ListSnapshots(ctx context.Context, req *milvuspb.ListSnapshot }, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -221,10 +225,11 @@ func (node *Proxy) RestoreExternalSnapshot(ctx context.Context, req *milvuspb.Re ) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetTargetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetTargetCollectionName()).Inc() if req.GetSnapshotMetadataUri() == "" { err := merr.WrapErrParameterInvalidMsg("snapshot_metadata_uri is required for restore external snapshot") - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.FailLabel, req.GetDbName(), req.GetTargetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetTargetCollectionName()).Inc() return &milvuspb.RestoreExternalSnapshotResponse{Status: merr.Status(err)}, nil } resp, err := node.mixCoord.RestoreSnapshot(ctx, &datapb.RestoreSnapshotRequest{ @@ -238,12 +243,13 @@ func (node *Proxy) RestoreExternalSnapshot(ctx context.Context, req *milvuspb.Re ExternalSpec: req.GetExternalSpec(), }) if err = merr.CheckRPCCall(resp, err); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.FailLabel, req.GetDbName(), req.GetTargetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetTargetCollectionName()).Inc() log.Warn(ctx, "RestoreExternalSnapshot failed", mlog.Err(err)) return &milvuspb.RestoreExternalSnapshotResponse{Status: merr.Status(err)}, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetTargetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetTargetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return &milvuspb.RestoreExternalSnapshotResponse{ Status: resp.GetStatus(), @@ -271,25 +277,29 @@ func (node *Proxy) ExportSnapshot(ctx context.Context, req *milvuspb.ExportSnaps ) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() if err := ValidateSnapshotName(req.GetName()); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.FailLabel, req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() return &milvuspb.ExportSnapshotResponse{Status: merr.Status(err)}, nil } if req.GetCollectionName() == "" { err := merr.WrapErrParameterInvalidMsg("collection_name is required for export snapshot") - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.FailLabel, req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() return &milvuspb.ExportSnapshotResponse{Status: merr.Status(err)}, nil } if req.GetTargetS3Path() == "" { err := merr.WrapErrParameterInvalidMsg("target_s3_path is required for export snapshot") - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.FailLabel, req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() return &milvuspb.ExportSnapshotResponse{Status: merr.Status(err)}, nil } collectionID, err := globalMetaCache.GetCollectionID(ctx, req.GetDbName(), req.GetCollectionName()) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.FailLabel, req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "ExportSnapshot failed to resolve collection", mlog.Err(err)) return &milvuspb.ExportSnapshotResponse{Status: merr.Status(err)}, nil } @@ -303,12 +313,13 @@ func (node *Proxy) ExportSnapshot(ctx context.Context, req *milvuspb.ExportSnaps ExternalSpec: req.GetExternalSpec(), }) if err = merr.CheckRPCCall(resp, err); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.FailLabel, req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "ExportSnapshot failed", mlog.Err(err)) return &milvuspb.ExportSnapshotResponse{Status: merr.Status(err)}, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return &milvuspb.ExportSnapshotResponse{ Status: resp.GetStatus(), @@ -328,7 +339,7 @@ func (node *Proxy) RestoreSnapshot(ctx context.Context, req *milvuspb.RestoreSna tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() t := &restoreSnapshotTask{ req: req, ctx: ctx, @@ -338,20 +349,21 @@ func (node *Proxy) RestoreSnapshot(ctx context.Context, req *milvuspb.RestoreSna err := node.sched.ddQueue.Enqueue(t) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "RestoreSnapshot failed to Enqueue", mlog.Err(err)) return &milvuspb.RestoreSnapshotResponse{Status: merr.Status(err)}, nil } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "RestoreSnapshot failed to WaitToFinish", mlog.Err(err)) return &milvuspb.RestoreSnapshotResponse{Status: merr.Status(err)}, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -368,7 +380,7 @@ func (node *Proxy) GetRestoreSnapshotState(ctx context.Context, req *milvuspb.Ge tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, "", "").Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, "", "").Inc() t := &getRestoreSnapshotStateTask{ req: req, ctx: ctx, @@ -378,7 +390,7 @@ func (node *Proxy) GetRestoreSnapshotState(ctx context.Context, req *milvuspb.Ge err := node.sched.ddQueue.Enqueue(t) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, "", "").Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, "", "").Inc() log.Warn(ctx, "GetRestoreSnapshotState failed to Enqueue", mlog.Err(err)) return &milvuspb.GetRestoreSnapshotStateResponse{ @@ -387,7 +399,8 @@ func (node *Proxy) GetRestoreSnapshotState(ctx context.Context, req *milvuspb.Ge } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), "", "").Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, "", "").Inc() log.Warn(ctx, "GetRestoreSnapshotState failed to WaitToFinish", mlog.Err(err)) return &milvuspb.GetRestoreSnapshotStateResponse{ @@ -395,7 +408,7 @@ func (node *Proxy) GetRestoreSnapshotState(ctx context.Context, req *milvuspb.Ge }, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, "", "").Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, "", "").Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -412,7 +425,7 @@ func (node *Proxy) ListRestoreSnapshotJobs(ctx context.Context, req *milvuspb.Li tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() t := &listRestoreSnapshotJobsTask{ req: req, ctx: ctx, @@ -422,7 +435,7 @@ func (node *Proxy) ListRestoreSnapshotJobs(ctx context.Context, req *milvuspb.Li err := node.sched.ddQueue.Enqueue(t) if err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "ListRestoreSnapshotJobs failed to Enqueue", mlog.Err(err)) return &milvuspb.ListRestoreSnapshotJobsResponse{ @@ -431,7 +444,8 @@ func (node *Proxy) ListRestoreSnapshotJobs(ctx context.Context, req *milvuspb.Li } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "ListRestoreSnapshotJobs failed to WaitToFinish", mlog.Err(err)) return &milvuspb.ListRestoreSnapshotJobsResponse{ @@ -439,7 +453,7 @@ func (node *Proxy) ListRestoreSnapshotJobs(ctx context.Context, req *milvuspb.Li }, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -458,7 +472,7 @@ func (node *Proxy) PinSnapshotData(ctx context.Context, req *milvuspb.PinSnapsho tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() t := &pinSnapshotDataTask{ req: req, ctx: ctx, @@ -467,7 +481,7 @@ func (node *Proxy) PinSnapshotData(ctx context.Context, req *milvuspb.PinSnapsho } if err := node.sched.ddQueue.Enqueue(t); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "PinSnapshotData failed to Enqueue", mlog.Err(err)) return &milvuspb.PinSnapshotDataResponse{ @@ -476,7 +490,8 @@ func (node *Proxy) PinSnapshotData(ctx context.Context, req *milvuspb.PinSnapsho } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), req.GetDbName(), req.GetCollectionName()).Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, req.GetDbName(), req.GetCollectionName()).Inc() log.Warn(ctx, "PinSnapshotData failed to WaitToFinish", mlog.Err(err)) return &milvuspb.PinSnapshotDataResponse{ @@ -484,7 +499,7 @@ func (node *Proxy) PinSnapshotData(ctx context.Context, req *milvuspb.PinSnapsho }, nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, req.GetDbName(), req.GetCollectionName()).Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, req.GetDbName(), req.GetCollectionName()).Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } @@ -501,7 +516,7 @@ func (node *Proxy) UnpinSnapshotData(ctx context.Context, req *milvuspb.UnpinSna tr := timerecord.NewTimeRecorder(method) log.Info(ctx, rpcReceived(method)) - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, "", "").Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.TotalLabel, metrics.CauseNA, "", "").Inc() t := &unpinSnapshotDataTask{ req: req, ctx: ctx, @@ -510,20 +525,21 @@ func (node *Proxy) UnpinSnapshotData(ctx context.Context, req *milvuspb.UnpinSna } if err := node.sched.ddQueue.Enqueue(t); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, "", "").Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.AbandonLabel, metrics.CauseNA, "", "").Inc() log.Warn(ctx, "UnpinSnapshotData failed to Enqueue", mlog.Err(err)) return merr.Status(err), nil } if err := t.WaitToFinish(); err != nil { - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failMetricLabel(err), "", "").Inc() + failStatus, failCause := failMetricLabel(err) + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, failStatus, failCause, "", "").Inc() log.Warn(ctx, "UnpinSnapshotData failed to WaitToFinish", mlog.Err(err)) return merr.Status(err), nil } - metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, "", "").Inc() + metrics.ProxyFunctionCall.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method, metrics.SuccessLabel, metrics.CauseNA, "", "").Inc() metrics.ProxyReqLatency.WithLabelValues(strconv.FormatInt(paramtable.GetNodeID(), 10), method).Observe(float64(tr.ElapseSpan().Milliseconds())) return t.result, nil } diff --git a/internal/proxy/util.go b/internal/proxy/util.go index 6f1e213ecb..8a928b720b 100644 --- a/internal/proxy/util.go +++ b/internal/proxy/util.go @@ -3509,17 +3509,18 @@ func getBM25FunctionOfAnnsField(fieldID int64, functions []*schemapb.FunctionSch } // failMetricLabel classifies a request failure for the ProxyFunctionCall -// metric, matching the fail_input/fail_system split that -// requestutil.ParseMetricLabel emits at the gRPC interceptor; the legacy -// bare "fail" value must not reappear in this metric's value domain. -func failMetricLabel(err error) string { - // Client cancellation is neither party's failure; keep it out of the - // fail_system bucket (parity with ParseMetricLabel). +// metric, returning the same (status, cause) pair that +// requestutil.ParseMetricLabel emits at the gRPC interceptor. The status stays +// the coarse "fail" so that a query written against it counts every hard +// failure regardless of who caused it. +func failMetricLabel(err error) (status string, cause string) { + // Client cancellation is neither party's failure; cause is what lets a + // consumer exclude it (parity with ParseMetricLabel). if errors.Is(err, context.Canceled) { - return metrics.CancelLabel + return metrics.FailLabel, metrics.CauseCancel } if merr.GetErrorType(err) == merr.InputError { - return metrics.FailInputLabel + return metrics.FailLabel, metrics.CauseUser } - return metrics.FailSystemLabel + return metrics.FailLabel, metrics.CauseSystem } diff --git a/internal/proxy/util_test.go b/internal/proxy/util_test.go index 9cbf412c2d..8e35705789 100644 --- a/internal/proxy/util_test.go +++ b/internal/proxy/util_test.go @@ -6869,14 +6869,23 @@ func TestInjectVirtualPKForExternalCollection(t *testing.T) { } func TestFailMetricLabel(t *testing.T) { + assertLabels := func(wantCause string, err error) { + t.Helper() + status, cause := failMetricLabel(err) + // Every failure keeps the coarse status a pre-2.6.19 dashboard queries; + // only the cause tells the parties apart. + assert.Equal(t, metrics.FailLabel, status) + assert.Equal(t, wantCause, cause) + } + // untyped / nil errors must take the conservative system bucket - assert.Equal(t, metrics.FailSystemLabel, failMetricLabel(nil)) - assert.Equal(t, metrics.FailSystemLabel, failMetricLabel(errors.New("plain error"))) - assert.Equal(t, metrics.FailSystemLabel, failMetricLabel(merr.WrapErrServiceInternalMsg("internal failure"))) + assertLabels(metrics.CauseSystem, nil) + assertLabels(metrics.CauseSystem, errors.New("plain error")) + assertLabels(metrics.CauseSystem, merr.WrapErrServiceInternalMsg("internal failure")) // input-classified merr errors go to the user bucket, also through wrapping - assert.Equal(t, metrics.FailInputLabel, failMetricLabel(merr.WrapErrParameterInvalidMsg("bad parameter"))) - assert.Equal(t, metrics.FailInputLabel, failMetricLabel(merr.Wrap(merr.WrapErrParameterInvalidMsg("bad parameter"), "context"))) + assertLabels(metrics.CauseUser, merr.WrapErrParameterInvalidMsg("bad parameter")) + assertLabels(metrics.CauseUser, merr.Wrap(merr.WrapErrParameterInvalidMsg("bad parameter"), "context")) // client cancellation is neither party's failure - assert.Equal(t, metrics.CancelLabel, failMetricLabel(context.Canceled)) - assert.Equal(t, metrics.CancelLabel, failMetricLabel(errors.Wrap(context.Canceled, "rpc aborted"))) + assertLabels(metrics.CauseCancel, context.Canceled) + assertLabels(metrics.CauseCancel, errors.Wrap(context.Canceled, "rpc aborted")) } diff --git a/internal/storage/insert_data.go b/internal/storage/insert_data.go index 39eab2486a..c5806d4e7a 100644 --- a/internal/storage/insert_data.go +++ b/internal/storage/insert_data.go @@ -62,7 +62,7 @@ func NewInsertDataWithCap(schema *schemapb.CollectionSchema, cap int, withFuncti // A nil schema here is an internal invariant violation (callers within // Milvus always pass a schema), not a client-supplied missing parameter. // ErrParameterMissing defaults to InputError, so mark this one site as a - // system error to keep it out of the client-fault (fail_input) bucket. + // system error to keep it out of the client-fault (cause="user") bucket. return nil, merr.WrapErrAsSysError(merr.WrapErrParameterMissing("collection schema")) } diff --git a/internal/util/queryutil/helpers.go b/internal/util/queryutil/helpers.go index f227a1aa4e..e52e84cbf5 100644 --- a/internal/util/queryutil/helpers.go +++ b/internal/util/queryutil/helpers.go @@ -17,7 +17,7 @@ // The merge/slice helpers in this file consume RetrieveResults produced by // segcore (and merged across query nodes), never raw user input, so every // data-shape assertion below classifies as ServiceInternal: a violation means -// a segcore/Milvus bug, and must not be attributed to the user (fail_input) +// a segcore/Milvus bug, and must not be attributed to the user (cause="user") // or suppress cross-replica failover the way an InputError would. package queryutil diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index e5ee3aa165..2008da6805 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -34,21 +34,19 @@ const ( RetryLabel = "retry" RejectedLabel = "rejected" - // Fine-grained hard-failure labels (composite status values, not a new - // dimension label, to keep Prometheus cardinality additive rather than - // multiplicative). They split the coarse "fail"/"rejected" into the - // responsible party so monitoring/alerting can tell a user-input error - // (the caller must fix the request) apart from an internal system error - // (operators must intervene). MIGRATION: old queries on status="fail" must - // move to status=~"fail_.*" (fail_input/fail_system), and old queries on - // status="rejected" must move to status=~"rejected_.*" (rejected_user for - // caller-side auth/privilege/bad-arg rejections, rejected_system otherwise). - // Dashboards still matching the bare "fail"/"rejected" values return nothing - // after this split. - FailInputLabel = "fail_input" - FailSystemLabel = "fail_system" - RejectedSystemLabel = "rejected_system" - RejectedUserLabel = "rejected_user" + // Values of the "cause" label, a dimension orthogonal to "status" that names + // the responsible party for a failed request, so monitoring can tell a + // user-input error (the caller must fix the request) apart from an internal + // system error (operators must intervene). It is a separate label rather + // than extra "status" values on purpose: the coarse status stays a valid + // query on its own ("fail" is still every hard failure, aggregated over + // cause by Prometheus), so dashboards written against it keep working. + // Cause is functionally dependent on the outcome, so the realized + // (status, cause) pairs are additive, not the product of both domains. + CauseUser = "user" // the request itself is at fault: bad arguments, missing auth, no such collection + CauseSystem = "system" // Milvus is at fault: component failure, IO error, internal bug + CauseCancel = "cancel" // neither party: the client gave up before the request completed + CauseNA = "na" // no cause applies: the request did not hard-fail HybridSearchLabel = "hybrid_search" @@ -123,6 +121,7 @@ const ( nodeIDLabelName = "node_id" nodeHostLabelName = "node_host" statusLabelName = "status" + causeLabelName = "cause" indexTaskStatusLabelName = "index_task_status" msgTypeLabelName = "msg_type" collectionIDLabelName = "collection_id" diff --git a/pkg/metrics/proxy_metrics.go b/pkg/metrics/proxy_metrics.go index f98c24cbf0..c76003f51c 100644 --- a/pkg/metrics/proxy_metrics.go +++ b/pkg/metrics/proxy_metrics.go @@ -233,7 +233,7 @@ var ( Subsystem: typeutil.ProxyRole, Name: "req_count", Help: "count of operation executed", - }, []string{nodeIDLabelName, functionLabelName, statusLabelName, databaseLabelName, collectionName}) + }, []string{nodeIDLabelName, functionLabelName, statusLabelName, causeLabelName, databaseLabelName, collectionName}) // ProxyReqLatency records the latency for each grpc request. ProxyGRPCLatency = prometheus.NewHistogramVec( @@ -243,7 +243,7 @@ var ( Name: "grpc_latency", Help: "latency of each grpc request", Buckets: buckets, // unit: ms - }, []string{nodeIDLabelName, functionLabelName, statusLabelName}) + }, []string{nodeIDLabelName, functionLabelName, statusLabelName, causeLabelName}) // ProxyReqLatency records the latency that for all requests, like "CreateCollection". ProxyReqLatency = prometheus.NewHistogramVec( diff --git a/pkg/util/merr/error_classification_test.go b/pkg/util/merr/error_classification_test.go index 4bd155f9c3..e18da41a88 100644 --- a/pkg/util/merr/error_classification_test.go +++ b/pkg/util/merr/error_classification_test.go @@ -100,7 +100,7 @@ func TestErrorTypeMarker(t *testing.T) { } // TestSentinelErrorTypeClassification guards the input/system split that the -// proxy fail_input/fail_system alerting relies on. A sentinel silently losing +// proxy cause="user"/cause="system" alerting relies on. A sentinel silently losing // (or gaining) WithErrorType(InputError) flips which party an alert blames. func TestSentinelErrorTypeClassification(t *testing.T) { // The request's own fault -> InputError (and therefore non-retriable). diff --git a/pkg/util/merr/errors.go b/pkg/util/merr/errors.go index 8857d26de0..f308467939 100644 --- a/pkg/util/merr/errors.go +++ b/pkg/util/merr/errors.go @@ -262,7 +262,7 @@ var ( // import job orchestration (job not found / no vchannels / restore / // job-count backpressure) and object-storage IO failures in the readers. // These are the operator's concern, not the caller's, so they stay - // SystemError and must not be bucketed as fail_input. + // SystemError and must not be bucketed as a user-caused failure. ErrImportSysFailed = newMilvusError("importing data failed on server side", 2101, false) // Search/Query related diff --git a/pkg/util/requestutil/getter.go b/pkg/util/requestutil/getter.go index 43feadb974..943ce96b8b 100644 --- a/pkg/util/requestutil/getter.go +++ b/pkg/util/requestutil/getter.go @@ -23,7 +23,7 @@ import ( "github.com/cockroachdb/errors" "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" + grpcstatus "google.golang.org/grpc/status" "github.com/milvus-io/milvus-proto/go-api/v3/commonpb" "github.com/milvus-io/milvus-proto/go-api/v3/schemapb" @@ -230,19 +230,18 @@ var retryableCode typeutil.Set[int32] = typeutil.NewSet( // ) // } -// ParseMetricLabel determines the final Prometheus status label based on the -// response and error. It implements the composite-label scheme: the coarse -// "fail"/"rejected" values are split into fine-grained labels (fail_input / -// fail_system / rejected_system) so monitoring can tell the responsible party -// apart. The split is encoded into the existing status label's value domain -// (additive cardinality) rather than a new dimension label (which would be -// multiplicative). Retryability takes priority over classification. -func ParseMetricLabel(resp any, err error) string { +// ParseMetricLabel determines the Prometheus status and cause labels of a +// finished request. The status domain is the coarse outcome and is deliberately +// the same one pre-2.6.19 emitted, so a query written against it keeps its +// meaning; cause is an orthogonal dimension naming the responsible party, which +// Prometheus aggregates away for consumers that only ask for the status. +// Retryability takes priority over classification. +func ParseMetricLabel(resp any, err error) (status string, cause string) { // A response carrying a non-OK status means the request was PROCESSED and - // failed (fail_*), and takes priority over a non-nil err: the REST v2 - // wrappers reconstruct err = merr.Error(status) from that same response, - // which previously routed every processed REST failure into the rejected_* - // buckets and left the fail_* series blind to the entire REST surface. + // failed, and takes priority over a non-nil err: the REST v2 wrappers + // reconstruct err = merr.Error(status) from that same response, which + // otherwise routes every processed REST failure into the rejected buckets + // and leaves the fail series blind to the entire REST surface. var st *commonpb.Status switch resp := resp.(type) { case interface{ GetStatus() *commonpb.Status }: @@ -251,48 +250,48 @@ func ParseMetricLabel(resp any, err error) string { st = resp } if st != nil && !merr.Ok(st) { - // Client cancellation is neither party's failure. + // Client cancellation is neither party's failure, but it stays a "fail" + // like it was before the cause dimension existed; cause is what lets a + // consumer exclude it. if st.GetCode() == merr.CanceledCode { - return metrics.CancelLabel + return metrics.FailLabel, metrics.CauseCancel } - // Retryability takes priority over input/system classification. + // Retryability takes priority over classification. if retryableCode.Contain(st.GetCode()) { - return metrics.RetryLabel + return metrics.RetryLabel, metrics.CauseNA } // Hard failure: classify by responsible party. merr.Status already // stamps the InputError flag into ExtraInfo, so read it directly instead // of reconstructing the whole milvusError (this is the proxy hot path). if st.GetExtraInfo()[merr.InputErrorFlagKey] == "true" { - return metrics.FailInputLabel + return metrics.FailLabel, metrics.CauseUser } - return metrics.FailSystemLabel + return metrics.FailLabel, metrics.CauseSystem } // No usable response status: err is the interceptor-level outcome (context // cancellation, flow control, transport issues, auth/privilege rejection) // — the request was rejected around processing. Classify merr first: a - // merr error has no GRPCStatus(), so status.Code(err) degrades to + // merr error has no GRPCStatus(), so grpcstatus.Code(err) degrades to // codes.Unknown and would misbucket user input errors as system // rejections. The auth/privilege interceptors deliberately return raw gRPC // codes (not merr, to keep SDK retry behavior correct); those are the // caller's fault, so bucket them as a user-side rejection. Everything else // is a system-side rejection. if err != nil { - // Client cancellation is neither party's failure; don't count it as a - // system rejection. if errors.Is(err, context.Canceled) { - return metrics.CancelLabel + return metrics.RejectedLabel, metrics.CauseCancel } if merr.GetErrorType(err) == merr.InputError { - return metrics.RejectedUserLabel + return metrics.RejectedLabel, metrics.CauseUser } - switch status.Code(err) { + switch grpcstatus.Code(err) { case codes.Unauthenticated, codes.PermissionDenied, codes.InvalidArgument: - return metrics.RejectedUserLabel + return metrics.RejectedLabel, metrics.CauseUser default: - return metrics.RejectedSystemLabel + return metrics.RejectedLabel, metrics.CauseSystem } } - return metrics.SuccessLabel + return metrics.SuccessLabel, metrics.CauseNA } diff --git a/pkg/util/requestutil/getter_test.go b/pkg/util/requestutil/getter_test.go index fc07c65075..76f499d94e 100644 --- a/pkg/util/requestutil/getter_test.go +++ b/pkg/util/requestutil/getter_test.go @@ -30,6 +30,7 @@ import ( "github.com/milvus-io/milvus-proto/go-api/v3/milvuspb" "github.com/milvus-io/milvus/pkg/v3/metrics" "github.com/milvus-io/milvus/pkg/v3/util/merr" + "github.com/milvus-io/milvus/pkg/v3/util/typeutil" ) func TestGetCollectionNameFromRequest(t *testing.T) { @@ -516,55 +517,106 @@ func TestGetStatusFromResponse(t *testing.T) { } func TestParseMetricLabel(t *testing.T) { - // transport / interceptor error -> rejected_system (highest priority) - assert.Equal(t, metrics.RejectedSystemLabel, - ParseMetricLabel(&commonpb.Status{}, errors.New("transport failed"))) + assertLabels := func(wantStatus, wantCause string, resp any, err error) { + t.Helper() + status, cause := ParseMetricLabel(resp, err) + assert.Equal(t, wantStatus, status) + assert.Equal(t, wantCause, cause) + } + + // transport / interceptor error -> rejected by the system + assertLabels(metrics.RejectedLabel, metrics.CauseSystem, + &commonpb.Status{}, errors.New("transport failed")) // success - assert.Equal(t, metrics.SuccessLabel, - ParseMetricLabel(&commonpb.Status{}, nil)) + assertLabels(metrics.SuccessLabel, metrics.CauseNA, &commonpb.Status{}, nil) // retryable hard failure -> retry (retryability beats classification) - assert.Equal(t, metrics.RetryLabel, - ParseMetricLabel(merr.Status(merr.ErrServiceRateLimit), nil)) + assertLabels(metrics.RetryLabel, metrics.CauseNA, + merr.Status(merr.ErrServiceRateLimit), nil) - // hard failure, system error -> fail_system - assert.Equal(t, metrics.FailSystemLabel, - ParseMetricLabel(merr.Status(merr.ErrSegcore), nil)) + // hard failure caused by Milvus itself + assertLabels(metrics.FailLabel, metrics.CauseSystem, + merr.Status(merr.ErrSegcore), nil) - // hard failure, input error -> fail_input + // hard failure caused by the request inputErr := merr.WrapErrAsInputError(merr.WrapErrParameterInvalidMsg("bad param")) - assert.Equal(t, metrics.FailInputLabel, - ParseMetricLabel(merr.Status(inputErr), nil)) + assertLabels(metrics.FailLabel, metrics.CauseUser, merr.Status(inputErr), nil) // response that itself implements GetStatus - assert.Equal(t, metrics.FailInputLabel, - ParseMetricLabel(&milvuspb.BoolResponse{Status: merr.Status(inputErr)}, nil)) + assertLabels(metrics.FailLabel, metrics.CauseUser, + &milvuspb.BoolResponse{Status: merr.Status(inputErr)}, nil) // merr input error through the err path (REST v2 handlers abort with merr - // directly; no GRPCStatus, so the gRPC switch alone would misbucket it as - // rejected_system) -> rejected_user - assert.Equal(t, metrics.RejectedUserLabel, - ParseMetricLabel(&commonpb.Status{}, merr.WrapErrAsInputError(merr.WrapErrParameterInvalidMsg("bad param")))) + // directly; no GRPCStatus, so the gRPC switch alone would misbucket it as a + // system rejection) + assertLabels(metrics.RejectedLabel, metrics.CauseUser, &commonpb.Status{}, + merr.WrapErrAsInputError(merr.WrapErrParameterInvalidMsg("bad param"))) - // merr system error through the err path -> rejected_system - assert.Equal(t, metrics.RejectedSystemLabel, - ParseMetricLabel(&commonpb.Status{}, merr.WrapErrServiceInternalMsg("boom"))) + // merr system error through the err path + assertLabels(metrics.RejectedLabel, metrics.CauseSystem, + &commonpb.Status{}, merr.WrapErrServiceInternalMsg("boom")) - // client cancellation through the err path -> cancel, not a system rejection - assert.Equal(t, metrics.CancelLabel, - ParseMetricLabel(&commonpb.Status{}, context.Canceled)) - assert.Equal(t, metrics.CancelLabel, - ParseMetricLabel(&commonpb.Status{}, errors.Wrap(context.Canceled, "rpc aborted"))) + // client cancellation is neither party's fault, but it stays a rejection so + // that a pre-existing status="rejected" query keeps counting it + assertLabels(metrics.RejectedLabel, metrics.CauseCancel, &commonpb.Status{}, context.Canceled) + assertLabels(metrics.RejectedLabel, metrics.CauseCancel, + &commonpb.Status{}, errors.Wrap(context.Canceled, "rpc aborted")) // REST v2 wrapper shape: the response carries the failed status AND the // wrapper reconstructs err from it. Processed failures must classify by - // the status (fail_*), not be misrouted into the rejected_* buckets. + // the status ("fail"), not be misrouted into the "rejected" bucket. restSys := merr.Status(merr.ErrSegcore) - assert.Equal(t, metrics.FailSystemLabel, ParseMetricLabel(restSys, merr.Error(restSys))) + assertLabels(metrics.FailLabel, metrics.CauseSystem, restSys, merr.Error(restSys)) restInput := merr.Status(merr.WrapErrAsInputError(merr.WrapErrParameterInvalidMsg("bad param"))) - assert.Equal(t, metrics.FailInputLabel, ParseMetricLabel(restInput, merr.Error(restInput))) - // Cancellation surfaced through the response status stays out of fail_system. + assertLabels(metrics.FailLabel, metrics.CauseUser, restInput, merr.Error(restInput)) + // Cancellation surfaced through the response status is still a "fail", as it + // was before the cause dimension existed; cause is what excludes it. restCancel := merr.Status(context.Canceled) - assert.Equal(t, metrics.CancelLabel, ParseMetricLabel(restCancel, merr.Error(restCancel))) + assertLabels(metrics.FailLabel, metrics.CauseCancel, restCancel, merr.Error(restCancel)) +} + +// The status label is a public monitoring contract: dashboards and alert rules +// written against status="fail" / status="rejected" predate the cause dimension +// and must keep working. Splitting the responsible party into new status values +// (fail_input, rejected_system, ...) silently zeroes those queries, so pin the +// domain here: anything finer belongs on the orthogonal cause label. +func TestParseMetricLabelStatusDomainIsStable(t *testing.T) { + legacyStatus := typeutil.NewSet( + metrics.SuccessLabel, + metrics.FailLabel, + metrics.RejectedLabel, + metrics.RetryLabel, + ) + knownCause := typeutil.NewSet( + metrics.CauseUser, + metrics.CauseSystem, + metrics.CauseCancel, + metrics.CauseNA, + ) + + inputErr := merr.WrapErrAsInputError(merr.WrapErrParameterInvalidMsg("bad param")) + cases := []struct { + name string + resp any + err error + }{ + {"success", &commonpb.Status{}, nil}, + {"transport error", &commonpb.Status{}, errors.New("transport failed")}, + {"rate limited", merr.Status(merr.ErrServiceRateLimit), nil}, + {"system failure", merr.Status(merr.ErrSegcore), nil}, + {"input failure", merr.Status(inputErr), nil}, + {"input rejection", &commonpb.Status{}, inputErr}, + {"canceled at interceptor", &commonpb.Status{}, context.Canceled}, + {"canceled in response", merr.Status(context.Canceled), nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + status, cause := ParseMetricLabel(tc.resp, tc.err) + assert.Truef(t, legacyStatus.Contain(status), + "status %q is outside the pre-2.6.19 domain %v; put the distinction on the cause label instead", + status, legacyStatus.Collect()) + assert.Truef(t, knownCause.Contain(cause), "unknown cause %q", cause) + }) + } }