feat: add MetricsContext and token tracking (Phase A)

[Aliciapet11]

Summary

• Add abevalflow/observability/ package with MetricsContext, TokenUsage, TimingRecord
• Add LLMResult dataclass and chat_completion_with_usage() to llm_client (backward compatible)
• Quality review now captures real token usage via chat_completion_with_usage()
• Add write_metrics_checkpoint.py script called from test phase finalize step
• store_results.py reads checkpoint and persists to observability_metrics table
• Add @timed_gate decorator for future gate timing instrumentation

Context

Phase A of APPENG-5370. Fills the observability_metrics table created in Phase 0 with real token data from LLM calls. After a pipeline run, the table shows actual prompt/completion token counts, model name, and LLM call count.

Validation

• All 911 tests pass locally
• CI ASE pipeline (ci-ase-9646p) passed on cluster in alicia-evalflow namespace
• observability_metrics table confirmed populated with real data: 947 prompt tokens, 608 completion tokens, 1 LLM call from quality review
• CI A2A, monitoring ASE, and monitoring A2A pipelines also passed
• Harbor skipped (known registry auth issue Guy is fixing separately)

Test plan

• Existing tests pass (911/911)
• New MetricsContext unit tests (14 tests)
• New LLM client token tests (3 tests)
• New store observability metrics tests (2 tests)
• CI ASE pipeline passes on cluster
• Real token data visible in observability_metrics table

[GuyZivRH]

Consolidated review — PR #54

feat: add MetricsContext and token tracking (Phase A)
#54

Verdict: Request changes (rebase required) → then approve

Design, tests, and Phase A scope are solid. CI green on the branch tip; cluster validation claimed (real tokens in observability_metrics). Not mergeable as-is — mergeable_state: dirty / conflicts with current main after AEH (#52).

Metric	Value
Commits	1 (clean, no AI trailers)
Files	11 (+583 / −10)
GitHub CI	✅ test passing
Local (reviewers)	ruff clean; large test suites green (counts vary by env/main tip — refresh after rebase)
Cluster	Author: ASE/A2A CI + monitoring passed; Harbor skipped (known registry issue)

---

Blocking

1. Rebase onto latest main
Hard conflict in scripts/test_quality_review.py:

• main: AEH quality-review path + _normalize_assessment()
• PR: chat_completion_with_usage() + token_usage on the assessment

pipeline/tasks/phases/test.yaml also changed on both sides — needs a careful rebase even if parts auto-merge.

Keep both: AEH review prompts/normalization and token capture for all engines (including AEH).

---

Major (fix on rebase; small diffs)

2. Metrics insert needs a savepoint
Scorecard uses session.begin_nested(); metrics does a bare session.add() inside a try/except. Unique-constraint failures surface at flush/commit, so that try/except does not protect against the real failure mode. A metrics conflict can roll back the whole store transaction (including work that already survived the scorecard savepoint).

Mirror the scorecard pattern:

if metrics_ctx:
    try:
        with session.begin_nested():
            session.add(ObservabilityMetricsRow(
                pipeline_run_id=effective_run_id,
                **metrics_ctx.to_observability_dict(),
            ))
    except Exception:
        logger.warning("Failed to persist observability metrics — continuing without", exc_info=True)

Low likelihood on happy-path single runs; still the right fix before this is live.

3. Empty checkpoints still create DB rows
write_metrics_checkpoint.py always writes; store inserts whenever the file exists — including skipped quality review / no token_usage (null tokens + null durations). Prefer write/persist only when there is real usage (or at least one timing), unless empty rows are intentional for “row per run.”

4. Idempotent store can skip metrics forever
Early return True when EvaluationRun already exists means a pre-Phase-A or partial run never backfills metrics. Pre-existing store pattern — document or upsert by (pipeline_run_id, attempt_number).

---

Minor (non-blocking)

Item	Note
to_observability_dict() x or None	0 → NULL; intentional for non-LLM runs, odd if a real 0 ever appears
@timed_gate	Unused scaffolding; OK for Phase A or drop until wired
Scope	Only quality-review tokens (other chat_completion callers unchanged) — fine; call out in PR body
MetricsContext.run_id	Written to checkpoint but DB uses effective_run_id from store — redundant, harmless
Checkpoint leftover	_metrics_checkpoint.json not deleted after store; workspace cleanup likely covers it
Finalize pip install pydantic	Consistent with other test steps; small runtime cost
PR body “911 tests”	Stale vs current main; refresh after rebase
Prefer usage.total_tokens when present	Avoid prompt+completion drift if providers add extra token classes

---

What looks good

• Clean abevalflow/observability/ package (MetricsContext, TokenUsage, TimingRecord) with checkpoint round-trip
• Additive LLMResult / chat_completion_with_usage(); chat_completion() stays a thin backward-compatible wrapper
• Clear path: quality review → _ai_review.json → checkpoint → ObservabilityMetricsRow
• Defensive layers: bad checkpoint JSON swallowed; Tekton finalize non-blocking (\|\| echo Warning…); missing checkpoint skips insert
• Field names align with Phase 0 schema / unique constraint
• Strong tests (context accumulation/checkpoint, LLM usage, store with/without checkpoint)
• Right Phase A proof: real prompt/completion tokens seen on cluster

---

Call

Do not merge until rebased with AEH quality-review behavior preserved and token capture kept.
On that rebase, land the begin_nested() metrics savepoint and preferably skip empty metrics rows. After that: approve and merge.

[Aliciapet11]

All review items addressed:

Blocking:

• Item 1 — Rebased onto latest main. Resolved conflict in test_quality_review.py: kept AEH review prompts + _normalize_assessment() AND token capture for all engines.

Major:

• Item 2 — Metrics insert now wrapped in session.begin_nested() savepoint, matching the scorecard pattern.
• Item 3 — Empty checkpoints no longer create DB rows. write_metrics_checkpoint.py skips checkpoint when total_tokens == 0. store_results.py also checks total_tokens > 0 before inserting.
• Item 4 — Added comment documenting that early return on duplicate run skips metrics, with pointer to backfill_scorecards.py.

Note: test_quality_review_aeh::test_aeh_review_uses_aeh_prompt_and_stays_passed fails on main too (APIConnectionError — test makes real API call). Not related to this PR.

[GuyZivRH]

PR #54 calls chat_completion_with_usage() (returns LLMResult), but the AEH quality-review test still patches chat_completion and returns a raw string. The mock never hits, so it tries a real LiteLLM URL.

In tests/test_quality_review_aeh.py, change to something like:

from abevalflow.llm_client import LLMResult

with patch(
    "scripts.test_quality_review.llm_client.chat_completion_with_usage",
    return_value=LLMResult(
        content=payload,
        prompt_tokens=1,
        completion_tokens=1,
        total_tokens=2,
        model="test",
    ),
):
    assessment = review_submission(sub)

That’s it for this CI failure. No product-code change required.

[Aliciapet11]

Good catch — fixed exactly as you suggested. Updated test_quality_review_aeh.py to mock chat_completion_with_usage with LLMResult instead of the old chat_completion. All 1026 tests passing now.