Handle backend 413 responses for span exports - #2338
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe OTLP exporter now converts HTTP 413 responses into Merge Risk: 🔵 Low · up to When compression is enabled, oversized responses may record the uncompressed payload size instead of the bytes actually transmitted, which can make retry diagnostics or size-based handling inaccurate. The change is otherwise mergeable with explicit owner awareness or a follow-up fix. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
|
CodeRabbit chat interactions are restricted to organization members for this repository. Ask an organization member to interact with CodeRabbit, or set |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Thanks! |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 4/5
- In
tests/exporters/test_retry_fewer_spans.py, removing the exception-injection regression test reduces coverage for the diagnostic re-export suppression path; a future change could leak the underlying exception instead of returningSpanExportResult.FAILUREwithout being detected—restore or replace the test.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/exporters/test_retry_fewer_spans.py">
<violation number="1">
P2: This removes the exception-injection regression test that verified a failure during the diagnostic re-export returns SpanExportResult.FAILURE instead of leaking the underlying exception. That suppression path is still live in otlp.py ('with handle_internal_errors' around 'super().export([error_span])'), and no other test covers it, so the previously-fixed RuntimeError leak can regress undetected. Keep a test that raises in the diagnostic export (first call BodyTooLargeError, second Raise RuntimeError) and asserts FAILURE after exactly two export attempts.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| @@ -1,8 +1,10 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
P2: This removes the exception-injection regression test that verified a failure during the diagnostic re-export returns SpanExportResult.FAILURE instead of leaking the underlying exception. That suppression path is still live in otlp.py ('with handle_internal_errors' around 'super().export([error_span])'), and no other test covers it, so the previously-fixed RuntimeError leak can regress undetected. Keep a test that raises in the diagnostic export (first call BodyTooLargeError, second Raise RuntimeError) and asserts FAILURE after exactly two export attempts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/exporters/test_retry_fewer_spans.py, line 165:
<comment>This removes the exception-injection regression test that verified a failure during the diagnostic re-export returns SpanExportResult.FAILURE instead of leaking the underlying exception. That suppression path is still live in otlp.py ('with handle_internal_errors' around 'super().export([error_span])'), and no other test covers it, so the previously-fixed RuntimeError leak can regress undetected. Keep a test that raises in the diagnostic export (first call BodyTooLargeError, second Raise RuntimeError) and asserts FAILURE after exactly two export attempts.</comment>
<file context>
@@ -1,10 +1,10 @@
from __future__ import annotations
-import json
from collections.abc import Sequence
from typing import cast
import pytest
+from inline_snapshot import snapshot
from opentelemetry.sdk.resources import Resource
</file context>
Summary
BodyTooLargeErrorsignal so multi-span payloads are retried in smaller batchesReadableSpaninstead of asserting or relying on suppressed instrumentationFixes #1035.
Tests
uv run --no-sync pytest -q tests/exporters— 21 passedmake lint— passedmake typecheck— passedAI assistance was used to develop and review this change; the diff and reported commands were manually inspected and executed.