mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 18:25:44 +00:00
master
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
0d2637fd63 |
fix: keep proxy metric status label stable, split cause into its own label (#51495)
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 <zhenshan.cao@zilliz.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
|
acfa147d30 |
doc: add error-handling casebook; harden guide/convention docs and agent entry (#50515)
Follow-up to #50221 (issue: #47420) — documentation only, no code change. ## Why The 60+ error misclassifications corrected during the #50221 review all happened **with the rules documents already in place** (`error_handling_guide.md`, `error_sentinel_convention.md`). The rules layer was sufficient; what was missing was (a) a layer of real positive/negative examples showing how the mistakes actually look in code, and (b) a mandatory entry point so coding agents and new authors read the right things before touching error handling. Linters catch *bare* errors, but they cannot catch a *wrong* code or a *wrong* classification — that needs case-level documentation. ## What - **`docs/dev/error_handling_casebook.md` (new).** The seven mistake patterns from the #50221 review, each with the actually-written wrong form (as first drafted, caught in review) and the merged fix, citing file + function: 1. "Looks like validation" is not user input (coordinator task types, plan shapes) 2. Internal component output is not user input (search-reduce, recovered panics) 3. `WrapErrXxxErr` is a relabel, not a context-adder (code masking) 4. The cause goes in the error argument, never the format string (`errors.Is` chain destruction) 5. InputError aborts `retry.Do`: scan before you mark (incl. the dual-identity boundary-stamp pattern) 6. Converting an `errors.Is` control-flow sentinel to merr widens every guard (the `errIgnored*` near-miss) 7. Boundary conversions that look wrong but are contracts (knowhere 2006, segcore wire collapse) Plus a quick-reference table for commonly confused sentinels and a minimum procedure for coding agents. - **`error_handling_guide.md`**: pre-flight scan recipe (`retry.Do` consumers) before InputError marking; `errors.Is`-widening warning before converting a sentinel to merr; casebook cross-link. - **`error_sentinel_convention.md`**: code-range partition table (which 100-range belongs to which family, what 65535 is, why segcore 2000–2099 is table-owned); "`milvusError.Is` matches by code" consequences; casebook cross-link. - **`CLAUDE.md`**: a mandatory error-handling procedure for coding agents (blame test, `merr.Wrap`-only context rule, the two pre-flight greps, code-range rule, guard tests + full test-go on contract changes). All file/function citations were verified against current master (post-#50221). --------- Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
|
e2787d3981 |
enhance: standardize error handling on merr + Sys/Input classification (#50221)
issue: #47420 ## What this PR does Project-wide migration of raw `fmt.Errorf` / `errors.New` in function bodies onto the `merr` framework, plus the Sys-vs-Input error classification and the machinery it drives (retriability, fine-grained metrics, segcore unification), plus the convention docs and a linter that keeps it from regressing. Scope: storage, proxy, coordinators (root/data/query), query node, data node, `pkg/util` & `internal/util`, expression parser, message queue, streaming, and misc packages. Bare raw-error usages went from ~3000 to a ~340 allowlist (package-level sentinels / build-tag / test sites). --- ## How to review this PR It is large but the vast majority is mechanical. Changes fall into three tiers; spend review budget on Part 2 and Part 3. ### Part 1 — Mechanical standardization (low risk, verify by rule) Each converted call follows one of a small fixed set of rules. To review, check that each site obeys the matching rule rather than reading every line: | Pattern | Rule | |---|---| | `fmt.Errorf("...")` originating a new error | → `merr.WrapErrXxxMsg("...")` with a code matching the failure's meaning | | Adding context to an existing typed error | → `merr.Wrap(err, "...")` / `merr.Wrapf(...)` — **preserves** the inner code (never `WrapErr*Err`, which overwrites it) | | Errors inside the streaming subsystem | → `status.New*` factories (StreamingError), **not** merr — this is the component-internal dialect (see `docs/dev/error_handling_guide.md`) | | Low-level / control-flow signal caught by `errors.Is` | → kept as a package-level `errors.New` sentinel (lowercase, same-package) | Conventions are documented in `docs/dev/error_handling_guide.md` (how-to) and `docs/dev/error_sentinel_convention.md` (rules + audit). A `gocritic`/`ruleguard` rule (`rawmerrerror`, in `rules.go`) enforces "no raw `return errors.New/fmt.Errorf`" under `make verifiers`. ### Part 2 — Behavior changes (review these closely) These are the sites where the wire contract or runtime behavior changes, not just the source text. Listed by category; representative locations given, full set in the diff. **A. gRPC wire-code shifts: `UnexpectedError(1)/Code 65535` → typed code.** Where a handler previously returned a raw error (collapsed to `Code=65535` on the wire), it now returns a typed merr, so the client sees a real code. The most common shift is to `IllegalArgument(5)/Code 1100` (ParameterInvalid). Touch points include datanode task handlers (CreateTask/Query/Drop), proxy Upsert, querynode GetMetrics, datacoord CreateIndex, httpserver query-response builder, and typeutil schema validation. One code refinement: an index-param validation moved `1100` → `1101` (ParameterMissing). **Client/SDK assertions and any code that switched on `Code=65535` for these paths must be re-checked** (the go_client e2e assertions were already aligned in this PR). **B. Prometheus `status` label contract change (externally visible).** The proxy metric's coarse `fail` / `rejected` values are split into `fail_input` / `fail_system` and `rejected_user` / `rejected_system` (in `requestutil.ParseMetricLabel`; auth/privilege rejections count as `rejected_user`), so dashboards can attribute a failure to caller vs operator. **Dashboards/alerts querying `status="fail"` must migrate to `status=~"fail_.*"`, and `status="rejected"` to `status=~"rejected_.*"`.** The in-repo Grafana dashboard is already migrated; external dashboards built on the old values silently go empty after upgrade. This is the one change that requires an ops-side migration. **C. Retriability semantics.** - C1: `merr.Status(err)` now forces `Retriable=false` when the error is an `InputError` — a malformed request can never succeed on blind retry, so clients never get the self-contradictory "your input is wrong but you may retry". - C2: `retry.Do` short-circuits an `InputError` (non-retriable) — **but only when the caller did not pass a `RetryErr` predicate**. The check is an `if c.isRetryErr != nil { ... } else if InputError { ... }` *mutually exclusive* branch (`pkg/util/retry/retry.go`): an explicit `RetryErr` takes precedence and bypasses the InputError abort. `retry.Handle` deliberately does **not** apply the InputError abort (its callers signal abort via `shouldRetry=false`). Four flusher startup callsites that must retry through transient "not ready" errors were given explicit `RetryErr` escape hatches. **D. segcore (C++→Go) error classification.** A single shared Go-side table (`pkg/util/merr/segcore.go`) maps each segcore code to a merr sentinel + InputError/signal category, replacing scattered hand-written `if errorCode == ...` switches in the cgo wrappers. **Wire `Code` values change for every segcore pass-through error, not just the remapped ones.** Named sentinels remap (C++ `2003` → merr `2001`, `2033` → `2002`, Folly/Knowhere codes likewise); **all remaining pass-through codes (`2004`–`2043`, previously surfaced to clients as raw C++ enum values) now serialize as `2000`** (`ErrSegcore`), with the original C++ code preserved in the `Reason` text (`segcoreCode=...`); unknown/future codes collapse to `2000` as well (pinned by the `wire_code_projection` test). Transient segcore classes (object storage / file IO / OOM / mmap / FieldNotLoaded — 11 codes) now report `Retriable=true`. **Any client switching on raw segcore codes in the `2004`–`2043` range must be re-checked**; the in-Reason code remains available for diagnostics. Signal codes (PretendFinished / FollyCancel) are recognized centrally. `errors.Is`-based control flow on these (e.g. scheduler skip/retry) is preserved. **E. InputError classification (25 sentinels + dynamic marks).** 25 sentinels in `errors.go` carry `WithErrorType(InputError)` (the Collection / ResourceGroup / Database families, `ErrIndexDuplicate`, `ErrParameterInvalid`, `ErrPrivilegeNotAuthenticated`, `ErrImportFailed`, `ErrQueryPlan`, ...), plus dynamic marks for the 8 segcore input codes (ExprInvalid, DimNotMatch, MetricTypeInvalid, FieldIDInvalid, ...) and `WrapErrAsInputError`. The widest blast radius is `ErrParameterInvalid` (1100): ~2335 `WrapErrParameterInvalid*` callsites now classify as input / non-retriable. Because of C1/C2 this changes retriability for any path that returns these. **The audit to confirm no transient path was mis-marked is the single most important review item** (see Part 3). One reverse correction: storage field-stats parsing moved from `ErrParameterInvalid` (input) to `ErrDataIntegrity` — a corrupted stored stat is data corruption, not user input. ### Part 3 — Known risks & traps (called out proactively) 1. **`merr.Wrap` vs `WrapErr*Err` (code-masking).** `WrapErr*Err` builds a `wrappedMilvusError{sentinel: ErrServiceInternal}` whose `code()` returns the *outer* sentinel — it overwrites the inner typed code and hides the `errors.Is` chain. This is intentional (use it to *deliberately* downgrade), but it was a recurring conversion defect; the rule "add context with `merr.Wrap`, downgrade with `WrapErr*Err`" is enforced by convention and reviewed across the diff. 2. **InputError × `retry.Do` blast radius.** Marking a sentinel `InputError` makes any `retry.Do(...)` without a `RetryErr` predicate stop retrying it. Reviewers should sanity-check that no transient use of the 19 newly-marked sentinels (especially `ErrParameterInvalid`) sits inside a retry loop that needed to keep spinning. The known flusher cases were handled (see C2). 3. **The ~340 raw-error allowlist.** What remains as bare `errors.New` is, by design: package-level sentinels (caught by `errors.Is`), `//go:build test` sites, and out-of-band trees (`cmd/`, `tests/`, codegen, walimpls). The linter only bans the *direct-return* form; assignment-then-return escapes and the full no-exceptions ban are deferred to an AST-based linter (Tier 2, documented). 4. **segcore C++ second step deferred.** This PR unifies classification on the Go side; splitting the dual-semantic C++ codes at the source is a follow-up. --- ## Validation - `make verifiers`: Go side clean (gofmt + static-check across modules, including the new `rawmerrerror` rule with a 0-hit baseline repo-wide). - `make test-go`: passing; the one real regression introduced (a datanode `invalid_task_type` assertion shifting `1` → `5` from a ParameterInvalid conversion) was fixed in-tree. - go_client e2e CreateIndex assertions aligned to the new merr messages. --------- Signed-off-by: zhenshan.cao <zhenshan.cao@zilliz.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |