refactor(telemetry): align metrics/pricing init with tracing pattern

[ajbozarth]

Pull Request

Issue

Fixes #

Description

Brings mellea.telemetry.metrics and mellea.telemetry.pricing onto the same module-init pattern that mellea.telemetry.tracing adopted in #1181. Eliminates importlib.reload(mellea.telemetry.*) from every telemetry test (~40 sites) and consolidates the three drifted copies of _reset_tracing_state() that PR #1181 left behind.

No public API change. No production behavior change. Test side absorbed nearly all of the diff.

Why: after #1181, tracing.py was the only telemetry pillar with its init logic in a recallable _setup_tracing() function. metrics.py and pricing.py still captured env vars into module-level constants at the top of the module, which forced every test that toggled an env var to importlib.reload(mellea.telemetry.metrics) to re-execute the module body. Each reload re-runs imports and side effects — slow, noisy, and fragile.

Source-side changes:

• metrics.py — Module-level env-read constants removed. The seven private _METRICS_* / _OTLP_METRICS_ENDPOINT / _SERVICE_NAME / _EXPORT_INTERVAL_MILLIS constants no longer exist; env-var reads live inside _setup_meter_provider(), matching tracing.py. The hot-path gate in every record_* function checks _meter is None directly (semantically equivalent to the old _METRICS_ENABLED check). Also fixes a parallel of the tracing-side regression caught in fix: reload module for telemetry testing so all tests can run #805: _meter is now bound off the freshly built provider rather than via OTel's global metrics.get_meter(), since set_meter_provider() is one-shot per process and the global meter would otherwise fall back to the original (now shutdown) provider after a reset.
• pricing.py — Init logic wrapped in _setup_pricing(). Same shape as the others; no production behavior change.

Minor cleanup: the redundant if is_tracing_enabled(): guard at the module-load _setup_tracing() call site is dropped (the function early-returns on its own), and is_metrics_enabled() / is_tracing_enabled() now cache their result like is_pricing_enabled() already does.

Test-side changes:

• New test/telemetry/conftest.py with shared reset_metrics_state(), reset_tracing_state(), reset_logging_state(), reset_pricing_state() helpers replacing all importlib.reload calls. New regression test test_meter_remains_functional_after_repeated_resets exercises _setup_metrics() across multiple reset cycles and asserts recordings still land in the active reader.
• Tracing test reset DRY-up: three drifted per-file copies of _reset_tracing_state() consolidated. test_tracing.py and test_tracing_backend.py previously omitted the _tracer_provider.shutdown() call that test_tracing_plugins.py had — the consolidated helper does the shutdown for all three. Strictly safer cleanup; no perceptible slowdown (idle BatchSpanProcessor workers join in microseconds).
• Test mocking layer change for _LITELLM_AVAILABLE and _OTEL_AVAILABLE: tests no longer manipulate sys.modules to fake import failure. They now use monkeypatch.setattr(...) directly on the module attribute. Same code paths exercised; module-load-time try: import ... blocks are no longer reverified per-test.
• Test assertions in test_metrics.py shifted from cached env captures (_METRICS_CONSOLE is True, _OTLP_METRICS_ENDPOINT == "...", etc.) to behavior on the constructed MeterProvider (resource attributes, attached metric readers, mock-patched exporter call args). Strictly stronger — proves env values were both read and applied.
• test/telemetry/__init__.py added so from test.telemetry.conftest import ... resolves under mypy. Same pattern test/plugins/__init__.py already uses.

What didn't change:

• Public API: is_metrics_enabled(), is_tracing_enabled(), is_pricing_enabled(), get_otlp_log_handler(), all record_* and create_* functions — same signatures, same semantics.
• Production behavior: no env var renames, no breaking changes, no warning text changes.
• Documentation: no doc updates required.
• Plugin registration: _plugins_registered semantics preserved (process-global, intentionally not reset by tests).

Testing

• Tests added to the respective file if code was changed
• New code has 100% coverage if code was added
• Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

$ uv run pytest test/telemetry/ -m "not qualitative"
211 passed, 17 deselected

2 e2e tests in test_metrics_backend.py::test_huggingface_token_metrics_integration fail on local — pre-existing huggingface.py:1119 torch tensor-indexing issue, unrelated to this PR. Not seen in -m "not e2e".
Opened #1253

Attribution

• AI coding assistants used

---

Adding a new component, requirement, sampling strategy, or tool?

If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.

• Component
• Requirement
• Sampling Strategy
• Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

[akihikokuroda]

PR #1252 Code Review — Complete Findings

I've completed a comprehensive review of PR #1252 (telemetry refactor) examining all of metrics.py, pricing.py, and tracing.py. Here are all the issues found:

🔴 CONFIRMED BUG — Issue #1

_meter not reset when metrics disabled (Line 319-341 in metrics.py)

The PR changed hot-path gates from if not _METRICS_ENABLED: to if _meter is None:. This exposes a state sync bug:

When _setup_metrics() disables metrics, it sets _metrics_enabled = False but fails to reset _meter = None, violating the invariant: _meter is not None ⟺ _metrics_enabled is True

Why tests don't catch it: conftest.py pre-clears _meter = None (line 11) before calling _setup_metrics(), masking the bug.

Fix: Add one line at line 329:
_metrics_enabled = False
_meter = None # ← ADD THIS
if not (_OTEL_AVAILABLE and _env_true("MELLEA_METRICS_ENABLED")):

---

⚠️ CRITICAL DESIGN FRAGILITY — Issue #2

Hardcoded lazy globals list has no safeguard (conftest.py lines 14-25)

The reset function hardcodes 11 lazy instrument globals. If a new lazy global is added to metrics.py and forgotten in conftest:

• Local tests pass (same process, old state lingers)
• CI tests fail (fresh processes, state accumulates)

This is a silent test isolation failure that's hard to debug.

Fix: Auto-discover lazy instruments in conftest instead of hardcoding the list.

---

⚠️ HIGH DESIGN FRAGILITY — Issue #3

Plugin registration won't recover if framework unavailable initially (metrics.py lines 273-294)

If _HAS_PLUGIN_FRAMEWORK is False on first setup, _plugins_registered is never set to True. Combined with conftest intentionally not resetting this flag, plugins will never register even if the framework becomes available later.

Fix: Either (a) always set flag on first call, or (b) use tri-state, or (c) document the assumption.

---

📋 PRE-EXISTING CLEANUP (Not PR issues)

• Issue Incorporate example Colab notebooks #4: Counter getter duplication (6 identical functions, ~60 lines)
• Issue Add ReadMe Colab links and jupyter notebooks for Colab #5: No-op class duplication (3 nearly identical classes, ~24 lines)

Both are pre-existing code debt. Don't apply in this PR; file separate tech-debt issues.

---

✅ What Works Well

1. Correct meter binding strategy (fixes pre-existing fix: reload module for telemetry testing so all tests can run #805 bug)
2. Proper initialization sequencing (safe state transitions)
3. Plugin registry idempotency (prevents duplicates)
4. Env vars read at call time (respects changes after import)
5. Tracing DRY-up (3 drifted copies now unified)
6. Test isolation properly maintained

---

Approval Decision

⚠️ CONDITIONAL: Apply the single-line fix for Bug #1, then approve. The two design fragilities should be filed as separate post-merge tech-debt issues.

[akihikokuroda]

Please fix the issue 1. Thanks!

[akihikokuroda]

For reference:

Executive Summary

This PR successfully refactors mellea/telemetry/metrics.py and pricing.py to use callable _setup_*() functions instead of module-level constants. This eliminates ~40 importlib.reload() calls in tests. However, the PR introduces a state synchronization bug and reveals 2 design fragilities that should be addressed.

Issues Found

ID	Severity	Type	Title	Location
1	CONFIRMED	Bug	_meter not reset when metrics disabled	metrics.py:319-341
2	CRITICAL	Design Fragility	Hardcoded lazy globals list has no safeguard	conftest.py:14-25
3	HIGH	Design Fragility	Plugin registration won't recover if framework unavailable	metrics.py:273-294
4	Medium	Cleanup (pre-existing)	Counter getter duplication (6 functions)	metrics.py:817-963
5	Medium	Cleanup (pre-existing)	No-op class duplication (3 classes)	metrics.py:348-372

---

ISSUE #1: CONFIRMED BUG — _meter Not Reset When Metrics Disabled

Severity: CONFIRMED BUG (medium impact, masked by test infrastructure)

Location: mellea/telemetry/metrics.py lines 319-341

The Problem

The PR changed hot-path gates from if not _METRICS_ENABLED: to if _meter is None:. This exposes a state synchronization bug:

When _setup_metrics() is called with metrics disabled, it sets _metrics_enabled = False but fails to reset _meter = None.

def _setup_metrics() -> None:
    global _meter_provider, _meter, _metrics_enabled
    
    _metrics_enabled = False
    # ❌ BUG: _meter is not reset here!
    if not (_OTEL_AVAILABLE and _env_true("MELLEA_METRICS_ENABLED")):
        return  # Early exit with _meter still pointing to stale provider
    
    # ... rest of setup ...
    _meter = _meter_provider.get_meter(...)
    _metrics_enabled = True

Invariant Violation

This breaks the invariant: _meter is not None ⟺ _metrics_enabled is True

Failure Scenario

Test 1: MELLEA_METRICS_ENABLED=true
  → _setup_metrics() runs fully
  → _meter = <MeterProvider>.get_meter() ✓
  → _metrics_enabled = True ✓

Test 2: reset_metrics_state() with MELLEA_METRICS_ENABLED=false
  → conftest line 11: _meter = None (defensive pre-clear)
  → conftest line 27: _setup_metrics() called
  → metrics.py line 328: _metrics_enabled = False
  → metrics.py line 329-330: Early return
  → ⚠️  BUG: _meter should be None but isn't!

Later: record_token_usage_metrics()
  → Line 519: if _meter is None: ← FALSE (stale reference!)
  → Attempts to record on shutdown provider ← UNDEFINED BEHAVIOR

Why Current Tests Don't Catch It

conftest.py explicitly pre-clears _meter = None (line 11) before calling _setup_metrics().

This defensive programming masks the bug. The bug only manifests if:

• Code calls _setup_metrics() directly without pre-clearing _meter
• Concurrent code paths reset state

The Fix

Add ONE line to guarantee the invariant:

def _setup_metrics() -> None:
    global _meter_provider, _meter, _metrics_enabled
    
    _metrics_enabled = False
    _meter = None  # ← ADD THIS LINE
    if not (_OTEL_AVAILABLE and _env_true("MELLEA_METRICS_ENABLED")):
        return

Verification

• 211 telemetry tests currently pass (masked by conftest defensive clear)
• With fix applied: same 211 tests pass + invariant guaranteed

---

ISSUE #2: CRITICAL DESIGN FRAGILITY — Hardcoded Lazy Globals List

Severity: CRITICAL (future maintainability risk)

Location: test/telemetry/conftest.py lines 14-25

The Problem

The reset_metrics_state() function hardcodes 11 lazy instrument globals to clear:

def reset_metrics_state() -> None:
    # ... cleanup code ...
    metrics._input_token_counter = None
    metrics._output_token_counter = None
    metrics._duration_histogram = None
    metrics._ttfb_histogram = None
    metrics._error_counter = None
    metrics._cost_counter = None
    metrics._sampling_attempts_counter = None
    metrics._sampling_successes_counter = None
    metrics._sampling_failures_counter = None
    metrics._requirement_checks_counter = None
    metrics._requirement_failures_counter = None
    metrics._tool_calls_counter = None
    # If a new lazy global is added, it must be manually added here too!

The Risk

If a developer adds a new lazy instrument to metrics.py:

_new_feature_counter: Any = None  # ← forgotten in conftest

The test isolation breaks:

• Local testing: Passes (same process, old state still bound)
• CI testing: Fails (fresh processes per test, old state accumulates)

This is a silent test isolation failure that's extremely hard to debug.

Current Lazy Globals (Must All Be Reset)

1. _input_token_counter
2. _output_token_counter
3. _duration_histogram
4. _ttfb_histogram
5. _error_counter
6. _cost_counter
7. _sampling_attempts_counter
8. _sampling_successes_counter
9. _sampling_failures_counter
10. _requirement_checks_counter
11. _requirement_failures_counter
12. _tool_calls_counter

Recommended Fix

Add validation in conftest that introspects metrics module:

def reset_metrics_state() -> None:
    from mellea.telemetry import metrics
    
    # Shutdown provider
    if metrics._meter_provider is not None:
        metrics._meter_provider.shutdown()
    
    # Reset core state
    metrics._meter_provider = None
    metrics._meter = None
    
    # Reset ALL lazy instruments (auto-discovered, not hardcoded)
    for attr_name in dir(metrics):
        if (attr_name.startswith('_') and 
            attr_name not in {'_OTEL_AVAILABLE', '_plugins_registered'} and
            getattr(metrics, attr_name) is None or isinstance(getattr(metrics, attr_name), (type(None), Any))):
            # This is a lazy instrument, reset it
            setattr(metrics, attr_name, None)
    
    metrics._setup_metrics()

Or, maintain a _LAZY_GLOBALS tuple in metrics.py that conftest reads from.

---

ISSUE #3: HIGH DESIGN FRAGILITY — Plugin Registration Won't Recover

Severity: HIGH (edge case, but design fragility)

Location: mellea/telemetry/metrics.py lines 273-294

The Problem

def _register_metrics_plugins() -> None:
    global _plugins_registered
    if _plugins_registered:
        return
    
    from mellea.plugins.registry import _HAS_PLUGIN_FRAMEWORK, register
    
    if not _HAS_PLUGIN_FRAMEWORK:
        warnings.warn(...)
        return  # ❌ BUG: Returns WITHOUT setting _plugins_registered = True
    
    # ... register plugins ...
    _plugins_registered = True

The Issue

If _HAS_PLUGIN_FRAMEWORK is False on first setup, the function returns without setting the flag. Combined with conftest's intentional non-reset of _plugins_registered, this means:

Plugins will NEVER register if the framework becomes available later in the same process.

Scenario

1. Module import: plugin framework not installed
   → _register_metrics_plugins() called
   → _HAS_PLUGIN_FRAMEWORK = False
   → Returns early WITHOUT setting _plugins_registered = True
   → _plugins_registered still False

2. Later in test suite: plugin framework imported/mocked as available
   → _setup_metrics() called again
   → _register_metrics_plugins() called
   → _plugins_registered is still False
   → But: if we tried to register, it would now work...
   → Instead: silently skipped because flag is False

Recommended Fix

Option A: Always mark decision as made:

_plugins_registered = True  # Mark as attempted, even if skipped

Option B: Use tri-state (never tried, tried and failed, tried and succeeded):

_plugins_registration_attempt_made = False  # Renamed

Option C: Document assumption explicitly with test:

def test_plugin_registration_assumption():
    """Verify that if plugin framework becomes available mid-process, 
    plugins still register on next _setup_metrics() call."""
    # This test documents the assumption that this edge case 
    # is not supported (plugin framework must be available at import).

---

ISSUE #4: PRE-EXISTING CLEANUP — Counter Getter Duplication

Severity: MEDIUM (pre-existing code debt, not introduced by PR)

Location: mellea/telemetry/metrics.py lines 472-963

The Issue

Six lazy-init counter functions follow identical global / if is None / create / return pattern:

• _get_token_counters() (lines 472–494)
• _get_error_counter() (lines 707–722)
• _get_cost_counter() (lines 768–783)
• _get_sampling_attempts_counter() (lines 817–827)
• _get_requirement_checks_counter() (lines 892–902)
• _get_tool_calls_counter() (lines 952–962)

Impact

~60 lines of boilerplate; future counter additions require copy-paste.

Recommendation

Don't apply in this PR. File separate tech-debt issue for post-merge cleanup.

---

ISSUE #5: PRE-EXISTING CLEANUP — No-Op Class Duplication

Severity: MEDIUM (pre-existing code debt, not introduced by PR)

Location: mellea/telemetry/metrics.py lines 348-372

The Issue

class _NoOpCounter:
    def add(self, amount, attributes=None): pass

class _NoOpHistogram:
    def record(self, amount, attributes=None): pass

class _NoOpUpDownCounter:
    def add(self, amount, attributes=None): pass

Three no-op classes differ only in method names. Could consolidate to ~8 lines with factory.

Recommendation

Don't apply in this PR. File separate tech-debt issue for post-merge cleanup.

---

[ajbozarth]

Thanks for the thorough review. Walking through each item:

Issue #1 — _meter not reset on disabled path: Fair catch. Reachable in practice only through code paths that already pre-clear _meter, but the invariant should hold from _setup_*() alone. Pushed a fix that nulls _meter/_meter_provider in _setup_metrics() and the three tracer fields in _setup_tracing() on the disabled-path early return — symmetric across both modules.

Issue #2 — Hardcoded lazy globals list in conftest: That fragility has to live somewhere — auto-discovery via dir() introspection trades an explicit list for a filter that has to correctly identify "lazy instrument" vs "everything else underscore-prefixed," and would silently miss any new global that doesn't match the heuristic. The current explicit list is the clearest place to enforce the "update conftest when you add a global" discipline. Declining.

Issue #3 — Plugin registration won't recover if framework unavailable: I think this analysis is inverted. _HAS_PLUGIN_FRAMEWORK is set by a try: import at module load in plugins/registry.py and is not runtime-mutable — it stays False for the process lifetime once False. The "later becomes available" scenario only arises through test mocking, which is exactly when we'd want re-registration to be re-attempted. The current behavior — not setting _plugins_registered = True on the no-framework branch — is what allows that retry. Setting the flag (Option A) would lock out recovery, which is the opposite of the stated goal. Same pattern lives in tracing.py:_register_tracing_plugins and is intentional there too. No code change.

Issues #4 & #5 — Counter/no-op duplication: These aren't issues — they're the right shape. Each lazy-init getter reads top-to-bottom in five seconds (name, description, unit, return); a registry/decorator would replace that with indirection that's harder to grep and harder to type-check. The three no-op classes are two lines each and match three distinct OTel protocols (Counter.add, Histogram.record, UpDownCounter.add) — collapsing via a factory means faking multiple protocols on one shim. Mechanical repetition that's obvious to read beats compressed code that requires holding the abstraction in your head. Not filing as follow-ups.

[akihikokuroda]

LGTM

[planetf1]

Nice work — clean approach and the _meter_provider.get_meter() fix is the right call. A few small notes below; none are blockers.

[planetf1]

This (and test_metrics_disabled_with_falsy_values just below) leaves the module enabled on exit — monkeypatch restores the env vars but won't re-run reset_metrics_state(), so the built provider lingers until the next test's fixture rebuilds it. Works today, but it's an order-dependency waiting to bite. Either a final reset after the loop:

Suggested change

	from mellea.telemetry.metrics import is_metrics_enabled
	assert is_metrics_enabled(), f"Failed for value: {value}"
	reset_metrics_state()

or adding clean_metrics_env to the signature so teardown handles it. Not a blocker.

[planetf1]

Sorry, comment off by a line

[planetf1]

This nulls each lazy instrument global by hand, and right now it's an exact match for the twelve in metrics.py — nice. The only fragility is drift: a new _*_counter getter added later won't get cleared here, and it'd leak a stale instrument across resets without any test failing loudly. Might be worth a short note on the getter section in metrics.py (or here) cross-referencing this reset, so the next person adding an instrument knows to update both spots. Not blocking.

[planetf1]

Could spell out the consequence here — the flag guards an idempotent register, and clearing it would make the next _setup_metrics() re-register already-registered plugins, which surfaces as spurious ValueError/UserWarning noise:

Suggested change

	# _plugins_registered intentionally NOT reset — registry is process-global.
	# _plugins_registered intentionally NOT reset: the registry is process-global,
	# and re-registering already-registered plugins raises ValueError (surfaced as
	# a UserWarning). The guard stays set so re-setup is a no-op for plugins.