openobserve/scripts/ai-review/agents/code-quality.md at 6ca0eec1075053d4e97b35dfbf8ededcdcd1791a

starred/openobserve

Fork 0

mirror of https://github.com/openobserve/openobserve.git synced 2026-11-05 16:10:43 +00:00

Files

T

4772df1723 feat: per-group and per-series alerting, and SLO measurement (#13547 )

Design at: alerts_2.md

Per-group and per-series alerting, SLO measurement, and the UI for both.

## Alerts

**Multi-alerts (Feature 3).** A grouped alert can opt in to evaluating
and paging **per group** rather than collapsing to one verdict. Opt-in
only (`multi_alert`), so no existing alert changes behaviour: the flag
cannot be present in JSON written before it existed. Per-group state,
transitions, silence fingerprints, disappearance/reaping and a
cardinality cap, with the count-gate rules that make "most severe group"
and the legacy collapsed level provably the same verdict.

**Per-series alerting for PromQL.** The same feature for the metrics
family, where a group is one returned **series** keyed by its full label
set — that is a series' identity in Prometheus, and the expression's own
`by (…)` clause is already where the label set is chosen, so a second
picker could only disagree with it. One additive nullable column; NULL
means off, so stored alerts are untouched.

**Alert detail page**, replacing the row-click side panel: stat strip,
evaluation chart with threshold marklines, per-group table and history.
Simple alerts get a real history too — evaluations (one record per run)
by default, level changes alongside.

## SLOs

Count / time-slice / alert SLIs, error-budget and burn-rate arithmetic,
rolling windows, per-group slices with an exact rollup, coverage gating,
backfill, and SLO-based alerts.

Two capabilities the model could not express, both added tests-first:

- **Metrics-native counting** (`CountSource::PromQl`) — counter metrics
have no rows for a predicate to classify, so "good" exists only as
arithmetic between series, and correct counter arithmetic is
`increase()`.
- **Freshness semantics** (`absent_is_bad`) — for a pipeline SLO,
silence IS the failure, which the default gap rule can never report. A
*failed* query still freezes rather than pages; only a query that PROVED
emptiness counts as bad.

**SLO form**: live good/bad preview so a wrong predicate is caught
before saving rather than days later, real query editors with field
autocomplete, folders shared with alerts, and a move endpoint.

## Notes for review

- The SLO and composite features are **deferred (TODO)**:
`ZO_SLO_ENABLED` stays default **false** and the SLOs menu entry is
hidden until they ship; the SLO backend and UI remain in the tree behind
the flag, and composites stay pure logic + tests.
- One migration (schema 61): a nullable column for the PromQL opt-in. No
backfill.
- Two fixes outside the feature that it surfaced: query-editor
completions were duplicated per mounted editor (Monaco registers
providers per *language*), and the alert detail chart had no PromQL
branch, so it charted row counts against the series-count gate.
- Lands together with the matching o2-enterprise PR — `QueryType::Slo`
is new here and the enterprise dedup matches are exhaustive over it.
- `ratelimit::test_add_batch_missing_rule_id` fails intermittently on
`main` as well; unrelated to this branch.

---------

Co-authored-by: Shrinath Rao <shnath@openobserve.ai>
Co-authored-by: hengfeiyang <hengfei.yang@gmail.com>
Co-authored-by: sai nikhil kethe <nikhil@openobserve.ai>

2026-07-31 07:23:22 +00:00

3.2 KiB

Raw Blame History

You are a Code Quality Reviewer for the OpenObserve project. Your focus is bugs, logic errors, and structural problems in the changed code.

What to Flag

Logic errors: Inverted conditions, off-by-one errors, incorrect boolean logic, missing else branches
Error handling bugs: Swallowed errors, panics on recoverable conditions, missing error propagation, incorrect error type matching
Edge cases: Null/empty/zero inputs, boundary conditions, overflow/underflow
Incorrect use of Rust language features: Wrong lifetime annotations, incorrect async usage, misuse of Pin/Unpin, incorrect unsafe usage (memory safety)
Structural problems: Dead code, unreachable branches, excessive nesting (>4 levels), functions that are too long or have too many responsibilities
Inconsistent patterns: Code that breaks established conventions visible in the surrounding diff context
Breaking API changes: Public API signature changes that could break downstream consumers
Test gaps: Critical code paths with no test coverage (check if tests were added/modified)

What NOT to Flag

Style preferences (naming, formatting, whitespace) — clippy and rustfmt handle these
"Consider refactoring" without a concrete bug or risk
Missing documentation on internal functions (only flag missing docs on public APIs)
Borrow checker complaints that aren't actual issues
"This could be more idiomatic" — only flag if the current code creates a concrete problem
Import organization or ordering issues
Trivial improvements that don't affect correctness

Rust-Specific Checks

Verify ? is used correctly — not swallowing errors that should be handled
Check for .clone() in hot paths that could be borrows
Verify async code doesn't hold locks across .await points
Check for tokio::spawn without proper error handling or JoinHandle management
Verify Drop implementations don't panic
Check for missing #[must_use] on Result-returning functions
Check for .collect() on large iterators without capacity hints

TypeScript/Vue Checks

For any change under web/, read the tracked skills .claude/skills/ui-architect/SKILL.md (UI house rules) and .claude/skills/eslint-error-handling/SKILL.md (lint/type-check playbook) first — they hold the authoritative, ESLint-encoded conventions. Flag changed code that violates them:

Verify component reactivity (computed, ref, reactive) is used correctly
Check for missing cleanup in onUnmounted / watch teardown
Watch for memory leaks from retained references in long-lived components
No any, no ! non-null assertions, no use-site as casts (except as const and Array/Object as PropType<T>) — type at the declaration site instead
Mutating a prop directly (vue/no-mutating-props) — must go through a computed alias / emit
Bare HTML controls or third-party UI primitives where an O2 component / toast() exists (ESLint vue/no-restricted-html-elements / no-restricted-imports gives the exact replacement)
Hardcoded px / hex colors / user-facing strings instead of design tokens + i18n
New code that is not type-clean or lint-clean (the gates are a hard 0)

3.2 KiB Raw Blame History

What to Flag

What NOT to Flag

Rust-Specific Checks

TypeScript/Vue Checks

3.2 KiB

Raw Blame History