Skip to content

fix(otlp): make the OTLP/HTTP receiver spec compliant - #223

Open
JHf0912 wants to merge 1 commit into
agentevals-dev:mainfrom
JHf0912:fix/otlp-http-spec-compliance
Open

JHf0912 wants to merge 1 commit into
agentevals-dev:mainfrom
JHf0912:fix/otlp-http-spec-compliance

Conversation

@JHf0912

@JHf0912 JHf0912 commented Sep 21, 2026

Copy link
Copy Markdown

Closes #171.

Problem

/v1/traces and /v1/logs accepted uncompressed JSON or protobuf, but were not a compliant OTLP/HTTP server. Four conformance gaps, each reproduced end to end against the current code:

  1. No gzip support. A stock OTel Collector gzip-compresses by default. Those requests arrived with Content-Type: application/x-protobuf plus Content-Encoding: gzip, took the protobuf branch, and failed in ParseFromString with an opaque 500. Measured against a live receiver with the real opentelemetry-exporter-otlp-proto-http: the export failed after retries and every span was silently lost, while the identical export uncompressed succeeded. This is the default configuration of the most common client — not an edge case.

  2. Malformed payloads returned 500, not 400 + google.rpc.Status. There was no try/except on these routes and no exception_handler anywhere in the repo, so json.JSONDecodeError and protobuf DecodeError bubbled into Starlette's ServerErrorMiddleware. A 500 is in the retryable class, so clients retried bodies that could never succeed.

  3. Responses never mirrored the request Content-Type. Both handlers returned the hard-coded literal {"partialSuccess":{}} with media_type="application/json" — including for protobuf requests, which the spec requires be answered with a protobuf ExportTraceServiceResponse.

  4. Dropped spans and logs reported full success. The per-session caps were enforced by a bare continue (the log path did not even log a warning), and the response was an unconditional success, so the caller could not learn that data had been discarded.

Why this matters for an evaluation tool. agentevals derives its results from ingested traces. Gaps 1 and 4 mean traces disappear while ingest reports success, so evaluations run on incomplete data with no signal — and the discarded traces are disproportionately the long, busy, or failing runs, which is exactly when the trace matters most. Gap 2 turns a permanently bad payload into an unbounded retry loop against a local server.

Solution

Added api/otlp_http.py as an explicit OTLP/HTTP protocol layer — content negotiation, compression, google.rpc.Status error bodies, and exception handlers — leaving the route handlers genuinely thin. This follows the layering the module docstring already claimed and keeps the logic unit-testable without HTTP.

  • Compression is bounded on both axes, and the decompression runs off the event loop. This process shares one loop with the dashboard API, the UI WebSockets and the gRPC receiver, so blocking is a whole-process concern.
  • google.rpc.Status is used directly rather than hand-encoded. The deciding argument is that a hand-rolled encoder verified by a hand-rolled decoder proves nothing about conformance; Status.FromString(resp.content) is real evidence.
  • Exception handlers are registered on the OTLP app only. require_trace_manager is shared with streaming_routes and debug_routes, which must keep their {"detail": ...} error shape.
  • process_traces/process_logs now return an ExportResult, so rejection counts are reportable. The gRPC receiver shares those functions and had the identical silent-success bug, so it is fixed in the same change — otherwise the two transports would disagree.

Changes

File Change
api/otlp_http.py new — media-type resolution, bounded gzip decode, Status responses, app-scoped handlers
api/otlp_routes.py handlers reduced to read → process → respond
api/otlp_processing.py ExportResult, counts at each drop site, build_traces_response/build_logs_response
api/otlp_app.py registers the handlers on the OTLP app only
api/otlp_grpc.py gRPC parity — returns partial_success too
pyproject.toml, uv.lock googleapis-common-protos added as a direct dependency (see Notes)
tests/test_otlp_http_protocol.py new — 53 protocol conformance tests, no API keys, no server
tests/test_otlp_receiver.py ExportResult counts + gRPC partial_success
docs/otel-compatibility.md, docs/streaming.md, README.md conformance table, partial-success semantics, gzip support

No new trace, span, log or metric is introduced. The change is to what the receiver reports about itself.

Evaluation

No LLM-evaluation metrics apply here, and I have not invented any. This change alters the ingest transport and its error signalling, not model or agent behavior, so tool-call accuracy, trajectory success and token cost are unaffected by construction. The evaluation-relevant risk is data integrity, so that is what I measured.

Measurement Before After Method
Spans ingested from a default-configured OTLP exporter 0 (export failed after retries) 1 Real opentelemetry-exporter-otlp-proto-http → real uvicorn receiver
HTTP status for a gzip body 500 (non-retryable data, retryable code) 200 Same harness
Response to a malformed body 500, plain text or {"detail"} 400 + decodable google.rpc.Status ASGI transport
Dropped-at-cap visibility none (success reported) partialSuccess.rejectedSpans / rejectedLogRecords Pre-filled session, then one more record

Dataset / test cases: no dataset — the units of evidence are the 53 protocol tests plus a three-mode end-to-end run (gzip / library default / no compression) against a live server.

Reproduce the headline result:

uv run pytest -m "not integration and not e2e" -q      # 829 passed
uv run pytest tests/integration/ -m "integration and not e2e" -q   # 29 passed
uv run --with opentelemetry-exporter-otlp-proto-http --with opentelemetry-sdk python <e2e script>

Observability

The receiver's self-reporting is the observability surface here, so this is the core of the change rather than a side effect:

  • New: partial_success on both transports. rejected_spans / rejected_log_records plus an English error_message, populated only when records are actually dropped. Per spec, partial_success is left unset on full success, so a clean export is byte-identical to before in meaning.
  • New: google.rpc.Status bodies on every 4xx/5xx, encoding-mirrored. Previously these were plain text, {"detail": ...}, or nothing at all.
  • Changed: a logger.warning on the log cap, which was previously silent while the span path already warned.
  • No change to existing UI/SSE behaviour. Verified the UI never speaks OTLP: partialSuccess and /v1/traces appear only in the receiver code and docs.

How to view: point a Collector at :4318 and inspect the export response, or run the receiver in non-live mode and observe the 503 Status body. Example of a capped export response:

{"partialSuccess":{"rejectedSpans":"3","errorMessage":"3 span(s) rejected: session has reached maximum span limit (10000)"}}

("3" is a string because proto3 JSON maps int64 to string; the proto field is int64.)

Testing

  • uv run pytest -m "not integration and not e2e" -q → 829 passed, 6 skipped (CI's unit job)
  • uv run pytest tests/integration/ -m "integration and not e2e" -q → 29 passed
  • uv run ruff check --no-fix . and uv run ruff format --check . → clean
  • uv lock --check → clean
  • End-to-end against a real exporter, three compression modes

Coverage: normal path (JSON and protobuf, both signals) · boundaries (empty body, exactly-at-cap, at member cap, missing Content-Type) · failure paths (malformed JSON, malformed protobuf, truncated / corrupt / over-cap gzip, structural JSON violations, deep nesting, wrong Content-Encoding, unknown media type, unhandled exception → 500, live mode off → 503) · compatibility (full-success body shape, UI unaffected, main app error shape unchanged) · concurrency (decompression runs off the event loop; all candidates rejected in milliseconds).

No real LLM API is required. Every test uses httpx.ASGITransport against the app; the end-to-end script uses a local receiver and a local exporter with no provider calls. make test-e2e (which does need keys) was not run and is unaffected.

The new test file is deliberately outside tests/integration/: that directory is marked pytest.mark.integration per file and CI's test job runs -m "not integration and not e2e", so anything placed there would never execute in CI. This change is a conformance fix; its tests must gate it.

Notes for Reviewer

Four decisions worth your call:

  1. New direct dependency, googleapis-common-protos. Required for google.rpc.Status. It was already resolved in uv.lock (transitively via google-adk → google-api-core), so this promotes an existing pin rather than adding a package — but pyproject.toml warns that pip ignores lockfiles, so relying on the transitive path would be wrong. If you'd rather not depend on it, the fallback is hand-encoding field 2 of Status.
  2. partial_success counts session-cap drops and records with no trace_id, but NOT filtered non-gen_ai.* log records. Those are dropped by design — an app instrumented with many libraries emits far more non-GenAI logs than GenAI ones, so counting them would attach a permanent warning to every export with no possible user action. The filter is now documented explicitly rather than left implicit. Orphan-buffered logs are likewise not counted: they are deferred, not rejected.
  3. Error bodies mirror the request encoding, matching the reference Go receiver. The spec's literal wording ("the response body for all HTTP 4xx and HTTP 5xx responses MUST be a Protobuf-encoded Status message") carries no JSON exemption, and mirroring is the reading that does not also violate the Content-Type rule. Called out in the docs.
  4. The gzip member cap returns 400, not 413. The other caps are about size; this one says the body is structurally pathological. Both are permanent, which is what matters for retry behaviour — but it's a judgement call.

Deliberate behaviour changes:

  • The full-success body goes from {"partialSuccess":{}} to {}. These are proto3-JSON equivalent (an empty message is indistinguishable from unset), and the spec requires the field be left unset on success.
  • Trailing NUL padding after a gzip member is tolerated, matching gzip.GzipFile and the gzip FAQ. A hand-rolled zlib.decompressobj loop rejects it by default, so this is explicit — with a test, since it is the kind of subtle delta a refactor would silently reintroduce.
  • process_traces / process_logs now return ExportResult instead of None. Any out-of-tree caller would see a changed (additive) return type; all in-tree callers are updated.

Where to look hardest: the gzip decompression bounds. Decoding cost scales with the number of concatenated members, not with their output, so a body of 20-byte empty members costs CPU in proportion to its size while producing nothing — it would otherwise burn seconds and still answer 200. The member cap is what actually bounds the work; measured, an uncapped 1.25 MB body of empty members takes 5.90 s versus 19.6 ms rejected. The MAX + 1 probe in the read is load-bearing: it is what keeps "expanded past the cap" at 413 while corruption stays 400.

Known limitations, not addressed here:

  • Memory amplification. A 64 MiB decompressed payload becomes roughly 1 GiB of live Python objects after MessageToDict (~16x). This is inherent to materialising OTLP into dicts and needs a design decision, not a cap.
  • The wire-body size check runs after the body is read, so it bounds what is parsed rather than what is received. A body-limiting proxy remains the way to bound the socket; the docs say so.
  • _replay_orphan_logs / _absorb_orphan_for_trace bypass can_accept_log() when extending a session, so a session can exceed its cap after replay — meaning the reported rejected count can understate true drops in that path. Pre-existing and narrow.
  • Log injection via session_name (otlp_processing.py, %s rather than %r) is pre-existing on main and left alone to keep this diff focused. It is a one-line fix and worth its own PR.

Self-check

  • Reproducible? Yes — every claim above has a command, and the gzip counterfactual is reproducible against main (0 spans) vs this branch (1 span).
  • Breaks public API? The OTLP endpoints' wire behaviour changes by design; that is the fix. Repo-internal, process_* gains a return value. The UI and the main API's error shape are verified unaffected. Existing JSON clients are unaffected for valid payloads.
  • Small PR? No — 14 files. It is one coherent conformance fix, and splitting it would leave the receiver half-compliant (e.g. gzip without bounded decompression). If you would prefer it split, the natural seam is gzip + 400/Status first, then Content-Type mirroring + partial_success.
  • Tests and verification commands included? Yes, above.

The /v1/traces and /v1/logs endpoints accepted uncompressed JSON or protobuf
but were not a compliant OTLP/HTTP server. Four defects, each confirmed in
code and reproduced end to end:

- No gzip support. A stock OTel Collector gzip-compresses by default, so its
  requests failed in ParseFromString and returned an opaque 500. Verified
  against the real exporter: the export failed after retries and every span
  was silently lost, while uncompressed exports succeeded.
- Malformed payloads returned 500 instead of 400 + google.rpc.Status. A 500
  is retryable, so clients retried bodies that could never succeed.
- Responses never mirrored the request Content-Type; protobuf clients got
  JSON back.
- Spans and logs dropped at the per-session caps reported full success
  instead of partial_success, hiding data loss from the caller.

Adds otlp_http.py as the protocol layer (media-type resolution, gzip decode,
Status error bodies, app-scoped exception handlers) so the route handlers
stay thin and the logic is testable without HTTP. Compression is bounded on
both the wire body and the decompressed output, and concatenated gzip members
are counted: decoding cost scales with member count rather than output, so a
body of 20-byte empty members would otherwise burn seconds of CPU and still
answer 200. Decompression runs off the event loop, which this process shares
with the dashboard API, the UI and the gRPC receiver.

process_traces/process_logs now return an ExportResult so drops are reported;
the gRPC receiver shares those functions and had the same silent-success bug.

Tests: new tests/test_otlp_http_protocol.py (deliberately outside
tests/integration/, which CI excludes) plus unit coverage for the counts and
gRPC parity.
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.

[OTel] Make OTLP/HTTP receiver spec compliant

1 participant