From bf6016fca502d085f1dbc9e54537838f8c01bf2d Mon Sep 17 00:00:00 2001 From: henry3260 Date: Fri, 24 Jul 2026 18:11:44 +0000 Subject: [PATCH] Align OTLP HTTP retry behaviour with the specification Per the OTLP spec, only 429, 502, 503 and 504 are retryable, and a Retry-After header on such a response must be honoured, falling back to exponential backoff only when it is absent. The HTTP exporters did neither: 429 was dropped immediately while 408 and every 5xx code was retried, and the backoff was computed before the request was sent, so Retry-After could never influence it. Retry exactly the four spec-mandated codes and let a parseable Retry-After value (delay-seconds or HTTP-date) override the backoff for that attempt, matching the rules the experimental otlp-common client already implements. --- .changelog/5460.changed | 1 + .../otlp/proto/http/_common/__init__.py | 52 +++++++++- .../otlp/proto/http/_log_exporter/__init__.py | 8 ++ .../proto/http/metric_exporter/__init__.py | 8 ++ .../proto/http/trace_exporter/__init__.py | 8 ++ .../metrics/test_otlp_metrics_exporter.py | 39 ++++++++ .../tests/test_common.py | 97 +++++++++++++++++++ .../tests/test_proto_log_exporter.py | 39 ++++++++ .../tests/test_proto_span_exporter.py | 39 ++++++++ 9 files changed, 286 insertions(+), 5 deletions(-) create mode 100644 .changelog/5460.changed create mode 100644 exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py diff --git a/.changelog/5460.changed b/.changelog/5460.changed new file mode 100644 index 00000000000..74a9ba555a5 --- /dev/null +++ b/.changelog/5460.changed @@ -0,0 +1 @@ +`opentelemetry-exporter-otlp-proto-http`: retry only 429/502/503/504 and honour `Retry-After`, per the OTLP spec diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py index 46db16dd86a..81ba4af13d2 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_common/__init__.py @@ -1,6 +1,11 @@ # Copyright The OpenTelemetry Authors # SPDX-License-Identifier: Apache-2.0 +import math +import time +from datetime import timezone +from email.utils import parsedate_to_datetime +from http import HTTPStatus from os import environ from typing import Literal @@ -14,6 +19,17 @@ # 64 MiB, in bytes. _DEFAULT_MAX_REQUEST_SIZE = 64 * 1024 * 1024 +# The OTLP specification lists exactly these response codes as retryable and +# requires that all other 4xx and 5xx codes are not retried. +_RETRYABLE_STATUS_CODES = frozenset( + { + HTTPStatus.TOO_MANY_REQUESTS.value, + HTTPStatus.BAD_GATEWAY.value, + HTTPStatus.SERVICE_UNAVAILABLE.value, + HTTPStatus.GATEWAY_TIMEOUT.value, + } +) + class RequestPayloadTooLargeError(Exception): """A serialized OTLP request exceeded the configured ``max_request_size``. @@ -24,11 +40,37 @@ class RequestPayloadTooLargeError(Exception): def _is_retryable(resp: requests.Response) -> bool: - if resp.status_code == 408: - return True - if resp.status_code >= 500 and resp.status_code <= 599: - return True - return False + return resp.status_code in _RETRYABLE_STATUS_CODES + + +def _extract_retry_after(resp: requests.Response) -> float | None: + """Parse the ``Retry-After`` header (RFC 7231) into a delay in seconds. + + Returns ``None`` when the header is absent or cannot be interpreted, in + which case the caller falls back to exponential backoff. A delay that has + already elapsed is returned as ``0.0``, meaning retry immediately. + """ + value = resp.headers.get("Retry-After") + if value is None: + return None + value = value.strip() + + # delay-seconds: a non-negative decimal integer. + try: + seconds = float(value) + except ValueError: + pass + else: + return max(seconds, 0.0) if math.isfinite(seconds) else None + + # HTTP-date: wait until the indicated absolute time. + try: + retry_at = parsedate_to_datetime(value) + except (TypeError, ValueError): + return None + if retry_at.tzinfo is None: + retry_at = retry_at.replace(tzinfo=timezone.utc) + return max(retry_at.timestamp() - time.time(), 0.0) def _is_request_too_large( diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py index c25ebec6756..be14e78f3ac 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/_log_exporter/__init__.py @@ -27,6 +27,7 @@ from opentelemetry.exporter.otlp.proto.http._common import ( _DEFAULT_MAX_REQUEST_SIZE, RequestPayloadTooLargeError, + _extract_retry_after, _is_request_too_large, _is_retryable, _load_session_from_envvar, @@ -262,6 +263,13 @@ def export( reason = resp.reason retryable = _is_retryable(resp) status_code = resp.status_code + # A server signalling backpressure gets to pick the delay. + if ( + retryable + and (retry_after := _extract_retry_after(resp)) + is not None + ): + backoff_seconds = retry_after if not retryable: _logger.error( diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py index 7020beb7f32..5b39b6e8f1f 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/metric_exporter/__init__.py @@ -41,6 +41,7 @@ from opentelemetry.exporter.otlp.proto.http._common import ( _DEFAULT_MAX_REQUEST_SIZE, RequestPayloadTooLargeError, + _extract_retry_after, _is_request_too_large, _is_retryable, _load_session_from_envvar, @@ -319,6 +320,13 @@ def _export_with_retries( reason = resp.reason retryable = _is_retryable(resp) status_code = resp.status_code + # A server signalling backpressure gets to pick the delay. + if ( + retryable + and (retry_after := _extract_retry_after(resp)) + is not None + ): + backoff_seconds = retry_after if not retryable: _logger.error( diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py index 56d0a92a9e6..f64b03d2f5a 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py @@ -29,6 +29,7 @@ from opentelemetry.exporter.otlp.proto.http._common import ( _DEFAULT_MAX_REQUEST_SIZE, RequestPayloadTooLargeError, + _extract_retry_after, _is_request_too_large, _is_retryable, _load_session_from_envvar, @@ -255,6 +256,13 @@ def export(self, spans: Sequence[ReadableSpan]) -> SpanExportResult: reason = resp.reason retryable = _is_retryable(resp) status_code = resp.status_code + # A server signalling backpressure gets to pick the delay. + if ( + retryable + and (retry_after := _extract_retry_after(resp)) + is not None + ): + backoff_seconds = retry_after if not retryable: _logger.error( diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/tests/metrics/test_otlp_metrics_exporter.py b/exporter/opentelemetry-exporter-otlp-proto-http/tests/metrics/test_otlp_metrics_exporter.py index 9ead610069e..f2382dc6ca7 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/tests/metrics/test_otlp_metrics_exporter.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/tests/metrics/test_otlp_metrics_exporter.py @@ -1538,6 +1538,45 @@ def test_export_no_collector_available(self, mock_post): warning.records[0].message, ) + @patch.object(Session, "post") + def test_too_many_requests_is_retried(self, mock_post): + exporter = OTLPMetricExporter(timeout=1.5) + + resp = Response() + resp.status_code = 429 + resp.reason = "TOO MANY REQUESTS" + mock_post.return_value = resp + with self.assertLogs(level=WARNING) as warning: + self.assertEqual( + exporter.export(self.metrics["sum_int"]), + MetricExportResult.FAILURE, + ) + # Retried once, then an early return before the second backoff sleep + # because it would exceed the timeout. + self.assertEqual(mock_post.call_count, 2) + self.assertIn( + "Transient error TOO MANY REQUESTS encountered while exporting metrics batch, retrying in", + warning.records[0].message, + ) + + @patch.object(Session, "post") + def test_retry_after_header_overrides_backoff(self, mock_post): + exporter = OTLPMetricExporter(timeout=1.5) + + resp = Response() + resp.status_code = 429 + resp.reason = "TOO MANY REQUESTS" + resp.headers["Retry-After"] = "10" + mock_post.return_value = resp + self.assertEqual( + exporter.export(self.metrics["sum_int"]), + MetricExportResult.FAILURE, + ) + # The server asked for longer than the remaining timeout allows, so the + # batch is dropped without a second attempt. The exponential backoff it + # replaced would have been ~1s and left room for a retry. + self.assertEqual(mock_post.call_count, 1) + @patch.object(Session, "post") def test_timeout_set_correctly(self, mock_post): resp = Response() diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py new file mode 100644 index 00000000000..55a756a6a06 --- /dev/null +++ b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_common.py @@ -0,0 +1,97 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +import unittest +from datetime import datetime, timedelta, timezone +from email.utils import format_datetime + +from requests.models import Response + +from opentelemetry.exporter.otlp.proto.http._common import ( + _extract_retry_after, + _is_retryable, +) + + +def _response(status_code: int, retry_after: str | None = None) -> Response: + resp = Response() + resp.status_code = status_code + if retry_after is not None: + resp.headers["Retry-After"] = retry_after + return resp + + +class TestIsRetryable(unittest.TestCase): + def test_retryable_status_codes(self): + # The OTLP specification lists exactly these codes as retryable. + for status_code in (429, 502, 503, 504): + with self.subTest(status_code=status_code): + self.assertTrue(_is_retryable(_response(status_code))) + + def test_non_retryable_status_codes(self): + # The specification requires that every other 4xx and 5xx code is not + # retried, including 408 and 5xx codes outside the table above. + for status_code in (200, 400, 401, 404, 408, 500, 501, 505): + with self.subTest(status_code=status_code): + self.assertFalse(_is_retryable(_response(status_code))) + + +class TestExtractRetryAfter(unittest.TestCase): + def test_missing_header(self): + self.assertIsNone(_extract_retry_after(_response(429))) + + def test_delay_seconds(self): + self.assertEqual(_extract_retry_after(_response(429, "30")), 30.0) + + def test_delay_seconds_surrounding_whitespace(self): + self.assertEqual(_extract_retry_after(_response(429, " 30 ")), 30.0) + + def test_header_name_is_case_insensitive(self): + resp = Response() + resp.status_code = 429 + resp.headers["retry-after"] = "7" + self.assertEqual(_extract_retry_after(resp), 7.0) + + def test_zero_delay_is_honoured_as_immediate_retry(self): + # A server asking for a zero-second delay gets a retry with no backoff. + # This matches the specification's "honour the value" requirement, so + # the caller must not treat 0.0 as "no header present". + self.assertEqual(_extract_retry_after(_response(429, "0")), 0.0) + + def test_negative_delay_clamped_to_zero(self): + self.assertEqual(_extract_retry_after(_response(429, "-5")), 0.0) + + def test_non_finite_delay_falls_back_to_backoff(self): + for value in ("inf", "-inf", "nan"): + with self.subTest(value=value): + self.assertIsNone(_extract_retry_after(_response(429, value))) + + def test_malformed_header_falls_back_to_backoff(self): + for value in ("", "soon", "Wed, 99 Xxx 2015 07:28:00 GMT"): + with self.subTest(value=value): + self.assertIsNone(_extract_retry_after(_response(429, value))) + + def test_http_date_in_the_future(self): + retry_at = datetime.now(timezone.utc) + timedelta(seconds=30) + retry_after = _extract_retry_after( + _response(429, format_datetime(retry_at, usegmt=True)) + ) + # Second-granularity truncation in the HTTP-date format costs up to 1s. + self.assertAlmostEqual(retry_after, 30.0, delta=1.5) + + def test_http_date_in_the_past_is_immediate_retry(self): + retry_at = datetime.now(timezone.utc) - timedelta(seconds=30) + self.assertEqual( + _extract_retry_after( + _response(429, format_datetime(retry_at, usegmt=True)) + ), + 0.0, + ) + + def test_naive_http_date_is_treated_as_utc(self): + # A "-0000" offset means "unknown local offset" and parses to a naive + # datetime, which is interpreted as UTC rather than local time. + retry_at = datetime.now(timezone.utc) + timedelta(seconds=30) + header = retry_at.strftime("%a, %d %b %Y %H:%M:%S -0000") + retry_after = _extract_retry_after(_response(429, header)) + self.assertAlmostEqual(retry_after, 30.0, delta=1.5) diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_log_exporter.py b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_log_exporter.py index 3663b0eb9bc..868468fded8 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_log_exporter.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_log_exporter.py @@ -612,6 +612,45 @@ def test_export_no_collector_available(self, mock_post): metrics[2].data.data_points[0].attributes, ) + @patch.object(Session, "post") + def test_too_many_requests_is_retried(self, mock_post): + exporter = OTLPLogExporter(timeout=1.5) + + resp = Response() + resp.status_code = 429 + resp.reason = "TOO MANY REQUESTS" + mock_post.return_value = resp + with self.assertLogs(level=WARNING) as warning: + self.assertEqual( + exporter.export(self._get_sdk_log_data()), + LogRecordExportResult.FAILURE, + ) + # Retried once, then an early return before the second backoff sleep + # because it would exceed the timeout. + self.assertEqual(mock_post.call_count, 2) + self.assertIn( + "Transient error TOO MANY REQUESTS encountered while exporting logs batch, retrying in", + warning.records[0].message, + ) + + @patch.object(Session, "post") + def test_retry_after_header_overrides_backoff(self, mock_post): + exporter = OTLPLogExporter(timeout=1.5) + + resp = Response() + resp.status_code = 429 + resp.reason = "TOO MANY REQUESTS" + resp.headers["Retry-After"] = "10" + mock_post.return_value = resp + self.assertEqual( + exporter.export(self._get_sdk_log_data()), + LogRecordExportResult.FAILURE, + ) + # The server asked for longer than the remaining timeout allows, so the + # batch is dropped without a second attempt. The exponential backoff it + # replaced would have been ~1s and left room for a retry. + self.assertEqual(mock_post.call_count, 1) + @patch.object(Session, "post") def test_timeout_set_correctly(self, mock_post): resp = Response() diff --git a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py index fa8b5fc1f44..bba1a8b1ebc 100644 --- a/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py +++ b/exporter/opentelemetry-exporter-otlp-proto-http/tests/test_proto_span_exporter.py @@ -468,6 +468,45 @@ def test_export_no_collector_available(self, mock_post): metrics[2].data.data_points[0].attributes, ) + @patch.object(Session, "post") + def test_too_many_requests_is_retried(self, mock_post): + exporter = OTLPSpanExporter(timeout=1.5) + + resp = Response() + resp.status_code = 429 + resp.reason = "TOO MANY REQUESTS" + mock_post.return_value = resp + with self.assertLogs(level=WARNING) as warning: + self.assertEqual( + exporter.export([BASIC_SPAN]), + SpanExportResult.FAILURE, + ) + # Retried once, then an early return before the second backoff sleep + # because it would exceed the timeout. + self.assertEqual(mock_post.call_count, 2) + self.assertIn( + "Transient error TOO MANY REQUESTS encountered while exporting span batch, retrying in", + warning.records[0].message, + ) + + @patch.object(Session, "post") + def test_retry_after_header_overrides_backoff(self, mock_post): + exporter = OTLPSpanExporter(timeout=1.5) + + resp = Response() + resp.status_code = 429 + resp.reason = "TOO MANY REQUESTS" + resp.headers["Retry-After"] = "10" + mock_post.return_value = resp + self.assertEqual( + exporter.export([BASIC_SPAN]), + SpanExportResult.FAILURE, + ) + # The server asked for longer than the remaining timeout allows, so the + # batch is dropped without a second attempt. The exponential backoff it + # replaced would have been ~1s and left room for a retry. + self.assertEqual(mock_post.call_count, 1) + @patch.object(Session, "post") def test_timeout_set_correctly(self, mock_post): resp = Response()