diff --git a/.agents/skills/pr-checklist/SKILL.md b/.agents/skills/pr-checklist/SKILL.md index 4846e8c226a8..f0ea6337b553 100644 --- a/.agents/skills/pr-checklist/SKILL.md +++ b/.agents/skills/pr-checklist/SKILL.md @@ -57,6 +57,7 @@ Exclude tests, documentation, comments, and whitespace from this count, as they - Rust: the `graham-code-review` skill - Python: [Python guidelines](../../../.ai/python-guidelines.md) - Tests: [pytest guidelines](../../../.ai/pytest-guidelines.md) and [test model size guardrails](../../../.ai/test-model-size-guardrails.md) +- Metrics, logs, and traces: [metrics guidelines](../../../.ai/metrics-guidelines.md) and the [metrics review prompt](../../../.github/review-prompts/metrics.md) - Docs, examples, and recipes: the `dynamo-docs` skill - Any directory: its nearest `AGENTS.md` @@ -94,4 +95,6 @@ Fill in every section of `.github/pull_request_template.md`, and title the PR pe **Validation.** Back each claim with evidence, ideally a table with one row per behavior: the case, what happened before, and what happens now. For bug fixes, explain what caused the problem and why the fix addresses it. Include performance data when the change claims or risks a performance impact. Name each key test added and the regression it catches. Do not list routine test runs, clippy or linter checks, or unnecessary detail. +**Metrics.** For each new metric, include the block that the [metrics guidelines](../../../.ai/metrics-guidelines.md#before-you-add-a-metric) ask for: its reader, labels, series count, why existing metrics or PromQL do not answer the question, and its catalog entry. + **Issues and Ongoing Work.** Reference the GitHub or Linear issue the PR implements, for example `Closes #8480` or `Closes DYN-1234`. List any similar PRs, issues, or code on `main` that the Existing work check found, each with one line on how this PR differs; if the search found nothing, say so. As the final step, run the `dynamo-issue-first` skill to validate the issue and its reference. diff --git a/.ai/metrics-guidelines.md b/.ai/metrics-guidelines.md new file mode 100644 index 000000000000..15cf988ecb8d --- /dev/null +++ b/.ai/metrics-guidelines.md @@ -0,0 +1,281 @@ + + + +# Metrics Guidelines + +Rules for adding or changing a metric, a log event, or a trace span in Dynamo. They keep +metrics useful, cheap to run, and stable for the people who build dashboards and alerts on +them. + +The rules apply to every new or changed metric, log event, and span, in Rust, Python, and Go. +Existing code that breaks a rule is not a precedent: do not copy it, and do not rename an +existing metric as a side effect of another change (see +[Stability and deprecation](#stability-and-deprecation)). Engine metrics that Dynamo passes +through unchanged (`vllm:*`, `sglang:*`, `lmcache:*`, and the TensorRT-LLM `trtllm_*` metrics) +keep their engine's names and are out of scope. A metric that Dynamo computes is a Dynamo +metric, even when an engine prefix filter passes it through. + +Each rule has an ID, such as **N2**, so that a review can cite it. An exception to a rule +needs a written reason in the pull request. + +## Metric, log, or trace + +**C1.** A metric exists only if something reads it continuously (an alert, an SLO, a +dashboard, or another Dynamo component) and its labels have a small, known set of values. +Everything else is a structured log event. + +| If | Then | +|----|------| +| You can name the dashboard, alert, or component that reads it, and the labels are few and bounded | Metric | +| You need a request, tenant, session, or user ID to make it useful | Log event | +| The reader is a report or an investigation (billing, audit, post-incident) | Log event | +| You need a live signal and the detail behind it | One small metric for the aggregate, and one log event for the detail | +| You want to see where one request spent its time across components | Trace span (sampled) | + +For example, failed requests are counted in +`dynamo_frontend_requests_total{model, endpoint, request_type, status, error_type}`, which +feeds an error-rate alert. The request ID and tenant of each failure go in a log event. +`record_unsupported_residency_event` in `lib/kv-router/src/indexer/metrics.rs` shows the +pair in code: it increments a counter whose labels have a fixed set of values, and it logs +the worker and rank as fields of a rate-limited event. + +## Before you add a metric + +Answer these questions in the pull request description. If an answer rules the metric out, +use a log event instead. + +**A1. Reader.** Name the alert, SLO, dashboard panel, or component that reads the metric. +"It might be useful" is not a reader. In-repo readers include the Grafana dashboards under +`dev/observability/grafana_dashboards/` and `deploy/observability/`, and the planner's +queries in `components/src/dynamo/planner/monitoring/`. + +**A2. Not a query.** If PromQL over existing metrics gives the same answer, do not add the +metric. Name the PromQL in the pull request when you rule it out. + +**A3. Not an engine metric.** Do not re-export an engine metric under a `dynamo_` name; +Dynamo already passes engine metrics through. Add a Dynamo metric only when it measures +something the engine cannot see (the frontend's or the router's view), or when Dynamo +components need the same signal from every engine under one name. In that case, normalize +it across engines and document the mapping from each engine. + +**A4. Not a sibling.** If the metric differs from an existing one by one dimension (a +modality, a direction, a role), add a label instead of a new name. When the existing metric +is stable, adding a label is a breaking change (**S1**): add the labeled metric under a new +name and deprecate the old one. + +**A5. Static configuration.** Values that never change while the process runs (context +length, block size) go in one `*_info` metric with value 1 and each parameter as a label, +for example `dynamo_model_info{model, context_length, kv_cache_block_size}`. + +**A6. Series count.** Write two numbers in the pull request, each as a formula and as a +number at a stated scale. Both are per process. + +- **Label combinations:** multiply the largest number of values that each label you declare + can reach. Name what drives each number: a fixed set (`status` has 3 values) or the + deployment's configuration (served models, LoRA adapters). Labels that the runtime adds + have one value per registration, so multiply by the number of endpoints or components that + register the metric in one process. Count every value a process can see over its + life: a series stays exported until code removes it, so models or adapters that come and go + accumulate. +- **Series:** label combinations times the series per combination: 1 for a counter or gauge, + and buckets + 3 for a histogram (`+Inf`, `_sum`, and `_count`). Python `prometheus_client` + also exports a `_created` series, so count 2 per counter combination and buckets + 4 per + histogram combination. Each series costs memory in Prometheus, and the cost across the fleet + is this number times the number of scraped processes. + +For example, a histogram with 10 buckets and `model` as its only label has `models` +combinations and `models x (10 + 3)` series: 130 per frontend at 10 models. A counter with +`model` and a 3-value `status` has `models x 3` combinations, and as many series in Rust or Go. + +If a metric can reach more than 100 label combinations in one process, justify it in the pull +request or use a log event or a trace instead (**L1**). A label whose values grow with the +number of workers is not allowed (**L5**). + +**A7. Documentation.** Document the metric in the same pull request (**D1**, **D2**). + +Write the answers as one block per metric in the pull request description: + +```text +Metric: dynamo_frontend_prefill_queue_wait_seconds (histogram) +Reader: "Prefill queue wait p95" panel in the router dashboard; alert at 2 s +Labels: model (served models; 10 in the reference deployment) +Series: models combinations; models x (10 buckets + 3) = 130 series per frontend at 10 models +Query?: No. Existing stage_duration_seconds does not split out queue wait. +Docs: Added to the Metrics Catalog page +``` + +## Naming + +Apply these rules to the exported name, which is what dashboards and alerts see. The APIs +can add a prefix or a suffix to the name in the code. + +**N1.** The exported name is snake_case and starts with `dynamo__`. + +**N2.** The name ends with its unit in base form: `seconds` (not `ms`, `us`, or `ns`), `bytes` +(not `KB` or `GB`), `tokens`, or `ratio` for a value from 0 to 1 (not a percentage). For +example, `request_duration_seconds` and `kv_cache_usage_ratio`. + +**N3.** A counter only goes up (requests, tokens, errors), ends in `_total`, and is charted +with `rate()`. A counter with a unit puts the unit before `_total`, as in +`detokenize_seconds_total`. + +**N4.** A gauge can go up and down (in-flight requests, cache usage) and does not end in +`_total`. + +**N5.** Do not put a label name or a label value in the metric name. Use a label. + +**N6.** The help text says what is measured and matches the value. If the code measures +something different from what the name says, fix the name. Changing what a metric measures +means a new name. + +```text +# BAD -- unit not in base form, percentage, counter without _total, label value in the name +dynamo_frontend_queue_wait_ms +dynamo_component_cache_usage_percent +dynamo_frontend_rejected_requests +dynamo_frontend_images_per_request + +# GOOD +dynamo_frontend_queue_wait_seconds +dynamo_component_cache_usage_ratio +dynamo_frontend_rejected_requests_total +dynamo_frontend_multimodal_items_per_request{modality} +``` + +## Labels + +**L1.** Aim for fewer than 10 values per label. A label that can reach about 100 values +does not belong on a metric; use a log event or a trace. + +**L2.** Never use a request, session, user, or tenant ID, a prompt, a URL path, or a raw +client-supplied value as a label. Map a client-supplied value to a known, bounded set +first, as `categorizeError` does in `deploy/operator/internal/observability/metrics.go`. + +**L3.** The `model` label comes from the set of served models, never from the raw value in +the request. A request that names a model that is not served uses one fixed value. In the +frontend, resolve the label with `ModelManager::metric_model_for`, which returns the +existing sentinel `unknown_model`, so that all frontend metrics agree. A component with no +existing sentinel uses `unknown`, as the endpoint picker's `set_served_model` does. + +**L4.** Summing over a label must give a number that means something. A label that mixes +different quantities, such as `kind` with the values `requests` and `bytes`, should be +separate metrics. Static `*_info` metrics are the exception. + +**L5.** Do not declare a label whose values grow with the number of workers, such as +`worker_id` or `dp_rank` on a frontend, router, or planner metric: every such process would +then hold series for the whole fleet. Export a per-worker value from the worker's own +metrics instead, where the `worker_id` that the runtime adds identifies the worker; that +label has one value per process and is not affected by this rule. On a worker's own metrics, +add a `dp_rank` label only when an autoscaler or the router needs per-rank values. + +**L6.** Pre-create the expected values of bounded labels (such as `status` and `endpoint`) +at startup, so that `rate()` and `absent()` alerts work from the first scrape. Create the +`model` values when the model registers, because they are not known at startup. + +**L7.** Histograms are the main cost. Use about 10 buckets, put a bucket edge at every alert +threshold, and use `model` as the only label that you declare. + +## Registration + +**R1.** Register a metric at startup in every process that updates it, not on first use. +A labeled family has no output until it has at least one child, so registration alone does +not make it visible: pre-create its children as well (**L6**). + +**R2.** Do not add a metric that no production code updates. + +**R3.** When you add, rename, or remove a name in `lib/runtime/src/metrics/prometheus_names.rs`, +regenerate its Python mirror in the same pull request with +`cargo run -p dynamo-codegen --bin gen-python-prometheus-names`. Never edit the mirror by hand. + +## Patterns to follow + +The right-hand column shows the shape to aim for in new metrics. It is not a list of metrics +that exist today. + +| Instead of | Do this | +|------------|---------| +| `images_per_request`, `videos_per_request`, `audio_per_request` | One `multimodal_items_per_request{modality}` | +| `model_context_length`, `model_kv_cache_block_size`, `model_max_num_seqs` | One `model_info{model, context_length, kv_cache_block_size, max_num_seqs}` with value 1 | +| `num_prefill_replicas`, `num_decode_replicas` | One `num_replicas{sub_component_type}` | +| `router_overhead_total_ms`, or a "percent" gauge that holds 0 to 1 | `_seconds` and `_ratio` names | +| A counter of events above a threshold, next to a histogram with that bucket edge | `increase(x_count[5m]) - increase(x_bucket{le="T"}[5m])` in PromQL, where `T` is an existing bucket edge as exported | + +## Stability and deprecation + +Renaming or reshaping a metric breaks dashboards and alerts, often silently: a panel just +shows no data. So every metric has one of two stability levels. + +- **Experimental:** marked as experimental in the Metrics Catalog (**D1**). It can change or + disappear in any release, with a release note. +- **Stable:** documented in the Metrics Catalog without an experimental mark, that is, with + the Status `Current` in a table. A metric becomes stable after at least one release as + experimental, once a maintainer has reviewed its name, unit, type, and labels, and a test + checks that the code updates it. + +A new metric therefore starts as experimental. A catalog entry with no experimental mark is +stable, including an entry whose name breaks these rules. + +**S1.** Do not rename a stable metric, change its type or unit, or add or remove labels. Add +a new metric and deprecate the old one. Adding a new value to an existing label is allowed +when existing queries stay correct (for example, `sum()` over the label still means the +same thing). + +**S2.** To deprecate a stable metric, mark it deprecated in the Metrics Catalog and in its +help text, name its replacement or a PromQL recipe, and name the release that removes it. +Move in-repo readers (**A1**) to the replacement, and state the deprecation in the pull +request description so that it can go into the release notes. + +**S3.** Keep a deprecated metric for at least two releases after the announcement, then +remove it. Never remove it in a patch release. + +**S4.** Remove a metric only if every question it answers can still be answered from what +remains, with the same meaning, measurement point, label granularity, scrape target, +timeliness, and availability. A metric that was never registered or never updated can be +removed without deprecation. + +Not promised as stable: engine metric names and labels (vLLM, SGLang, and TensorRT-LLM +metrics pass through under their own names), experimental metrics, per-worker debugging +gauges, and metrics that are not in the Metrics Catalog. + +## Documentation + +**D1.** The [Metrics Catalog](../docs/fern/pages/reference/observability/metrics-catalog.mdx) +is the source of truth for metric documentation, and a metric that is not in it is +unsupported. Operator metrics are documented in +[Operator Metrics](../docs/fern/pages/reference/observability/operator-metrics.mdx) instead, +and every rule here about the catalog applies to that page too. In the same pull request, +add or update the metric's entry: its type, unit, labels and their expected values, and +stability level. If no section fits the metric's family, add one. + +- In a table, set the **Status** column to `Experimental` for a new metric, or to + `Deprecated` for a deprecated one, and name the replacement and the removal release in its + description. +- In a detailed `ParamField` entry, start a new metric's description with + `**Experimental.**`, or mark a deprecated metric with `deprecated={true}` and name the + replacement and the removal release in its description. + +**D2.** Update the `AGENTS.md` nearest to the code that owns the metric to say what the +metric is and how it can be used. The "Frontend Serving-Load Metrics" section of +`lib/llm/AGENTS.md` is an example. + +## Logs and traces + +**T1.** Logs are structured JSON, one event per line. Write each event so that its details +are separate fields when the JSONL console format is on +(`DYN_LOGGING_CONSOLE_FORMAT=jsonl`; the default is readable text). + +- Rust: use `tracing` macros with `key = value` fields and a fixed message, for example + `tracing::warn!(worker_id, dp_rank, "Ignoring KV event")`. +- Python: the log bridge forwards only the formatted message to Rust, so fields passed with + `extra=` are lost. Put the values in lazy `%s` arguments, as `.ai/python-guidelines.md` + requires. + +**T2.** Request-path log events carry `x_request_id`, `trace_id`, and `span_id`. + +- Rust: emit the event inside the request's span; the JSONL formatter takes these IDs from + the span. Attach the span to spawned tasks with `.instrument(...)` or `in_current_span()`. +- Python: include the request ID with `context.id()`; `context.trace_id` and + `context.trace_headers()` give the trace context. + +**T3.** Use a trace span, which is sampled, to show where one request spent its time across +components. Do not add a metric label to get that view. diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 79027a5f98f9..bc369791e4f4 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -37,9 +37,14 @@ reviews: instructions: "Review against .ai/bash-launch-guidelines.md" - path: "**/*.{py,rs,go,ts,tsx,js,mjs,cjs,sh,yaml,yml,toml}" instructions: "Flag internal Linear ticket references (e.g. DIS-XXXX, DYN-XXXX, OPS-XXXX, DEP-XXXX, or any -NNNN ID) in the added lines; see .ai/linear-ticket-refs.md for what to recommend." + - path: "**/*.{rs,py,go}" + instructions: "If the change adds or modifies a metric, a log event, or a trace span, review it against .ai/metrics-guidelines.md, using the checks and finding format in .github/review-prompts/metrics.md." + - path: "docs/fern/pages/reference/observability/*.mdx" + instructions: "Review metric entries that the change adds, deprecates, or removes against the Stability and Documentation rules in .ai/metrics-guidelines.md." knowledge_base: code_guidelines: filePatterns: - ".ai/**/*.md" + - ".github/review-prompts/metrics.md" chat: auto_reply: true diff --git a/.github/codeowners/areas.yaml b/.github/codeowners/areas.yaml index fb594be65413..a613c4f6fce0 100644 --- a/.github/codeowners/areas.yaml +++ b/.github/codeowners/areas.yaml @@ -589,6 +589,10 @@ shared: owners: - ops - runtime +- glob: .github/review-prompts/metrics.md + owners: + - ops + - observability # CODEOWNERS stays on the process area alone. GitHub requests every team on # the matching line, even when one approval satisfies the gate, so listing # every area here pages every codeowner group for a generated-file edit. @@ -625,6 +629,10 @@ shared: owners: - process - agents +- glob: .ai/metrics-guidelines.md + owners: + - process + - observability - glob: .github/workflows/fern-docs.yml owners: - ops diff --git a/.github/review-prompts/README.md b/.github/review-prompts/README.md index ae0520fdc64f..d2de16f13a90 100644 --- a/.github/review-prompts/README.md +++ b/.github/review-prompts/README.md @@ -53,3 +53,14 @@ Keep these prompts in plain English and usable directly when updating them. Add concern when multiple source discussions substantiate it, retain the concrete behavioral distinction, and avoid turning a historical fix into an unconditional requirement for unrelated changes. + +## Metrics review prompt + +The [metrics](metrics.md) prompt is not a codeowner prompt. It applies the +[metrics guidelines](../../.ai/metrics-guidelines.md) to the metrics, log events, and +trace spans that a change adds or modifies, in any area of the repository. Its checks +come from that written policy rather than from review history, and each one cites the +guideline rule it enforces. The guideline owns the rules: change a rule there first, +then update the check that cites it. The prompt also defines what a finding must +contain, so that each one names the correction, and it keeps questions that only the +author can answer, such as a metric's reader, separate from defects. diff --git a/.github/review-prompts/metrics.md b/.github/review-prompts/metrics.md new file mode 100644 index 000000000000..a6164b1f3863 --- /dev/null +++ b/.github/review-prompts/metrics.md @@ -0,0 +1,216 @@ + + +# Metrics review + +Review the metrics, log events, and trace spans that the proposed change adds or modifies, and +check them against the [metrics guidelines](../../.ai/metrics-guidelines.md). The guidelines own +the rules; cite their IDs, such as **N2**, in every finding. Read the diff and enough surrounding +code to establish each metric's exported name, type, labels, and the code that registers and +updates it. Apply the checks to any area of the repository that the change touches. + +A metric's name, type, unit, and labels are a contract that dashboards and alerts depend on. A +rule violation is a contract defect, not a style preference. + +## Find the telemetry + +1. Look for metric definitions and changes: + - Rust: new `pub const` names in `lib/runtime/src/metrics/prometheus_names.rs`; + `.metrics().create_*` and `create_metric_with_options` calls; `prometheus` crate + constructors such as `IntCounterVec::new`; custom `Collector` implementations. + - Python: `prometheus_client` `Counter`, `Gauge`, `Histogram`, `Info`, and `Enum`; registry + bridges such as `register_engine_metrics_callback`, + `register_prometheus_expfmt_callback`, and `register_prometheus_typed_callback`. + - Go: `client_golang` constructors such as `prometheus.NewGaugeVec`, and registration in + `InitMetrics`. + - Docs: `metrics-catalog.mdx`, `operator-metrics.mdx`, and `metric-labels.mdx` under + `docs/fern/pages/reference/observability/`. + +2. Look for added log events (`tracing::` macros, Python `logging` and `logger` calls) on request + paths, and for added spans (`tracing` span macros, `#[instrument]`, `.instrument(...)`, and + `dynamo.common.backend.telemetry`). + +3. Skip test-only code (`tests/`, `#[cfg(test)]`, and code behind the `bench` feature) and the + generated `lib/bindings/python/src/dynamo/prometheus_names.py`; review its Rust source + instead. When the change adds, renames, or removes a constant in `prometheus_names.rs` + without regenerating that file, report it under **R3** with the regenerate command. If the + change adds or modifies no metric, log event, or span, say so and stop. + +4. A metric that the change registers, exports, or updates for the first time is new for this + review. Check its whole definition (name, type, unit, labels, and buckets), even where those + lines are unchanged. + +## Build the metric table + +5. For each new or changed metric, record its exported name, type, declared labels with the + source of each label's values, series per process, registration site, production update + site, catalog entry, and stability level. Resolve the exported name the way the API builds + it: + - Rust `create_*` prepends `dynamo_component_`, or the prefix given in `MetricOptions`. A + full `dynamo_...` name passed in gets the prefix twice. + - Go joins `Namespace` and `Name` with `_`. + - Python `prometheus_client` adds `_total` to a counter's exported name. + +6. Count only the labels that the author declares. The runtime adds `dynamo_namespace`, + `dynamo_component`, `dynamo_endpoint`, and `worker_id`, and the Python bridges add `model` + and `model_name`; do not report those against **L1**, **L5**, or **L7**. + +## Checks + +### Metric or log + +7. **C1, L2.** Report a label whose value is a request, session, user, or tenant ID, a prompt, a + URL path, or a value copied from a request or protocol field. The correction moves the detail + to a log event with fields and keeps, at most, an aggregate counter with bounded labels. + +### Justification + +8. **A1-A6.** Check the pull request description for the Metric, Reader, Labels, Series, + Query?, and Docs block for each new metric. When the block or an answer is missing, ask for + it under "Questions for the author"; do not report it as a defect. When you cannot see the + description, ask the questions anyway. Check both numbers that **A6** asks for against the + declared labels, the buckets, and the number of endpoints or components that register the + metric in one process: label combinations, and series (1 per combination for a counter + or gauge, buckets + 3 for a histogram in Rust and Go, and in Python buckets + 4 per histogram + and 2 per counter combination). Report a metric whose declared labels alone can exceed 100 + combinations in one process when the pull request does not justify it; ask when the count + depends on deployment values the diff does not show. + +9. **A2, A4, A5.** Search `prometheus_names.rs`, the Metrics Catalog, and + `metrics-comparison.md` for an existing metric with the same measurement point, or one that + differs only by a modality, a direction, or a role. Report only a concrete match: name both + metrics, and give the PromQL that replaces the new one or the label to add. Report static + configuration exported as separate gauges, and give the `*_info` metric that replaces them. + +10. **A3.** Report a new `dynamo_` metric that copies a value the engine already exports, unless + the pull request says which engines it normalizes and documents the mapping. + +### Naming + +11. **N1-N5.** Check the exported name: the `dynamo__` prefix; a unit in base form + (not `_ms`, `_us`, `_ns`, `_percent`, `_kb`, or `_gb`); `_total` on a counter, after the + unit; no `_total` on a gauge; no label name or value in the name. The correction gives the + exact replacement name and, in Rust, the constant to change. + +12. **N6.** Read the update site and confirm that the value matches the name and help text. For + example, a `_seconds` histogram that observes milliseconds is a defect. N6 concerns the + quantity, unit, and measurement point. A change that only alters when the value is published, + such as batching or flushing updates, is not an N6 defect; raise stale wording on unchanged + lines as a question, not a finding. + +### Labels + +13. **L1, L3, L4.** Confirm that each declared label takes its values from a bounded set: an + enum, a constant, or a mapping with a fallback value. The frontend resolves `model` with + `ModelManager::metric_model_for`; other components use a fixed `unknown` value. Report a + label that mixes different quantities. + +14. **L5.** Report a new declared label whose values grow with the number of workers, such as + `worker_id` or `dp_rank` on a frontend, router, or planner metric. The correction exports + the value from the worker's own metrics, where the runtime's `worker_id` identifies it. For + a new `dp_rank` label on a worker's own metric, ask which autoscaler or router needs + per-rank values when the pull request does not say. + +15. **L6, R1.** Report a new labeled family whose children are created only in the request path, + or whose registration happens on first use, for example inside `OnceLock::get_or_init` on + the update path. Before reporting, search the code at the pull request head for a seeding + call at startup or at model registration. A registered family with no children does not + appear on `/metrics`. + +16. **L7.** For a new histogram, check for about 10 buckets, an edge at each named alert + threshold, and `model` as the only declared label. A Rust histogram created with `None` + buckets gets the default edges for seconds (0.005 to 10), which do not fit tokens or bytes. + When a correction introduces or changes a histogram, give a literal bucket list in the + metric's unit, and name the edge that matches each alert threshold or say that none is named. + +17. **R2.** Report a new metric with no production update site at the pull request head. Also + confirm that each request path that the name and help text cover updates the metric (for + example, streaming and unary, token and text modes, aggregated and disaggregated), and report + a path that registers the family but never updates it. + +### Stability + +18. **S1-S4.** For a change to an existing metric, decide its stability from its catalog entry + (Operator Metrics for operator metrics) at the base revision, for example with + `git show :`, because a pull request that removes a metric may also + remove its entry. A table row with the Status `Current` is stable, `Experimental` is + experimental, and `Deprecated` is deprecated; a detailed entry is stable unless its + description starts with `**Experimental.**` or it carries `deprecated={true}`. Report, on a + stable metric, a rename, a type or unit change, or an added or removed label that does not + add a new metric and deprecate the old one. Report a deprecation whose entry at the pull + request head lacks the catalog mark, or that lacks the help-text note, the replacement or + PromQL recipe, or the removal release. Report a removal of a stable metric that has not been + deprecated for two releases, or any removal on a `release/*` base branch. Search the in-repo + readers (dashboards under `dev/observability/` and `deploy/observability/`, and the planner) + for a renamed or removed name, and report each reader that still uses it. + +### Documentation + +19. **D1.** Report a new or changed Dynamo metric whose catalog entry the pull request does not + add or update, and a new entry that lacks the type, unit, labels and their values, or the + experimental mark (the Status `Experimental` in a table, or `**Experimental.**` at the start + of a detailed entry). + +20. **D2.** When the pull request adds a metric and does not describe it in the `AGENTS.md` + nearest to the owning code, ask for that description under "Questions for the author" and + name the file. Do not ask for a new `AGENTS.md` file. + +### Logs and traces + +21. **T1, T2.** On added lines only, report a Rust request-path event that formats an ID into + its message instead of passing it as a field, a spawned request-path task that does not + carry the span (`.instrument(...)` or `in_current_span()`), a Python request-path warning or + error that omits `context.id()`, and Python fields passed with `extra=`, which the log + bridge drops. Do not report a log event for not being JSON; the output format is deployment + configuration. + +22. **T3.** Report a metric label added to follow one request across components; a span gives + that view. Report span parentage defects only as the runtime review prompt describes them, + and do not invent span naming rules. + +## Out of scope + +Do not report: + +- A violation on a line that the change does not add or modify, including code that the change + only moves. A metric that the change registers, exports, or updates for the first time is not + pre-existing (step 4). +- Engine metrics that Dynamo passes through (`vllm:`, `sglang:`, `lmcache:`, and the engine's + `trtllm_` metrics), or changes to the prefix filters that pass them. +- A rule for which the pull request description records an exception with a reason. Mention the + exception once; do not argue it again. + +## Reporting findings + +Report as findings only rule violations that the change introduces and that you verified in the +code at the pull request head. Each finding states: + +- **Location:** the file and line of the definition or update. +- **Rule:** the rule IDs. +- **Problem:** the exported name, label, or line, quoted. +- **Consequence:** the concrete cost, such as the series count with its arithmetic, the + dashboard or alert that would break, a unit that readers would misread, or a series that is + absent until the first request. +- **Correction:** the exact change: the replacement name and constant, the label set, the + seeding call, the log event or PromQL to use instead, or the catalog entry to add. + +Keep one finding per underlying defect. When one fact breaks several rules, report one finding +that cites all of them; when one defect repeats across metrics added together, report it once +and list every location. Put findings about names, types, units, labels, stability, and +unbounded series first, because they are hardest to change after release. For example: + +```text +lib/llm/src/http/service/metrics.rs:1412 (N2, N3) +Problem: new IntCounterVec "prefill_wait_ms" exports dynamo_frontend_prefill_wait_ms{model}. +Consequence: the unit is not base form and the counter has no _total, so rate() panels show + milliseconds per second under a name that reads as a gauge; renaming after release breaks them. +Correction: record seconds and export dynamo_frontend_prefill_wait_seconds_total: rename the + constant to PREFILL_WAIT_SECONDS_TOTAL = "prefill_wait_seconds_total" in prometheus_names.rs. +``` + +Put information that only the author can give (a reader, a series count, a label's source, which +processes update a metric) in a separate "Questions for the author" block, and do not present it +as a defect. End with the metric table from step 5. If no finding qualifies, state that no new +findings were identified; this does not establish that the change is ready to approve. diff --git a/AGENTS.md b/AGENTS.md index a714920139c2..a1680dd09f9a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -18,7 +18,7 @@ SPDX-License-Identifier: Apache-2.0 | `docs/fern/` | Fern docs site; pages are in `pages/` | | `examples/`, `recipes/` | Runnable examples and deployment recipes | | `benchmarks/`, `tests/` | Benchmark harnesses and the top-level pytest suite | -| `.ai/` | Topic guidelines for agents (pytest, Python, CI, launch scripts, AISimulate); read the matching one before writing that kind of code | +| `.ai/` | Topic guidelines for agents (pytest, Python, CI, launch scripts, AISimulate, and metrics, logs, and traces); read the matching one before writing that kind of code | | `.agents/skills/` | Agent skills (see [Skills](#skills)) | | `agents/`, `agent-docs/` | Role contracts, guides, and rules for the recipe optimization loop | diff --git a/CODEOWNERS b/CODEOWNERS index 14790e356336..5bf11ca6f806 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -647,6 +647,7 @@ requirements.txt /recipes/**/tokenspeed/** @ai-dynamo/dynamo-performance-codeowners @ai-dynamo/dynamo-tokenspeed-codeowners /recipes/**/vllm/**/*xpu* @ai-dynamo/dynamo-performance-codeowners @ai-dynamo/dynamo-backend-vllm-codeowners @ai-dynamo/dynamo-xpu-codeowners /tests/serve/test_vllm.py @ai-dynamo/dynamo-runtime-codeowners @ai-dynamo/dynamo-backend-vllm-codeowners +/.ai/metrics-guidelines.md @ai-dynamo/dynamo-process-codeowners @ai-dynamo/dynamo-observability-codeowners /deploy/operator/Dockerfile @ai-dynamo/dynamo-operator-codeowners @ai-dynamo/dynamo-ops-codeowners /lib/llm/src/request_trace/ @ai-dynamo/dynamo-runtime-codeowners @ai-dynamo/dynamo-agents-codeowners @ai-dynamo/dynamo-observability-codeowners /lib/mocker/servers/sglang/ @ai-dynamo/dynamo-performance-codeowners @ai-dynamo/dynamo-backend-sglang-codeowners @ai-dynamo/dynamo-sidecar-codeowners @@ -682,6 +683,7 @@ requirements.txt /lib/kvbm-config/src/messenger.rs @ai-dynamo/dynamo-kv-memory-codeowners @ai-dynamo/dynamo-runtime-codeowners /lib/runtime/src/system_health.rs @ai-dynamo/dynamo-runtime-codeowners @ai-dynamo/dynamo-observability-codeowners /tests/serve/multimodal_profiles/ @ai-dynamo/dynamo-runtime-codeowners @ai-dynamo/dynamo-multimodal-codeowners +/.github/review-prompts/metrics.md @ai-dynamo/dynamo-ops-codeowners @ai-dynamo/dynamo-observability-codeowners /.github/review-prompts/runtime.md @ai-dynamo/dynamo-ops-codeowners @ai-dynamo/dynamo-runtime-codeowners /docs/fern/pages/use-cases/agents/ @ai-dynamo/dynamo-docs-codeowners @ai-dynamo/dynamo-agents-codeowners /examples/deployments/**/sglang/** @ai-dynamo/dynamo-operator-codeowners @ai-dynamo/dynamo-backend-sglang-codeowners diff --git a/docs/fern/pages/developer-guide/advanced-customizations/metrics-developer-guide.md b/docs/fern/pages/developer-guide/advanced-customizations/metrics-developer-guide.md index 28444d163874..d565e65e2a13 100644 --- a/docs/fern/pages/developer-guide/advanced-customizations/metrics-developer-guide.md +++ b/docs/fern/pages/developer-guide/advanced-customizations/metrics-developer-guide.md @@ -7,6 +7,8 @@ subtitle: Reference for creating counters, gauges, and histograms with Dynamo's This guide explains how to create and use custom metrics in Dynamo components using the Dynamo metrics API. +Before you add or change a metric, read the [metrics guidelines](https://github.com/ai-dynamo/dynamo/blob/main/.ai/metrics-guidelines.md). They cover when to use a metric instead of a log or a trace, and the rules for names, labels, registration, stability, and documentation. + ## Metrics Exposure All metrics created via the Dynamo metrics API are automatically exposed on the `/metrics` HTTP endpoint in Prometheus Exposition Format text when the following environment variable is set: @@ -87,10 +89,10 @@ latency.observe(0.023); // 23ms ```rust // Create vector metrics with label names -let requests_by_model = endpoint.metrics().create_countervec( - "requests_by_model", - "Requests by model", - &["model_type", "model_size"], +let model_requests_total = endpoint.metrics().create_countervec( + "model_requests_total", + "Requests handled, by served model", + &["model"], &[] )?; @@ -101,8 +103,8 @@ let memory_by_gpu = endpoint.metrics().create_gaugevec( &[] )?; -// Use with specific label values -requests_by_model.with_label_values(&["llama", "7b"]).inc(); +// Use with specific label values; take the model from the served models, not the request +model_requests_total.with_label_values(&["Qwen/Qwen3-0.6B"]).inc(); memory_by_gpu.with_label_values(&["0", "allocated"]).set(8192.0); ``` diff --git a/lib/runtime/src/metrics/prometheus_names.rs b/lib/runtime/src/metrics/prometheus_names.rs index be7a21ae0c04..ad80bf3ed1bc 100644 --- a/lib/runtime/src/metrics/prometheus_names.rs +++ b/lib/runtime/src/metrics/prometheus_names.rs @@ -15,13 +15,19 @@ //! //! ## Naming Conventions //! +//! Before adding or changing a name here, read `.ai/metrics-guidelines.md`, which also covers +//! labels, registration, stability, and documentation. Existing names that break these +//! conventions are kept for compatibility; do not copy them, and do not rename them as a +//! side effect of another change. +//! //! All metric names should follow: `{prefix}_{name}_{suffix}` //! //! **Prefix**: Component identifier (`dynamo_component_`, `dynamo_frontend_`, etc.) //! **Name**: Descriptive snake_case name indicating what is measured //! **Suffix**: -//! - Units: `_seconds`, `_bytes`, `_ms`, `_percent`, `_messages`, `_connections` -//! - Counters: `_total` (not `total_` prefix) - for cumulative metrics that only increase +//! - Units, in base form: `_seconds` (not `_ms`), `_bytes`, `_tokens`, and `_ratio` for a +//! value from 0 to 1 (not `_percent`) +//! - Counters: `_total` (not `total_` prefix), after the unit - for cumulative metrics that only increase //! - Gauges: No `_total` suffix - for current state metrics that can go up and down //! - Note: Do not use `_counter`, `_gauge`, `_time`, or `_size` in Prometheus names (too vague) //! @@ -29,14 +35,13 @@ //! - ❌ `_counter` → ✅ `_total` //! - ❌ `_sum` → ✅ `_total` //! - ❌ `_gauge` → ✅ (no suffix needed for current values) -//! - ❌ `_time` → ✅ `_seconds`, `_ms`, `_hours`, `_duration_seconds` -//! - ❌ `_time_total` → ✅ `_seconds_total`, `_ms_total`, `_hours_total` -//! - ❌ `_total_time` → ✅ `_seconds_total`, `_ms_total`, `_hours_total` +//! - ❌ `_time`, `_ms` → ✅ `_seconds`, `_duration_seconds` +//! - ❌ `_time_total`, `_total_time`, `_ms_total` → ✅ `_seconds_total` //! - ❌ `_total_time_seconds` → ✅ `_seconds_total` -//! - ❌ `_average_time` → ✅ `_seconds_avg`, `_ms_avg` -//! - ❌ `_size` → ✅ `_bytes`, `_total`, `_length` -//! - ❌ `_some_request_size` → ✅ `_some_request_bytes_avg` -//! - ❌ `_rate` → ✅ `_per_second`, `_per_minute` +//! - ❌ `_average_time` → ✅ a `_seconds` histogram; its average is `rate(x_sum[5m]) / rate(x_count[5m])` +//! - ❌ `_size` → ✅ `_bytes`, `_tokens` +//! - ❌ `_percent` → ✅ `_ratio` (0 to 1) +//! - ❌ `_rate`, `_per_second` → ✅ a `_total` counter, charted with `rate()` //! - ❌ `disconnected_clients_total` → ✅ `disconnected_clients` (gauge, not counter) //! - ❌ `inflight_requests_total` → ✅ `inflight_requests` (gauge, not counter) //! - ❌ `connections_total` → ✅ `current_connections` (gauge, not counter) @@ -47,9 +52,9 @@ //! - ✅ `dynamo_component_errors_total` - Total error counter (not `total_errors`) //! - ✅ `dynamo_component_memory_usage_bytes` - Memory usage gauge //! - ✅ `dynamo_frontend_active_requests` - Current active requests gauge -//! - ✅ `dynamo_component_cpu_usage_percent` - CPU usage percentage -//! - ✅ `dynamo_frontend_tokens_per_second` - Token generation rate -//! - ✅ `dynamo_messaging_client_connection_duration_ms` - Connection time in milliseconds +//! - ✅ `dynamo_component_cpu_usage_ratio` - CPU usage from 0 to 1 +//! - ✅ `dynamo_frontend_output_tokens_total` - Output token counter; `rate()` gives tokens per second +//! - ✅ `dynamo_messaging_client_connection_duration_seconds` - Connection time histogram //! - ✅ `dynamo_messaging_client_current_connections` - Current active connections gauge //! - ✅ `dynamo_messaging_client_in_messages_total` - Total messages received counter //!