Repository navigation
opentelemetry-exporter-otlp-proto-grpc: tolerate invalid timeout env vars - #5448
Conversation
3baee13 to
76c758f
Compare
173ff7a to
d46d1ad
Compare
Pull request dashboard statusMerged · refreshed 2026-10-07 01:21 UTC Status above doesn't look right?
|
The gRPC exporters parse their timeout environment variables with a bare float(), so a value like OTEL_EXPORTER_OTLP_TIMEOUT=10s raises ValueError during exporter construction and takes down SDK startup. Add a shared _timeout_from_env() helper in opentelemetry-exporter-otlp- proto-common that warns and falls back instead, matching how the repo already handles invalid temporality and export-interval values, and use it from the gRPC mixin and the three signal exporters. Precedence between the signal-specific and generic variables is unchanged. Also fix `timeout or ...` silently discarding an explicit timeout=0; it is now `timeout is not None`. The HTTP exporters got the same treatment in open-telemetry#5389, so this change is limited to the gRPC exporters.
969244c to
8cb472c
Compare
Eason09053360
changed the title
|
Rebased onto main and rescoped this PR to the gRPC exporters only. While this was open, #5389 landed the same warn-and-fall-back behaviour for the HTTP exporters (_resolve_timeout() in otlp/proto/http/_common), including the timeout=0 precedence fix. That made the HTTP half of this PR redundant and was the source of the merge conflicts, so I dropped it. What remains is the gRPC side, which still parses its timeout env vars with a bare float(): grpc/exporter.pyself._timeout = timeout or float(environ.get(OTEL_EXPORTER_OTLP_TIMEOUT, 10)) grpc/{trace,metric,_log}_exporter/init.pyenviron_timeout = environ.get(OTEL_EXPORTER_OTLP_TRACES_TIMEOUT) The shared _timeout_from_env() helper stays in opentelemetry-exporter-otlp-proto-common, since the gRPC exporter depends on that package but not on the newer opentelemetry-exporter-otlp-common. One follow-up worth doing separately: _resolve_timeout() is now duplicated between otlp-proto-http and otlp-json-http, and neither names the offending environment variable in its warning (the thing @herin049 asked for here). Happy to open a PR unifying those if that sounds useful. |
Eason09053360
changed the title
tammy-baylis-swi
moved this to In progress
in Python: Align export_timeout_millis across SDK
Only the gRPC exporters use it now that the HTTP side landed in open-telemetry#5389. It lives in the package __init__ rather than exporter.py because the logger there has a DuplicateFilter keyed on the message template, which would drop the warning for a second invalid variable.
Honouring an explicit timeout=0 makes the gRPC exporter set a deadline that has already passed, so every export fails with DEADLINE_EXCEEDED. Declarative config passes timeout=0 for "no limit", so that needs its own change. Keep the existing `timeout or ...` fallback here.
|
I also dropped the timeout=0 change, so the gRPC exporters keep their existing timeout or ... fallback. If an explicit timeout=0 is honoured, the gRPC exporter sets a deadline that has already passed, and every export fails with DEADLINE_EXCEEDED. Declarative config also passes timeout: 0 to mean "no limit". |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused changes preserve timeout precedence, include regression coverage, and have no identified blocking issues.
Review effort: Balanced
Findings: None
What changed in this PR
Makes OTLP gRPC exporter startup tolerate malformed timeout environment variables, complementing the HTTP handling delivered in #5389.
Changes:
- Adds shared timeout parsing with warnings and fallback behavior.
- Applies it to the mixin and trace, metric, and log exporters while preserving precedence.
- Adds regression tests and a changelog entry.
| File | Description |
|---|---|
| exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_timeout_from_env.py | Tests parsing, fallback, precedence, and warnings. |
| exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py | Tests invalid trace timeout handling. |
| exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_exporter_mixin.py | Tests generic timeout fallback. |
| exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/trace_exporter/__init__.py | Uses shared parsing for traces. |
| exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/metric_exporter/__init__.py | Uses shared parsing for metrics. |
| exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py | Uses shared parsing for the generic timeout. |
| exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/_log_exporter/__init__.py | Uses shared parsing for logs. |
| exporter/opentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/__init__.py | Defines the timeout parsing helper. |
| .changelog/5448.fixed | Documents the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ar (open-telemetry#5705) * fix(prometheus): tolerate invalid OTEL_EXPORTER_PROMETHEUS_PORT env var _AutoPrometheusMetricReader parsed OTEL_EXPORTER_PROMETHEUS_PORT with a bare int() in __init__, so a non-integer value (e.g. "9464abc") or an empty string raised ValueError while building the auto-configured reader and took opentelemetry-instrument/SDK startup down. This is the same class of startup crash already fixed for the OTLP exporters (HTTP _resolve_timeout in open-telemetry#5389, gRPC _timeout_from_env in open-telemetry#5448) and the Zipkin exporters (open-telemetry#5704). Add a private _port_from_env() that skips unset/empty values silently, logs a warning for non-integer values, and falls back to the 9464 default. Explicit configuration is unaffected (the reader's auto server only reads the env var). Assisted-by: Doubao * changelog: add fragment for PR open-telemetry#5705 * test(prometheus): ruff-format the new entrypoint timeout tests The `misc / precommit` job failed because the two added test signatures were split across lines while ruff (v0.16.0, line-length 120) keeps them on one line. Run `ruff format` so `pre-commit run ruff-format` is clean.
Description
The OTLP gRPC exporters parse their timeout env vars with a bare
float()in__init__, so an invalid value likeOTEL_EXPORTER_OTLP_TIMEOUT=10s— or a declared-but-empty variable — raisesValueErrorduring exporter construction and takes down SDK startup.This adds a
_timeout_from_env()helper inopentelemetry-exporter-otlp-proto-grpcthat warns and falls back to the default instead, matching how the repo already handles invalid temporality and export-interval values. Applied to the four gRPC call sites (mixin, trace, metric, log); precedence between the signal-specific and generic variables is unchanged.Scope note: this PR originally covered the HTTP exporters as well. #5389 landed the same warn-and-fall-back behaviour for HTTP while this was open (
_resolve_timeout()inotlp/proto/http/_common), so the HTTP half has been dropped and this is now gRPC-only. Deduplicating_resolve_timeout()betweenotlp-proto-httpandotlp-json-httpis left for a follow-up.Type of change
How Has This Been Tested?
Does This PR Require a Contrib Repo Change?
Checklist: