Repository navigation
feat(tracing): add ingestion and UI on Rust foundation - #43864
yujonglee-berri wants to merge 8 commits into
Conversation
GitHub Actions
Error
|
|
GitHub Actions
Error
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
GitHub Actions
Error
GitHub Actions
Error
13ff79d to
535c3ad
Compare
0abf1e4 to
c7482fd
Compare
GitHub Actions
Error
GitHub Actions
Error
GitHub Actions
Error
b0c1c56 to
8786405
Compare
GitHub Actions
Error
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| content_encoding: Option<&str>, | ||
| max_decompressed_bytes: usize, | ||
| ) -> PyResult<Bound<'py, PyAny>> { |
There was a problem hiding this comment.
🟡 Invalid OTLP payloads return server errors
When OTLP decoding rejects malformed or oversized compressed input, trace_decode_otlp raises ValueError. The endpoint does not catch it, so exporters receive 500 instead of a payload error.
Learn more
The Rust decoder distinguishes invalid payloads from decompressed bodies over its size limit, but this binding converts both into a Python ValueError. ingest_otlp_traces only handles RuntimeError and TracingPayloadTooLargeError. A decode failure therefore escapes as HTTP 500, while a compressed body can pass the raw-body size check and fail its decompressed limit later.
Example: A client sends invalid protobuf bytes with Content-Type: application/x-protobuf; decoding fails and the proxy returns 500 rather than 400. A small gzip body expanding past 8 MiB also returns 500 rather than 413.
Recommended fix: Preserve DecodeError::TooLarge separately at the bridge boundary and map invalid payloads to a distinct exception. Translate both in ingest_otlp_traces to 413 and 400 respectively.
Was this helpful? React with 👍 or 👎 to provide feedback.
| .append_pair("async_insert", "1") | ||
| .append_pair("wait_for_async_insert", "1") | ||
| .append_pair("date_time_input_format", "best_effort"); |
There was a problem hiding this comment.
🔴 Retried exports duplicate trace totals
When ClickHouse commits an insert but its response is lost, insert_rows returns an error and the exporter retries. Without async-insert deduplication, both writes enter the rollup and double span and token counts.
Learn more
The endpoint acknowledges an export only after the ClickHouse HTTP response succeeds. If ClickHouse commits a batch but the response is lost or times out, the exporter retries that same batch. The materialized view increments span and token totals for every insertion; neither the request nor the table establishes retry deduplication.
Example: ClickHouse stores a 10-span batch, but the 30-second HTTP timeout fires. The exporter retries and gets 200. The list view reports 20 spans even though the trace contains only 10 unique spans.
Recommended fix: Establish a stable batch identity across exporter retries or deduplicate (TeamId, TraceId, SpanId) before rollup; validate the chosen ClickHouse async-insert deduplication settings and their behavior across supported versions and partial batches.
Was this helpful? React with 👍 or 👎 to provide feedback.
| LIMIT {{limit:UInt32}} | ||
| ) AS t | ||
| GROUP BY t.TraceId | ||
| ORDER BY start_ms DESC, t.TraceId DESC |
There was a problem hiding this comment.
🟡 Trace listing discards traces across teams
For admins viewing traces from several teams, LIST_TRACES_SQL limits grouped team rows before regrouping by trace ID. Multiple teams using the same trace ID consume page slots, so the visible page returns fewer traces and cursors skip remaining results.
Learn more
The inner query groups by both TeamId and TraceId and applies the page limit; the outer query groups again by TraceId. Admin scope has an empty team list, so equal trace IDs from different teams produce multiple inner rows but only one response row. The cursor is built from the final response rows in list_traces, and cannot recover team rows discarded by the inner limit.
Example: With page size 2, team A and team B each have trace ID same, and another trace next is older. Both same rows occupy the inner two slots, the response contains one result, and next never appears because there is no next cursor.
Recommended fix: Apply the page limit after aggregating by the public trace identity, or preserve the team identity in the response and cursor so pagination uses the same grouping key throughout.
Was this helpful? React with 👍 or 👎 to provide feedback.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| FROM {AGENT_TRACES_TABLE} AS a | ||
| WHERE (empty({{team_ids:Array(String)}}) OR TeamId IN {{team_ids:Array(String)}}) | ||
| AND ({{api_key_hash:String}} = '' OR TraceId IN ( | ||
| SELECT TraceId FROM {OTEL_TRACES_TABLE} WHERE {_SCOPE_OTEL})) |
There was a problem hiding this comment.
If two team-less keys export spans with the same trace ID, this query checks that the caller owns a span with that ID but reads a summary shared by both keys. The caller can see the other key’s root name and input preview, while the detail queries show only their own spans. The summary must be scoped by key, not trace ID alone.
How this was verified: The materialized view groups spans by team and trace ID without an API-key field, and the list query checks key ownership only through a trace-ID subquery.
Knowledge Base Used: Proxy authentication and authorization
| if _normalize_media_type(content_type) in _BINARY_CONTENT_TYPES: | ||
| parsed_body = _parse_binary_body(await request.body()) |
There was a problem hiding this comment.
If the optional request-size middleware is disabled, authentication buffers the entire protobuf body here, and the trace endpoint reads the body before checking the 8 MB OTLP limit. An authenticated exporter can therefore send a much larger body and exhaust proxy memory before receiving 413. The limit needs to apply before the body is buffered.
How this was verified: Authentication and ingestion both request the complete exporter-controlled body, while ingestion checks its size only afterward.
Knowledge Base Used: Proxy authentication and authorization
| # agent tracing: OTLP ingest + reads (scoped to the caller's team in the handler) | ||
| "/v1/traces", | ||
| "/v1/traces/{trace_id}", | ||
| "/v1/traces/{trace_id}/spans/{span_id}", |
There was a problem hiding this comment.
A view-only administrator can POST to /v1/traces: registering it as an LLM API route makes the route checker allow it before reaching the view-only restriction, and the ingestion handler does not check for write access. A read-only credential can consequently insert arbitrary traces.
How this was verified: The LLM-route branch permits the new path before the viewer check, and the POST handler accepts the authenticated identity without checking its role.
Knowledge Base Used: Proxy authentication and authorization
| ) | ||
| }) | ||
| .map_err(|error| PyValueError::new_err(error.to_string()))?; | ||
| litellm_host_python::Pythonized(spans).into_pyobject(py) |
There was a problem hiding this comment.
Invalid OTLP JSON or protobuf, as well as a gzip body that expands beyond the size limit, becomes a ValueError here. The ingestion handler does not catch it, so the proxy returns 500 instead of a client error or 413. Exporters receive the wrong signal for a request they need to correct.
| while self.log_queue: | ||
| batch = self.log_queue[: self.batch_size] | ||
| self.log_queue = self.log_queue[len(batch) :] | ||
| if not await self._insert(batch): |
There was a problem hiding this comment.
If a background flush is cancelled while an insert is pending, this code has already removed the batch from the queue. _insert restores rows only for ordinary exceptions, not cancellation, so unconfirmed rows can be lost during shutdown. Keep the batch available until the write is confirmed or cancellation cleanup restores it.
Knowledge Base Used: Preserve Failed Spend Logs Across Pod Restarts
| TRACE_SPANS_SQL: Final = f""" | ||
| SELECT o.SpanId AS span_id, o.ParentSpanId AS parent_span_id, o.SpanName AS name, | ||
| o.ObservationType AS type, o.AgentName AS agent, o.StatusCode AS status, | ||
| o.StatusMessage AS status_message, | ||
| toUnixTimestamp64Nano(o.Timestamp) AS start_ns, o.Duration AS duration_ns, | ||
| o.ServiceName AS service, o.InputPreview AS input_preview, o.Model AS model, | ||
| o.InputTokens AS input_tokens, o.OutputTokens AS output_tokens, | ||
| o.LiteLLMRequestId AS litellm_request_id | ||
| FROM {OTEL_TRACES_TABLE} AS o | ||
| WHERE o.TraceId = {{trace_id:String}} AND {_SCOPE_OTEL} | ||
| ORDER BY o.Timestamp | ||
| LIMIT 1 BY o.SpanId |
There was a problem hiding this comment.
When an agent run has more than 1,000 spans, this unpaginated detail query exceeds the native read transport’s 1,000-row limit, which is configured to throw on overflow. GET /v1/traces/{trace_id} then fails instead of displaying the trace tree.
| def enqueue(self, rows: list[dict[str, Any]]) -> None: | ||
| """Never awaits ClickHouse. Kicks off an early flush once a full batch is queued.""" | ||
| self.log_queue.extend(rows) | ||
| if len(self.log_queue) >= self.batch_size: | ||
| asyncio.get_running_loop().create_task(self.flush_queue()) |
There was a problem hiding this comment.
enqueue adds rows without checking CLICKHOUSE_MAX_BUFFERED_ROWS; is_full() is only advisory. The spend logger in PR #43865 calls enqueue directly, so sustained ClickHouse failures can grow its in-memory queue past the configured cap.
df529d4 to
b7ab950
Compare
GitHub Actions
Waiting
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| groupUniqArrayArray(AgentNames) AS AgentNames | ||
| FROM {AGENT_TRACES_TABLE} AS a | ||
| WHERE (empty({{team_ids:Array(String)}}) OR TeamId IN {{team_ids:Array(String)}}) | ||
| AND ({{api_key_hash:String}} = '' OR TraceId IN ( |
There was a problem hiding this comment.
Medium: Cross-key trace summary disclosure
A team-less key holder who knows another key’s trace ID can ingest a non-root span with that ID, then request /v1/traces to read the other key’s root input preview. The subquery checks ownership of the ID, but the outer query aggregates all rows with that team ID and trace ID; retain the key hash in the rollup and filter it before aggregation.
| .unwrap_or(SpanKind::Unspecified) | ||
| .as_str_name() | ||
| .to_owned(), | ||
| resource_attributes: resource_attributes.clone(), |
There was a problem hiding this comment.
Medium: Small OTLP exports can exhaust worker memory
An authenticated caller can send a roughly 2 KB gzip export containing 20,000 spans and one 64 KiB resource attribute; cloning that attribute into every span allocates about 1.25 GiB before the insert-size limit runs. Bound the span count and aggregate decoded size before making per-span resource copies.
PR overviewThe PR adds trace ingestion and a tracing UI on a Rust foundation, including OTLP processing and trace storage and listing. Two security issues remain open, with none addressed yet. A team-less key holder who knows another key’s trace ID can expose that key’s root input preview through trace listing. Authenticated callers can also submit a small compressed OTLP export that expands into roughly 1.25 GiB of allocations, potentially exhausting worker memory before insert-size limits apply. Open issues (2)
Fixed/addressed: 0 · PR risk: 7/10 |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
b7ab950 to
4a5f738
Compare
Pull request was closed
3 participants
This replaces #43816, which GitHub marked merged after its head briefly reached the foundation branch
TLDR
Problem this solves:
How it solves it:
Intentional product change: The Logs page gains Agent traces in All, and failed trace writes return 503 with Retry-After so exporters can retry
User Flow
Before: a developer exports agent spans, but the proxy has no trace view
After: the same spans appear as an agent trace after storage confirms the write
Pre-Submission checklist
Type
New Feature
Caveats (if any)
Medium
Low
Final Attestation