Repository navigation
feat(traces): add Rust storage foundation - #43819
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2259b10 to
ed2e507
Compare
13ff79d to
535c3ad
Compare
535c3ad to
1f3eb17
Compare
There was a problem hiding this comment.
Devin Review found 7 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| <query>GRANT SELECT ON default.otel_traces</query> | ||
| <query>GRANT SELECT ON default.agent_traces</query> | ||
| <query>GRANT SELECT ON default.spend_logs</query> |
There was a problem hiding this comment.
🔴 Reader cannot query custom trace databases
When ensure_schema creates a nondefault database, litellm_traces_reader retains grants only on default. Queries against the configured trace tables fail with access denied
Learn more
The schema creator accepts any valid database identifier in schema_statements. The reader profile grants SELECT on tables in default only. When trace_query selects another database through Connection::configured, SELECT reads tables on which that user has no grants.
Example: Creating tables in trace_test succeeds. Querying trace_test.otel_traces as litellm_traces_reader fails with access denied.
Recommended fix: Match reader grants to the configured schema database during deployment or provisioning, and test a nondefault database with the restricted reader.
Was this helpful? React with 👍 or 👎 to provide feedback.
| PARTITION BY toDate(StartTs) | ||
| ORDER BY (TeamId, TraceId) |
There was a problem hiding this comment.
🔴 Trace rollups split across calendar days
When one trace spans midnight, StartTs puts its batches into different partitions. AggregatingMergeTree cannot merge the same (TeamId, TraceId) across partitions, leaving multiple incomplete rollup rows
Learn more
The materialized view agent_traces_mv emits one aggregate row per trace and insert batch, with StartTs equal to the minimum span timestamp in that batch. AggregatingMergeTree only merges rows with the same key within one partition. A trace with batches on different dates has two distinct aggregate rows even after merges. A read that expects one row per trace receives split counts and incompatible root fields.
Example: Team A inserts the root at 23:59 and an LLM span at 00:01 the next day with the same TraceId. The agent and LLM counts remain in separate rows instead of forming one trace.
Recommended fix: Choose a partition expression stable for all batches of a trace, or explicitly aggregate across partitions on every read and ensure retention matches the intended whole-trace lifetime.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ENGINE = ReplacingMergeTree(end_time) | ||
| PARTITION BY toYYYYMM(start_time) | ||
| ORDER BY (team_id, toDateTime(start_time), request_id) |
There was a problem hiding this comment.
🔴 Same-second spend logs collapse into one
When distinct spend logs share a team, second, and request ID, ReplacingMergeTree treats them as one row. A later merge silently discards one spend entry
Learn more
ReplacingMergeTree identifies duplicates using the ORDER BY tuple, not the version column. Converting start_time to DateTime truncates the DateTime64(3) milliseconds. Distinct request logs that reuse an ID within a second therefore have the same replacing key, and ClickHouse retains only the highest end_time on merge.
Example: Two entries for request req-1 from the same team start at 12:00:00.100 and 12:00:00.800. They share the key (team, 12:00:00, req-1) and one spend record disappears.
Recommended fix: Preserve millisecond precision in the ordering key and determine whether request ID is unique across all spend-log events. Use a key that identifies actual retry duplicates rather than unrelated events.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let response = client | ||
| .post(connection.url().clone()) | ||
| .body(statement) | ||
| .send() | ||
| .await |
There was a problem hiding this comment.
🔴 Schema setup can wait indefinitely
If ClickHouse accepts a schema request but never responds, ensure_schema has no request timeout. Python's ensure_schema future remains pending indefinitely, blocking startup paths awaiting it
| let response = client | |
| .post(connection.url().clone()) | |
| .body(statement) | |
| .send() | |
| .await | |
| let response = client | |
| .post(connection.url().clone()) | |
| .timeout(std::time::Duration::from_secs(15)) | |
| .body(statement) | |
| .send() | |
| .await |
Was this helpful? React with 👍 or 👎 to provide feedback.
| anyLastIf(SpanName, ParentSpanId = '') AS RootName, | ||
| anyLastIf(InputPreview, ParentSpanId = '') AS RootInput, | ||
| anyLastIf(StatusCode, ParentSpanId = '') AS RootStatus, |
There was a problem hiding this comment.
🟡 Missing-root batches erase trace root fields
When later batches contain no root span, anyLastIf produces empty root values for their rollup rows. anyLast merges those rows with the root batch and can replace the root name, input, and status with empties
Learn more
The view groups spans from each incoming insert into one row per trace. For a batch with no root span, anyLastIf returns the default empty string. The destination's RootName fields use SimpleAggregateFunction(anyLast, String), which can retain the empty value during part merges rather than preserve the original root.
Example: Insert a root span named agent in batch one, then an LLM child alone in batch two. The second rollup supplies an empty RootName, and the merged trace can lose agent.
Recommended fix: Store a nullable or explicitly conditional root aggregate that ignores absent root spans when combining batches, and test separate root and child inserts followed by merges.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if sql.trim().is_empty() { | ||
| return Err(Error::EmptySql); |
There was a problem hiding this comment.
🟥 Untrusted SQL bypasses reader-only intent
With a privileged ClickHouse account, execute_read accepts arbitrary SQL and relies on a request setting for read-only behavior. The account retains its underlying privileges, so a query that bypasses the setting can modify data.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if !matches!(url.scheme(), "http" | "https") || url.host().is_none() { | ||
| return Err(Error::InvalidUrl); |
There was a problem hiding this comment.
|
| RootName SimpleAggregateFunction(anyLast, String), | ||
| RootInput SimpleAggregateFunction(anyLast, String), | ||
| RootStatus SimpleAggregateFunction(anyLast, String), |
There was a problem hiding this comment.
Root fields can disappear When a root span arrives in one batch and child spans arrive in a later batch, the later batch has empty root fields. Merging those batches with
anyLast can overwrite the real values, leaving the trace list in PR #43864 with a blank name, input preview, or status.
| spend_log_retention_days: u32, | ||
| ) -> Result<(), Error> { | ||
| for statement in schema_statements(database, trace_retention_days, spend_log_retention_days)? { | ||
| let response = client |
There was a problem hiding this comment.
| for statement in schema_statements(database, trace_retention_days, spend_log_retention_days)? { | ||
| let response = client | ||
| .post(connection.url().clone()) | ||
| .body(statement) | ||
| .send() |
There was a problem hiding this comment.
Schema setup can stall startup If ClickHouse accepts a connection but stalls while handling a schema statement, this POST has no total request timeout. The proxy startup path in PR #43864 awaits tracing setup, so startup can remain blocked indefinitely.
| <query>GRANT SELECT ON default.otel_traces</query> | ||
| <query>GRANT SELECT ON default.agent_traces</query> | ||
| <query>GRANT SELECT ON default.spend_logs</query> |
There was a problem hiding this comment.
Reader grants exclude default database If this reader profile is installed with the tracing caller's default configuration from PR #43864, the tables are created in
litellm but these grants cover only default. Valid trace reads then fail with access denied.
| pub fn encode_rows(rows: Vec<BTreeMap<String, Value>>) -> Result<String, Error> { | ||
| rows.into_iter() | ||
| .map(|row| { | ||
| let encoded = row | ||
| .into_iter() | ||
| .map(|(name, value)| insert_value(&name, value).map(|value| (name, value))) | ||
| .collect::<Result<BTreeMap<_, _>, _>>()?; | ||
| serde_json::to_string(&encoded).map_err(|_| Error::InvalidRow) | ||
| }) | ||
| .collect::<Result<Vec<_>, _>>() | ||
| .map(|rows| rows.join("\n")) | ||
| } |
There was a problem hiding this comment.
Row encoding has no limit This exported function collects every serialized row and then joins them into another string without checking the total byte count. A large valid batch can consume substantial additional memory before its caller can enforce a write limit; the bounded insert path added in PR #43864 does not change this function.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
TLDR
Problem this solves:
How it solves it:
Pre-Submission checklist
Type
New Feature
Caveats (if any)
Medium
Low
Final Attestation