Skip to content

feat: LLM observability - per-request token, cost, and latency telemetry with tracing (Closes #61) - #69

Merged
zeemscript merged 5 commits into
Deen-Bridge:devfrom
mzterwalexzyy:feat/llm-observability
Jul 25, 2026
Merged

feat: LLM observability - per-request token, cost, and latency telemetry with tracing (Closes #61)#69
zeemscript merged 5 commits into
Deen-Bridge:devfrom
mzterwalexzyy:feat/llm-observability

Conversation

@mzterwalexzyy

@mzterwalexzyy mzterwalexzyy commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds per-request LLM observability: token, cost, and latency telemetry with lightweight tracing, plus a GET /metrics surface. Closes #61.

The whole thing is standard library only (no new dependency in requirements.txt), which keeps the footprint appropriate for the small Render service.

What changed

New telemetry.py:

  • Configurable per-model price table (override with the LLM_PRICE_TABLE env var). Unknown models surface as priced: false / cost 0.0 rather than a silently wrong number.
  • capture_usage() reads the Gemini SDK's usage_metadata defensively: a missing or malformed field degrades to zeroed counts instead of raising, so telemetry never breaks the request it is measuring.
  • Trace with per-stage spans (context manager plus a manual add_span).
  • Thread-safe in-memory MetricsRegistry: request/error counts, token totals, cost totals, p50/p95 latency (handler and model), and a per-model breakdown. Latency samples are capped so memory stays bounded on a long-lived process.

Wired into main.py:

  • One uuid4 trace id per /chat request.
  • Both model calls are recorded: the safety classifier (generate_content) and the main generation (send_message), each capturing input/output/total tokens, model name, estimated cost, and model-call latency.
  • End-to-end handler latency is timed.
  • Four stage spans so a slow request is attributable: classification, retrieval, generation, post_processing (the last covers the scholar-review enqueue, which does I/O).
  • Per-request totals are returned on X-Trace-Id / X-LLM-Total-Tokens / X-LLM-Cost-USD / X-Handler-Latency-Ms response headers, mirroring the existing X-Semantic-Cache convention. The trace id is also attached to error responses.
  • New GET /metrics returning the aggregate. Cache hit-rate is sourced from the semantic cache's own counters ([Enhancement] Semantic response cache with embedding-similarity matching for repeated questions #27) rather than re-derived, so it stays consistent with /cache/stats.

CI (.github/workflows/ci.yml): telemetry.py and the new test are added to the flake8 and compileall steps, and a pytest step runs the telemetry tests.

Redaction

Redaction is a design invariant, not a filter. Every telemetry function takes only counts, durations, costs, model names, and trace ids, so prompt or answer text cannot reach a metric label, a span, or a log line from this module. Three dedicated tests assert that a secret-looking prompt never appears in any snapshot, label, or span.

Relationship to adjacent issues (integrates with, does not duplicate)

Design note for the maintainer

The issue mentions exposing token/cost as optional response metadata. I put it on response headers rather than in the JSON body, because it is non-breaking (the ChatResponse schema is unchanged) and middleware for #9/#13 can read headers just as easily. Happy to move it into the response body instead if you would prefer that consumption pattern.

How I tested

  • pytest tests/test_telemetry.py: 17 passed, fully offline (a fake Gemini response carrying a known usage_metadata, no live API calls). Covers cost-table math, missing/partial usage metadata, latency percentiles, span emission, add_span, and the three no-content-leak assertions.
  • flake8 on telemetry.py main.py tests/test_telemetry.py with the repo's CI config (--max-line-length=120 --ignore=E501,W503): clean.
  • compileall on the changed modules: ok.

I did not run the full existing suite locally because it needs the whole dependency set plus a GEMINI_API_KEY to import main; CI covers that. The main.py changes are additive and pass compileall.

Screenshot of the passing telemetry tests and clean lint is attached below.

Screenshot / Recording

image

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Added stdlib-only telemetry with per-request tracing, latency tracking, and token-based cost estimation.
    • Instrumented chat flows (classification, retrieval, generation) and streaming with trace IDs and telemetry spans.
    • Added GET /metrics to return a telemetry registry snapshot plus semantic cache stats.
  • Bug Fixes
    • Improved error observability by mapping upstream failures to clearer HTTP statuses and returning X-Trace-Id.
  • Tests
    • Added offline unit tests validating usage parsing, cost math, metrics aggregation, tracing, and no-content-leak guarantees.
  • Chores
    • Expanded CI linting/syntax checks and added telemetry-specific pytest coverage.

Adds telemetry.py (standard library only, no new dependency): a configurable
per-model price table, usage capture from the Gemini SDK's usage_metadata,
lightweight per-stage spans, and a thread-safe in-memory MetricsRegistry.

Wires it into /chat: a uuid4 trace id per request, both model calls (safety
classifier and generation) record input/output/total tokens, model name,
estimated cost, and model latency; end-to-end handler latency is timed;
retrieval and generation stages emit spans; per-request totals are returned
on X-Trace-Id / X-LLM-* response headers. New GET /metrics exposes request
count, token and cost totals, p50/p95 latency, error rate, and cache hit-rate
(sourced from the existing semantic cache counters, not re-derived).

Telemetry only ever handles counts, durations, costs, model names, and trace
ids, so no prompt or answer content can leak into a metric label; a test
asserts this. Reuses rather than duplicates adjacent work: reads the SDK's
usage_metadata instead of re-counting tokens (Deen-Bridge#13), exposes cost/token data
for future budget/rate-limit logic without enforcing it (Deen-Bridge#9), and notes that
Deen-Bridge#11 should eventually own the trace-id scheme.

Adds offline tests/test_telemetry.py (fake Gemini response, no live calls)
covering capture, cost math, spans, percentiles, and the no-leak property,
and adds a pytest step plus flake8/compileall coverage for the new modules
to CI.
…ns, trace id on errors

- Remove the unused cache_hits/cache_misses/hit_rate block from
  MetricsRegistry and /metrics: cache hit-rate is served from the semantic
  cache's own counters (snapshot["semantic_cache"]), so the registry's block
  only ever emitted misleading zeros. Add a test asserting it is gone.
- Widen span coverage from two stages to four: add "classification" around
  fiqh/intent classification and "post_processing" around the tail (history,
  hadith grading, confidence, and the scholar-review enqueue, which does I/O),
  so a slow request is attributable to that stage. Add Trace.add_span for
  manual timing of blocks awkward to wrap in a context manager.
- Carry X-Trace-Id on error responses via HTTPException(headers=...); setting
  it on the injected Response does not propagate through a raised exception,
  so a failed request previously had no trace id to correlate with its logs.
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds dependency-free Gemini telemetry for token usage, cost, latency, tracing, and aggregate metrics. Instruments /chat, exposes /metrics, adds trace metadata and error tracking, and validates telemetry behavior and content redaction in CI.

Changes

LLM observability

Layer / File(s) Summary
Telemetry primitives and aggregation
telemetry.py
Adds configurable cost estimation, defensive usage capture, request spans, trace correlation, model-call recording, thread-safe aggregation, and latency percentiles.
Chat tracing and metrics endpoint
main.py
Records classification and generation calls, traces retrieval and post-processing, finalizes success and cache-hit responses with telemetry headers, tracks errors with trace IDs, and exposes /metrics.
Offline validation and CI checks
tests/test_telemetry.py, .github/workflows/ci.yml
Tests pricing, usage fallbacks, aggregation, spans, reset behavior, and content redaction; CI lints, compiles, and runs the telemetry suite.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant ChatHandler
  participant Trace
  participant Gemini
  participant MetricsRegistry
  Client->>ChatHandler: /chat request
  ChatHandler->>Trace: create trace and stage spans
  ChatHandler->>Gemini: classification and generation calls
  Gemini-->>ChatHandler: usage metadata and response
  ChatHandler->>MetricsRegistry: record request totals or error
  ChatHandler-->>Client: response with trace headers
  Client->>ChatHandler: /metrics request
  ChatHandler->>MetricsRegistry: snapshot metrics
  MetricsRegistry-->>Client: telemetry metrics
Loading

Possibly related PRs

  • Deen-Bridge/dnb-ai#48: Adds the fiqh and madhhab classification flow that this PR instruments with telemetry spans and model-call recording.

Suggested reviewers: zeemscript, fury03, david-282

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: LLM observability with token, cost, latency telemetry and tracing.
Linked Issues check ✅ Passed The PR implements per-call token/cost/latency telemetry, request tracing, a metrics endpoint, and offline tests, matching #61.
Out of Scope Changes check ✅ Passed The CI and error-handling updates support the telemetry work and do not appear unrelated to #61.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
main.py (1)

486-502: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

_finalize() runs before the response object is fully built — possible double-count on a late failure.

_finalize() (line 488) records the request as successful, but the ChatResponse(...) call below it (including the nested Moderation(...) construction) is evaluated after that. If building that response object ever raises (e.g. a Pydantic validation failure), the except block at line 504-507 will also call record_request(..., error=True) for the same request — double-counting it in both request_count/error_count and the handler-latency samples.

Low probability given the fields are already validated upstream, but the fix is essentially free: construct the response before finalizing.

🩹 Proposed fix
         trace.add_span("post_processing", (time.perf_counter() - _pp_start) * 1000.0)
         logger.info("Chat response generated successfully")
-        _finalize()
-        return ChatResponse(
+        response_obj = ChatResponse(
             response=response_text,
             chat_id=chat_id,
             history=history,
             moderation=Moderation(
                 category_id=safety_result.category_id,
                 action=safety_result.action,
             ) if safety_result and safety_result.category_id else None,
             fiqh=fiqh_info,
             hadith_references=hadith_refs,
             tafsir=tafsir_info,
             confidence=assessment,
             zakat=zakat_info,
         )
+        _finalize()
+        return response_obj
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 486 - 502, Construct the complete ChatResponse,
including its nested Moderation value, before calling _finalize(). Keep
_finalize() immediately before the successful return, and return the prebuilt
response so any construction or validation failure is handled only by the
existing error path.
🧹 Nitpick comments (3)
main.py (2)

504-518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider chaining the re-raised HTTPException with from e.

Static analysis flags the raise HTTPException(...) here as missing exception chaining (flake8-bugbear B904). It's not enforced by this repo's CI flake8 invocation (no bugbear plugin installed per the workflow), so it won't break the build, but chaining preserves the original traceback for anyone inspecting logs/APM later — a natural complement to the trace-id correlation this PR is adding.

♻️ Optional tweak
         raise HTTPException(
             status_code=500,
             detail=error_msg,
             headers={"X-Trace-Id": trace.trace_id},
-        )
+        ) from e
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 504 - 518, Update the HTTPException re-raise in the
exception handler to explicitly chain it from the caught exception e, while
preserving the existing status, detail, headers, telemetry, and cleanup
behavior.

Source: Linters/SAST tools


540-555: 🧹 Nitpick | 🔵 Trivial

Consider access control on /metrics before production exposure.

The endpoint returns aggregate cost/token/latency data and cache stats. The code already acknowledges this is deferred to issue #9 (auth/rate limiting) per the inline comment, so this is just a heads-up rather than a new gap introduced here — worth keeping on the radar so it isn't publicly reachable without auth once merged.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 540 - 555, Before exposing the metrics endpoint in
production, apply the project’s established authentication and access-control
mechanism to the metrics handler, while preserving its existing snapshot and
semantic_cache statistics response. Track the deferred `#9` requirement so
/metrics cannot be publicly reached without authorization.
telemetry.py (1)

46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Model-name literal is duplicated between this pricing table and main.py.

The pricing table keys must exactly match the hardcoded "gemini-2.5-flash-preview-05-20" strings sprinkled through main.py (classifier, cache-hit rehydration, and generate()). A future model rename in one place but not the other silently degrades to priced=False/$0 rather than erroring — already mitigated by the priced flag, but a shared constant would remove the drift risk entirely.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@telemetry.py` around lines 46 - 52, Define a shared model-name constant and
use it for the `"gemini-2.5-flash-preview-05-20"` key in `_DEFAULT_PRICE_TABLE`
and every corresponding model reference in `main.py`, including classifier,
cache-hit rehydration, and `generate()` paths. Remove the duplicated literals
while preserving the existing pricing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@telemetry.py`:
- Around line 329-332: Update Telemetry.reset to preserve the existing
self._lock: while holding it, reset the instance state without calling
self.__init__, and reinitialize only the fields needed for reset such as the
sample storage and related counters while retaining configuration like
max_samples.

---

Outside diff comments:
In `@main.py`:
- Around line 486-502: Construct the complete ChatResponse, including its nested
Moderation value, before calling _finalize(). Keep _finalize() immediately
before the successful return, and return the prebuilt response so any
construction or validation failure is handled only by the existing error path.

---

Nitpick comments:
In `@main.py`:
- Around line 504-518: Update the HTTPException re-raise in the exception
handler to explicitly chain it from the caught exception e, while preserving the
existing status, detail, headers, telemetry, and cleanup behavior.
- Around line 540-555: Before exposing the metrics endpoint in production, apply
the project’s established authentication and access-control mechanism to the
metrics handler, while preserving its existing snapshot and semantic_cache
statistics response. Track the deferred `#9` requirement so /metrics cannot be
publicly reached without authorization.

In `@telemetry.py`:
- Around line 46-52: Define a shared model-name constant and use it for the
`"gemini-2.5-flash-preview-05-20"` key in `_DEFAULT_PRICE_TABLE` and every
corresponding model reference in `main.py`, including classifier, cache-hit
rehydration, and `generate()` paths. Remove the duplicated literals while
preserving the existing pricing behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a316c677-2f51-4fd9-a063-f72659d49276

📥 Commits

Reviewing files that changed from the base of the PR and between aba4995 and ca59d82.

📒 Files selected for processing (4)
  • .github/workflows/ci.yml
  • main.py
  • telemetry.py
  • tests/test_telemetry.py

Comment thread telemetry.py
- MetricsRegistry.reset() now resets fields in place under the existing lock
  instead of calling __init__, which would swap self._lock for a new object
  while another thread could still be blocked on the old one (race window).
  Add a test asserting state is cleared and the lock instance is preserved.
- Build the ChatResponse before calling _finalize() on the success path, so a
  response construction/validation failure is handled only by the error path
  and the request is not double-counted as both success and error.
- Chain the re-raised HTTPException with `from e` to preserve the original
  traceback (complements the trace-id correlation this PR adds).
- Introduce telemetry.GEMINI_MODEL and use it for the price-table key and every
  model reference in main.py, so the pricing table and call sites cannot drift
  apart and silently unprice the model on a rename.

Skipped (documented, not a change for this PR): access control on /metrics is
deferred to Deen-Bridge#9 per the existing inline comment.
@zeemscript

Copy link
Copy Markdown
Contributor

@mzterwalexzyy please fix conflicts, thank you

Resolves the conflicts the maintainer asked about on Deen-Bridge#69. Upstream's
/chat/stream commit renamed the Gemini model from
"gemini-2.5-flash-preview-05-20" to "gemini-2.5-flash" at the three
GenerativeModel call sites this branch had already replaced with the
shared telemetry.GEMINI_MODEL constant.

Resolution keeps the constant and moves it to upstream's name, so the
rename lands once instead of at each call site:

- telemetry.GEMINI_MODEL is now "gemini-2.5-flash".
- The superseded preview alias stays in the default price table, so
  telemetry replayed from before the rename still prices instead of
  reporting priced=False / $0.
- The new /chat/stream session now builds from telemetry.GEMINI_MODEL
  too, rather than hardcoding the name a fourth time.

Full suite passes (512), telemetry tests pass (18), flake8 clean.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (5)
main.py (5)

359-367: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Offload Gemini calls from async handlers. In both /chat and /chat/stream, send_message(...) runs synchronously inside async request handling, and the stream endpoint iterates the blocking response stream inline. That can pin the event loop for model latency and stall unrelated requests. Use the Gemini async API or move the call and chunk iteration to a worker thread while preserving SSE delivery.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 359 - 367, Update the `/chat` and `/chat/stream`
handlers so synchronous Gemini `send_message` calls and streaming chunk
iteration do not run on the event loop; use the Gemini async API or offload both
operations to a worker thread, while preserving the existing response behavior
and SSE delivery.

Source: Path instructions


526-527: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Instrument /chat/stream end to end.

Unlike /chat, this endpoint never creates a trace, records model usage, finalizes request latency, or emits trace/token/cost metadata. Streaming traffic will therefore be missing from /metrics, and streaming errors have no trace ID, contradicting the PR objective that both model paths be observable.

Also applies to: 557-590, 616-619, 621-629

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 526 - 527, Instrument the chat_stream endpoint end to
end by creating and propagating a trace, recording model usage, finalizing
request latency, and emitting trace, token, and cost metadata across both
successful and error streaming paths. Reuse the observability flow and
established helpers used by the chat endpoint, ensuring streaming errors include
the trace ID and all completed requests are represented in metrics.

616-619: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Return generic errors to clients.

Both error paths expose raw exception text. Provider errors may contain internal URLs, request data, or other sensitive implementation details. Log the exception server-side and return a stable generic message, associating it with the trace ID once streaming telemetry is added.

As per path instructions, FastAPI handlers must not leak internal exception details to clients.

Also applies to: 631-634

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 616 - 619, Update the streaming exception handlers
around the logger.error calls and error_data construction to keep raw exception
details server-side while returning a stable generic error message to clients.
Apply this to both error paths, and associate the server-side log and client
response with the available trace ID when streaming telemetry is present; do not
expose str(e) in the yielded response.

Sources: Path instructions, Linters/SAST tools


531-533: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Keep prompt content out of logs.

prompt[:100] still writes user content, including possible PII, to logs. Secret-key redaction does not make arbitrary prompt text safe to retain. Log only content-free metadata such as the chat ID, trace ID, and prompt length.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 531 - 533, Update the streaming chat request logging
near redact_secret_keys so logger.info no longer includes prompt content or any
substring of it. Log only content-free metadata, such as the chat ID, trace ID,
and prompt length, while preserving the existing redaction and request handling.

540-545: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Pass redacted inputs to every retriever.

zakat_retriever(request.prompt, request.context) bypasses the redaction applied immediately above. A user-supplied secret can therefore reach downstream retrieval or logging code. Use the sanitized variables consistently:

Proposed fix
-        zakat_context = await zakat_retriever(request.prompt, request.context)
+        zakat_context = await zakat_retriever(prompt, extra_context)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 540 - 545, Update the retriever calls in the
surrounding request-handling flow so zakat_retriever receives the same redacted
prompt and context variables used by tafsir_retriever, rather than
request.prompt and request.context. Preserve the existing language fallback and
retrieval behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@main.py`:
- Around line 359-367: Update the `/chat` and `/chat/stream` handlers so
synchronous Gemini `send_message` calls and streaming chunk iteration do not run
on the event loop; use the Gemini async API or offload both operations to a
worker thread, while preserving the existing response behavior and SSE delivery.
- Around line 526-527: Instrument the chat_stream endpoint end to end by
creating and propagating a trace, recording model usage, finalizing request
latency, and emitting trace, token, and cost metadata across both successful and
error streaming paths. Reuse the observability flow and established helpers used
by the chat endpoint, ensuring streaming errors include the trace ID and all
completed requests are represented in metrics.
- Around line 616-619: Update the streaming exception handlers around the
logger.error calls and error_data construction to keep raw exception details
server-side while returning a stable generic error message to clients. Apply
this to both error paths, and associate the server-side log and client response
with the available trace ID when streaming telemetry is present; do not expose
str(e) in the yielded response.
- Around line 531-533: Update the streaming chat request logging near
redact_secret_keys so logger.info no longer includes prompt content or any
substring of it. Log only content-free metadata, such as the chat ID, trace ID,
and prompt length, while preserving the existing redaction and request handling.
- Around line 540-545: Update the retriever calls in the surrounding
request-handling flow so zakat_retriever receives the same redacted prompt and
context variables used by tafsir_retriever, rather than request.prompt and
request.context. Preserve the existing language fallback and retrieval behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d41c47af-f37a-4add-86af-b4e952df4998

📥 Commits

Reviewing files that changed from the base of the PR and between ce61a9d and b1c8265.

📒 Files selected for processing (2)
  • main.py
  • telemetry.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • telemetry.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
main.py (3)

439-459: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Finalize cache hits after response validation.

_finalize() records success before annotate_hadith() and ChatResponse(...) run. If either raises, finally records the same request as an error too, violating the intended exactly-once accounting.

Proposed fix
-                _finalize()
-                _succeeded = True
-                return ChatResponse(
+                response_obj = ChatResponse(
                     response=cached.response,
                     chat_id=chat_id,
                     history=cached.history,
                     fiqh=fiqh_info,
                     hadith_references=annotate_hadith(cached.response),
                 )
+                _finalize()
+                _succeeded = True
+                return response_obj
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 439 - 459, The semantic-cache hit path finalizes the
request before response construction and hadith annotation complete. In the
cached-response branch, move `_finalize()` and the `_succeeded = True`
assignment to after `ChatResponse(...)` is successfully constructed, including
`annotate_hadith(cached.response)`, so exceptions are recorded only as failures
and successful cache hits are finalized exactly once.

707-747: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Route streaming through the async Gemini API and add telemetry to the stream. chat_stream() calls synchronous send_message() and iterates the blocking stream inline inside an async endpoint, which can stall the event loop for the full response. It also skips the trace/request/model-call accounting used by /chat, so streamed usage won’t show up in /metrics. Use send_message_async(..., stream=True) or offload the whole stream, and finalize the trace once streaming ends.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 707 - 747, Update event_generator in chat_stream to use
the async Gemini send_message_async API with stream=True and asynchronously
consume the response, avoiding synchronous blocking on the event loop. Wrap the
streamed request with the same telemetry trace/request/model-call accounting
used by /chat, and finalize the trace after the stream completes so streamed
usage is recorded in /metrics.

Source: Path instructions


365-367: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Record the corrective Gemini call.

Citation correction can consume tokens but bypasses record_model_call, so request headers and aggregate cost/token metrics undercount corrected responses.

Proposed fix
-    corrective_response = await send_message_with_retry(chat_session, correction_prompt)
+    started = time.perf_counter()
+    corrective_response = await send_message_with_retry(chat_session, correction_prompt)
+    telemetry.record_model_call(
+        corrective_response,
+        telemetry.GEMINI_MODEL,
+        (time.perf_counter() - started) * 1000.0,
+        stage="citation_correction",
+    )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@main.py` around lines 365 - 367, Update the corrective-response flow around
send_message_with_retry in the citation correction path to record the Gemini
call through record_model_call, including its request headers and token/cost
metrics. Preserve extract_text_safely and the existing safe_text or
original_text fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@main.py`:
- Around line 439-459: The semantic-cache hit path finalizes the request before
response construction and hadith annotation complete. In the cached-response
branch, move `_finalize()` and the `_succeeded = True` assignment to after
`ChatResponse(...)` is successfully constructed, including
`annotate_hadith(cached.response)`, so exceptions are recorded only as failures
and successful cache hits are finalized exactly once.
- Around line 707-747: Update event_generator in chat_stream to use the async
Gemini send_message_async API with stream=True and asynchronously consume the
response, avoiding synchronous blocking on the event loop. Wrap the streamed
request with the same telemetry trace/request/model-call accounting used by
/chat, and finalize the trace after the stream completes so streamed usage is
recorded in /metrics.
- Around line 365-367: Update the corrective-response flow around
send_message_with_retry in the citation correction path to record the Gemini
call through record_model_call, including its request headers and token/cost
metrics. Preserve extract_text_safely and the existing safe_text or
original_text fallback.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ec3c7e3a-9fd2-4205-ac50-e493215b3861

📥 Commits

Reviewing files that changed from the base of the PR and between b1c8265 and f0a1e26.

📒 Files selected for processing (1)
  • main.py

@zeemscript
zeemscript merged commit e1a020a into Deen-Bridge:dev Jul 25, 2026
2 checks passed
zeemscript added a commit that referenced this pull request Jul 25, 2026
Merge pull request #69 from mzterwalexzyy/feat/llm-observability

feat: LLM observability - per-request token, cost, and latency telemetry with tracing (Closes #61)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants