Skip to content

feat(telemetry): wire metrics plugins to generation_batch_* hooks#1254

Merged
ajbozarth merged 2 commits into
generative-computing:mainfrom
ajbozarth:feat/metrics-batch-hooks
Jun 11, 2026
Merged

feat(telemetry): wire metrics plugins to generation_batch_* hooks#1254
ajbozarth merged 2 commits into
generative-computing:mainfrom
ajbozarth:feat/metrics-batch-hooks

Conversation

@ajbozarth

Copy link
Copy Markdown
Contributor

Issue

Fixes #1182

Description

PR #1181 introduced generation_batch_{pre_call,post_call,error} hooks for
the raw path; the tracing plugin subscribed but the metrics plugins did
not, so generate_from_raw calls produced zero metrics. This wires the
four affected metrics plugins to the corresponding batch hooks so that
token, latency, cost, and error metrics are recorded for raw-path
generations the same as for the chat path.

Source-side changes (mellea/telemetry/metrics_plugins.py):

  • TokenMetricsPlugin now also subscribes to generation_batch_post_call
    and reads payload.usage.
  • LatencyMetricsPlugin now also subscribes to generation_batch_post_call
    and records duration from payload.latency_ms. The batch path is
    non-streaming, so TTFB does not apply.
  • ErrorMetricsPlugin now also subscribes to generation_batch_error and
    classifies the exception from payload.exception.
  • CostMetricsPlugin now also subscribes to generation_batch_post_call
    and computes cost from payload.usage / payload.model /
    payload.provider.

The per-MOT handlers are unchanged. Class docstrings updated to reflect
the additional hook subscription.

Doc: removed the now-stale note in
docs/docs/observability/metrics.md claiming generate_from_raw does not
record token metrics.

Test-side changes:

  • test/telemetry/test_metrics_plugins.py: 13 new unit tests mirroring the
    existing per-MOT coverage for token, latency, cost, and error batch
    handlers (success, missing fields, fallback to "unknown", cache token
    forwarding for cost, unknown-model skip).
  • test/telemetry/test_metrics_backend.py: one e2e test that drives a real
    Ollama backend through generate_from_raw and asserts token and latency
    metrics are recorded.

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
$ uv run pytest test/telemetry/ -m "not qualitative and not e2e"
223 passed, 19 deselected
$ uv run pytest test/telemetry/test_metrics_backend.py::test_ollama_generate_from_raw_metrics_integration
1 passed

Attribution

  • AI coding assistants used

@ajbozarth
ajbozarth requested a review from a team as a code owner June 10, 2026 22:38
@github-actions github-actions Bot added the enhancement New feature or request label Jun 10, 2026
@ajbozarth ajbozarth self-assigned this Jun 10, 2026
Fixes generative-computing#1182. PR generative-computing#1181 introduced generation_batch_{pre_call,post_call,error}
hooks for the raw path; the tracing plugin subscribed but the metrics
plugins did not, so generate_from_raw calls produced zero metrics.

Token, latency, error, and cost metrics plugins now subscribe to the
matching generation_batch_* hook in addition to their existing per-MOT
hook. Per-MOT handlers are unchanged. The stale note in
docs/docs/observability/metrics.md is removed.

Assisted-by: Claude Code
Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
@ajbozarth
ajbozarth force-pushed the feat/metrics-batch-hooks branch from cdf973a to 1479a5b Compare June 10, 2026 22:44
@akihikokuroda

Copy link
Copy Markdown
Member

CostMetricsPlugin (lines 144–187)

Current implementation:
@hook("generation_post_call", mode=PluginMode.FIRE_AND_FORGET)
async def record_cost_metrics(self, payload: GenerationPostCallPayload, ...):
gen = payload.model_output.generation
if gen.usage is None:
return

model = gen.model or "unknown"
provider = gen.provider or "unknown"
details = gen.usage.get("prompt_tokens_details")
cached_tokens = (
    details.get("cached_tokens") if isinstance(details, dict) else 0
) or 0
cache_creation = gen.usage.get("cache_creation_input_tokens") or 0
prompt_tokens = gen.usage.get("prompt_tokens") or 0
cost = compute_cost(
    model=model,
    provider=gen.provider,  # ⚠️ BUG: should be `provider` not `gen.provider`
    prompt_tokens=prompt_tokens,
    completion_tokens=gen.usage.get("completion_tokens"),
    cached_tokens=cached_tokens,
    cache_creation_tokens=cache_creation,
)
if cost is not None:
    record_cost(cost=cost, model=model, provider=provider)

🐛 BUG IDENTIFIED (line 179):
cost = compute_cost(
model=model,
provider=gen.provider, # ❌ Should be provider (the fallback "unknown")
...
)

When gen.provider is None, this passes None to compute_cost, but:

  • Line 170: provider = gen.provider or "unknown" (correctly gets "unknown")
  • Line 179: provider=gen.provider (ignores the fallback!)
  • Line 186: record_cost(..., provider=provider) (records with "unknown")

This means compute_cost receives None while record_cost receives "unknown" — inconsistent, and compute_cost may fail if it expects a string.

Fix needed:
cost = compute_cost(
model=model,
provider=provider, # Use the fallback value
prompt_tokens=prompt_tokens,
...
)

PR's batch handler (lines 257–289) repeats this bug:
cost = compute_cost(
model=model,
provider=payload.provider, # ❌ Same issue — should use the fallback
...
)

@planetf1 planetf1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clean fix — wiring all four plugins symmetrically with the tracing plugin is exactly right, and the test coverage is thorough. Two small notes below.

Comment thread mellea/telemetry/metrics_plugins.py
@planetf1

Copy link
Copy Markdown
Contributor

The unit tests call the handlers directly, bypassing hook dispatch — the thing this PR is actually wiring. The generation_batch_error path has no end-to-end coverage. Could we add an e2e/ollama test driving generate_from_raw against a nonexistent model and asserting mellea.llm.errors == 1? Happy to track as a follow-up if easier.

Adds an integration test that drives generate_from_raw against a
nonexistent model and asserts the error counter is recorded, covering
the dispatch path through the generation_batch_error hook.

Also fixes the existing error test to honor OLLAMA_HOST instead of
hardcoding localhost:11434, matching the rest of the file.

Assisted-by: Claude Code
Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
@ajbozarth

Copy link
Copy Markdown
Contributor Author

Could we add an e2e/ollama test driving generate_from_raw

done in a03e99e

This means compute_cost receives None while record_cost receives "unknown" — inconsistent, and compute_cost may fail if it expects a string.

@akihikokuroda this is intentional, compute_cost accepts str | None and passes it through to litellm as custom_llm_provider=provider or None, where None triggers model-name-based provider inference. Sending "unknown" would break pricing lookup. The raw value goes to compute_cost (resolution); the labeled value goes to record_cost (attribution).

@akihikokuroda akihikokuroda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ajbozarth
ajbozarth enabled auto-merge June 11, 2026 18:46
@ajbozarth
ajbozarth added this pull request to the merge queue Jun 11, 2026
Merged via the queue into generative-computing:main with commit 7e23948 Jun 11, 2026
9 checks passed
@ajbozarth
ajbozarth deleted the feat/metrics-batch-hooks branch June 11, 2026 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: subscribe metrics plugins to generation_batch_* hooks

3 participants