Repository navigation
feat(tracing): correlate spend logs with agent traces - #43865
yujonglee-berri wants to merge 12 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
0abf1e4 to
c7482fd
Compare
b0393e3 to
263c404
Compare
8d323d9 to
5e4f8be
Compare
There was a problem hiding this comment.
Devin Review found 7 potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| litellm_settings: | ||
| success_callback: | ||
| - clickhouse |
There was a problem hiding this comment.
🔴 Failed requests lose spend rows
When configured through success_callback, ClickHouseSpendLogger never receives failed requests. Its failure handler runs only for callbacks registered for failures, leaving failed calls absent from spend logs
| litellm_settings: | |
| success_callback: | |
| - clickhouse | |
| litellm_settings: | |
| callbacks: | |
| - clickhouse |
Was this helpful? React with 👍 or 👎 to provide feedback.
| sum(SpanCount) AS SpanCount, sum(AgentCount) AS AgentCount, sum(LlmCount) AS LlmCount, | ||
| sum(ToolCount) AS ToolCount, sum(ErrorCount) AS ErrorCount, sum(InputTokens) AS InputTokens, | ||
| sum(OutputTokens) AS OutputTokens, groupUniqArrayArray(Models) AS Models, | ||
| groupUniqArrayArray(AgentNames) AS AgentNames, groupArrayArray(RequestIds) AS RequestIds |
There was a problem hiding this comment.
🔴 Repeated span IDs inflate trace cost
When multiple spans carry one LiteLLMRequestId, RequestIds repeats that ID. The array join sums its spend repeatedly, so the trace list overstates cost
Learn more
The agent trace materialized view stores one request ID per span, including repeated IDs across partial rows. The list query expands this array into one row per ID before summing joined spend. Duplicate IDs thus count the same request multiple times, even though trace_from_rows derives the detail cost separately from spans
Example: Two spans both carry response-1 and its spend log costs $0.05. The list reports $0.10 instead of $0.05
Recommended fix: Deduplicate request IDs within each trace before the array join, and add a ClickHouse integration test with repeated span response IDs
Was this helpful? React with 👍 or 👎 to provide feedback.
| _clickhouse_spend_logger: Final = ClickHouseSpendLogger() | ||
| _in_memory_loggers.append(_clickhouse_spend_logger) |
There was a problem hiding this comment.
🔴 Spend-only setup loses every batch
When clickhouse runs without tracing enabled, ClickHouseSpendLogger never creates its spend_logs table. Fresh installations reject every batch and eventually drop all spend rows
Learn more
The callback constructs a ClickHouse storage client but never initializes its table. TraceReceiver.start is the only production caller of schema initialization, and startup skips it unless general_settings.tracing.store is clickhouse. ClickHouseBatchLogger._insert retries failed inserts and then drops the batch
Example: Enable only litellm_settings.callbacks: [clickhouse] with a new ClickHouse database. The first insert fails because spend_logs does not exist, and three attempts later its rows disappear
Recommended fix: Initialize the spend schema when starting the callback independently of trace ingestion, or reject a callback-only configuration explicitly until its table is ready
Was this helpful? React with 👍 or 👎 to provide feedback.
| _clickhouse_spend_logger: Final = ClickHouseSpendLogger() | ||
| _in_memory_loggers.append(_clickhouse_spend_logger) |
There was a problem hiding this comment.
🔴 Synchronous setup leaves spend queue idle
When ClickHouseSpendLogger initializes outside an event loop, its periodic flush never starts. Calls below the batch threshold remain queued indefinitely and vanish on exit
Learn more
ClickHouseBatchLogger.init suppresses the missing-loop error without scheduling a later start. Its start method exists but no production caller invokes it, and enqueue triggers a flush only at the batch threshold
Example: An SDK process configures the callback synchronously and makes 10 requests in an event loop. With the default 10,000-row batch, those rows never reach ClickHouse
Recommended fix: Schedule the periodic flush on the first async event or explicitly start the callback when the application loop becomes available, with idempotent task lifecycle handling
Was this helpful? React with 👍 or 👎 to provide feedback.
| @@ -70,7 +70,6 @@ def from_env(cls) -> "TraceReceiver": | |||
| TraceStorage( | |||
| database=os.getenv("CLICKHOUSE_DATABASE", "litellm"), | |||
| url=os.environ["CLICKHOUSE_URL"], | |||
There was a problem hiding this comment.
🟡 Configured trace reader is ignored
When CLICKHOUSE_READER_URL points to a separate reader, TraceReceiver uses CLICKHOUSE_URL for every read instead. Read traffic shifts to the writer, bypassing the configured reader
Learn more
Previously NativeTraceStorage accepted a separate reader URL, while this call now passes only the writer. Its constructor creates a reader connection using that same writer URL. Existing installations with CLICKHOUSE_READER_URL no longer direct traffic to their reader
Example: Set CLICKHOUSE_URL to the primary and CLICKHOUSE_READER_URL to a replica. All trace list and detail requests now query the primary
Recommended fix: Keep an optional separate reader endpoint in the native constructor and use the reader environment variable when present, falling back to the writer URL only if absent
Was this helpful? React with 👍 or 👎 to provide feedback.
| AND (empty({team_ids:Array(String)}) OR team_id IN {team_ids:Array(String)}) AND ({api_key_hash:String} = '' OR api_key = {api_key_hash:String}) | ||
| ) AS s ON s.response_id = o.LiteLLMRequestId |
There was a problem hiding this comment.
| LITELLM_MASTER_KEY: test | ||
| LITELLM_SALT_KEY: sk-local-tracing-salt-key | ||
| DATABASE_URL: postgresql://litellm:litellm@db:5432/litellm | ||
| STORE_MODEL_IN_DB: "True" | ||
| CLICKHOUSE_URL: http://default:local-tracing-write@clickhouse:8123 | ||
| CLICKHOUSE_DATABASE: default |
There was a problem hiding this comment.
|
| class ClickHouseSpendLogger(ClickHouseBatchLogger): | ||
| table = SPEND_LOGS_TABLE |
| .map(|value| Connection::reader(value, &database)) | ||
| .transpose() | ||
| .map_err(map_error)?, | ||
| reader: Connection::reader(url, &database).map_err(map_error)?, |
There was a problem hiding this comment.
Separate trace readers stop working If a deployment uses
CLICKHOUSE_READER_URL for read credentials or a replica, this now ignores it and sends reads to the writer. A writer without SELECT permission cannot serve trace reads. The repository requires a user-controlled flag for this backwards-incompatible change.
Rule Used: What: avoid backwards-incompatible changes without user-controlled flags Why: This breaks current behaviour for users using existing functionality Example of BAD: this PR (#22164) introduced run_post_custom... (source)
| ) AS t | ||
| LEFT ARRAY JOIN t.RequestIds AS request_id | ||
| LEFT JOIN ( | ||
| SELECT response_id, any(spend) AS spend FROM spend_logs FINAL WHERE (empty({team_ids:Array(String)}) OR team_id IN {team_ids:Array(String)}) AND ({api_key_hash:String} = '' OR api_key = {api_key_hash:String}) GROUP BY response_id |
| input_tokens=int(row["input_tokens"]), | ||
| output_tokens=int(row["output_tokens"]), | ||
| litellm_request_id=row["litellm_request_id"] or None, | ||
| litellm=_litellm_request(row), |
|
|
||
|
|
||
| def test_success_row_mapping(): | ||
| row = spend_log_row_from_payload(_payload(), {}) # type: ignore[arg-type] |
There was a problem hiding this comment.
Banned type suppression added This test uses
# type: ignore, which the repository guide bans because pyright does not honor it. The changed trace-store tests also use it. This repository requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
| tracing: | ||
| store: clickhouse # + CLICKHOUSE_URL env var | ||
| store: clickhouse | ||
| export CLICKHOUSE_URL=<ClickHouse URL> |
| read_queries! { | ||
| ListTraces => "list_traces" => "list_traces.sql", | ||
| TraceSpans => "trace_spans" => "trace_spans.sql", | ||
| SpanDetail => "span_detail" => "span_detail.sql", |
There was a problem hiding this comment.
Spend joins lack database tests The database-backed named-read test runs only
SpanDetail, not the new list or span spend joins. A query-planning or join regression could therefore reach deployment without a failing test.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
df529d4 to
b7ab950
Compare
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
55edb12 to
0841d91
Compare
b7ab950 to
4a5f738
Compare
TLDR
Problem this solves:
How it solves it:
Intentional product change: Trace rows show cost and request details when a spend row matches. Missing spend remains unavailable rather than appearing as $0
User Flow
Before: an operator can inspect an agent trace, but cannot see the LiteLLM request or cost behind its LLM spans
After: the same trace shows the matching request and cost
clickhousecallback, then open http://localhost:4000/ui/?page=logsPre-Submission checklist
Type
New Feature
Caveats (if any)
Medium
clickhousecallback and a matching response idLow
Final Attestation