Commit Graph

1 Commits

Author SHA1 Message Date
Phil 4797489281
telemetry: zero-initialize bounded-label metrics so an absent series means a broken scrape (#927)
* telemetry: materialize dropped-event counter children at 0

A labeled Prometheus counter exports no series until its first labels()
call, so telemetry_events_dropped stayed invisible until an event was
actually dropped — impossible to alert on or graph, and "no drops" was
indistinguishable from "metric missing / scrape broken".

Pre-create the (namespace, reason) children at 0 on emitter start, for
each reason the emitter can emit, so the metric is always present.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* telemetry: generalize counter zero-init to all bounded-label counters

Extends #927 (which zero-inited telemetry_events_dropped) to every counter
whose label domain is bounded and known at startup, so metrics are present in
Prometheus before their first event — a missing series then signals a broken
scrape rather than "nothing happened yet".

- add initialize_bounded_metrics(instance_type) on PrometheusMetrics; call it
  per-process from main.py (api) and deriver/__main__.py (deriver).
- extract a shared _touch() helper; refactor initialize_telemetry_dropped_metrics
  onto it (that one stays per-emitter in start() — it's prefix-dependent).
- explicit ALL_EVENT_TYPES / HIGH_VOLUME_EVENT_TYPES registry in telemetry.events,
  drift-guarded by tests that walk BaseEvent subclasses.
- only VALID (task_type, token_type, component) tuples for deriver_tokens (the
  cartesian product would fabricate impossible always-0 series); only high-volume
  event types for sampled_out; high-cardinality labels (endpoint, workspace_name)
  left open.
- gauges: zero-init embed_now_tasks_in_flight + telemetry_buffer_size; add a new
  message_embeddings_pending backlog gauge, set each reconciliation cycle and
  zero-inited at deriver startup (Rajat's pending/in-flight ask).
- backfills the tests #927 shipped without.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* review: task-aware deriver combos + fail-soft gauge zero-init

I1: _DERIVER_TOKEN_COMBOS was factored task-independently, materializing the
impossible (ingestion, input, previous_summary) series — previous_summary is
summary-only. Make combos task-aware (_DERIVER_TOKEN_COMBOS_BY_TASK) so no
always-0 impossible series is fabricated, matching the PR's own goal. Tests
tightened to assert the ingestion/previous_summary series is absent.

I2: the three gauge .set(0) zero-inits were bare while the counter inits go
through the fail-soft _touch. Add _set_gauge_zero() so a gauge init can't
propagate an exception into process startup either.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(telemetry): isolate zero-init namespaces, add deriver-to-api guard

Global-REGISTRY assertions used a fixed "test" namespace, which several
other suites also pin, so another test's materialized children could
satisfy a presence assertion or break an absence one. Each test now runs
under a unique namespace resolved from settings at read time.

Adds the inverse per-process isolation test: deriver-only init must not
materialize API-only series (dialectic tokens, embed_now).

Co-Authored-By: Claude <noreply@anthropic.com>

* review: per-replica backlog gauge, drop duplicated constants and .meta refs

Addresses Vineeth's review on #927.

Blocking:
- message_embeddings_pending is a DB-global count, so drive it from
  ReconcilerScheduler._scheduler_loop (runs on every replica, every
  interval) instead of run_vector_reconciliation_cycle (runs off the
  queue behind work-unit dedup, so one replica per cycle). Combined with
  the zero-init, the old placement made every replica that never won the
  work unit export a confident permanent 0. Help string now names the
  owner so dashboards don't reach for sum().
- guard initialize_telemetry_dropped_metrics on METRICS.ENABLED,
  matching its sibling initializer.
- drop the duplicate REASONING_LEVELS; import the one in src/config.

Non-blocking:
- walk BaseSpecialist recursively via a shared utils.types.walk_subclasses
  (replaces the direct-children-only __subclasses__() and the test's
  private copy of the same helper).
- derive the specialist assertion from the subclasses instead of
  hardcoding two names — the hardcoded pair kept passing after
  CardRefreshSpecialist landed, leaving it uncovered.
- inline the zero-init rationale and the multi-instance bucket taxonomy;
  removes both pointers to a .meta design doc that is not in the repo.

Tests: new tests/reconciler/test_pending_backlog_gauge.py pins both
halves of the relocation (verified it fails when reverted).

* review: fix inert test guard, stale comments, and the REASONING_LEVELS drift claim

Second review pass on the branch. Findings, most severe first:

- tests/reconciler/test_pending_backlog_gauge.py: the _try_enqueue_task stub
  was patched onto the class but declared without `self`, so calling it
  raised TypeError — which _scheduler_loop swallows. The guard was inert and
  the test passed for the wrong reason. Fixed the arity.

- metrics.py still commented that the backlog gauge is "set live each
  reconciliation cycle". That is the exact claim the previous commit
  overturned; it now contradicted the help string, the bucket-3 docstring
  and sync_vectors.py.

- metrics.py claimed REASONING_LEVELS is "derived from the config Literal so
  it never drifts", but config.py hand-listed it, so the earlier dedup had
  quietly traded away the guarantee the original get_args() call provided.
  Made it true instead: config.REASONING_LEVELS = list(get_args(...)), which
  keeps the dedup and restores the invariant.

- dropped _set_gauge_zero: all three gauges it zeroed already have identical
  fail-soft setters, so it was a second way to do one thing. Using the
  setters also makes _handle_metric_error name the actual gauge.

- record_pending_embeddings_backlog's docstring oversold the covering index
  as making the COUNT "negligible". The index makes cost proportional to the
  pending backlog, not to the table — which is worst precisely when the
  backlog matters. Stated honestly.

- _scheduler_loop's docstring said it only enqueues; it also refreshes the
  gauge, at a cadence set by the shortest task interval.

- comment reconciliation: stripped #927 / "the generalization" temporal
  anchoring, a CardRefreshSpecialist change-narration clause, and
  reviewer-directed phrasing from the test file; disambiguated the
  src/utils/summarizer.py path.

- CLAUDE.md had no Prometheus section at all, so the new "add a BaseEvent
  subclass -> update ALL_EVENT_TYPES" obligation and the never-sum() rule
  for non-additive gauges were undiscoverable from the architecture doc.

Verified: ruff + basedpyright clean (0 errors), tests/telemetry + reconciler
+ dialectic + llm 497 passed, full suite 1768 passed with only the 4
pre-existing test_document failures (OpenAI key required, reproduced on
clean origin/main). Re-confirmed the relocation guard fails when reverted.

* fix: silence the two basedpyright warnings inherited from main

CI runs `uv run basedpyright` bare, and basedpyright exits non-zero on any
warning — so these two have been failing the staticanalysis job on every
branch cut from current main, not just this one:

- src/vector_store/__init__.py:209 implicit string concatenation (#496)
- tests/test_cache_redaction.py:5 private import (#869)

Both predate this branch and are unrelated to the telemetry work; fixed
here only because they block this PR from going green. Verified: clean
origin/main also reports "0 errors, 2 warnings" and exits 1.

basedpyright now 0 errors, 0 warnings, exit 0.

* docs(telemetry): make the bucket-3 aggregation rule precise

The multi-instance taxonomy said a service-scoped non-additive metric has
"no aggregation correct once they disagree", then immediately mandated that
every instance refresh on its own timer. Those undercut each other: staggered
timers ALWAYS disagree slightly, so as written the rule reads as "ensure they
don't", which is unachievable, and it leaves the reader unsure whether max()
and avg() survived the fix.

The actual rule is bounded disagreement plus a scale-preserving aggregator.
Instances are N witnesses to one fact, not N parts of one whole, so sum() can
never be correct (it scales with replica count) while max()/avg()/quantiles
are correct precisely because the per-instance timer bounds the spread.

Wording only; no behavior change. The gauge help string already said
"max() or avg(), never sum()" — this makes the normative docstring agree
with it. Surfaced walking Vineeth's comment 3668208059 for comprehension.

* refactor(bench): import REASONING_LEVELS from config instead of re-listing

Third copy of the constant, missed when ee781c0/694e07f deduped the other
two. This one re-declared the ReasoningLevel Literal as well as the list,
so the type alias could diverge from config's with nothing to catch it —
and the list was hand-written, the variant that typechecks clean while
missing a member.

No import barrier justified it: this module already imports from src, as do
seven of its siblings in tests/bench. Concrete effect of the drift was that
a newly added sixth reasoning level would be rejected by the bench CLI's
argparse choices=.

src.config.REASONING_LEVELS is now the single definition repo-wide.

* test(telemetry): pin the METRICS.ENABLED guard on the per-emitter initializer

initialize_telemetry_dropped_metrics gained a METRICS.ENABLED guard in
ee781c0, addressing Vineeth's asymmetry comment, but nothing asserted it —
it had only the enabled half of the pair its sibling has. Deleting the guard
left the suite green, so the fix closed the asymmetry in the guards and
reproduced it one level up in the tests.

Mirrors test_init_noop_when_metrics_disabled. Verified live rather than
assumed: deleting the two guard lines turns this test red.

Uses a unique namespace, without which the absence assertion would be
satisfied by the enabled test's children rather than by the guard.

* docs(telemetry): fold zero-init why-prose behind # region ai markers

Comment/docstring-only pass over the changed files, per the groudon
comment-marker standard: the terse human-facing "what" stays visible, and
load-bearing "why" (the zero-init / absent-series-means-broken-scrape
rationale, gotchas, receipts) folds into # region ai / # ai: blocks.

Behavior-preserving: AST-identical modulo docstrings/comments vs the
pre-pass merge; ruff, ruff format --check, and basedpyright all clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-08-20 10:24:49 -04:00