docs(insight): state HttpExporter timeout_ms scope - #732
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
wangyb-A
force-pushed
the
docs/insight-http-timeout-semantics
branch
from
September 16, 2026 17:33
44c8812 to
6c6f2d7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
The HttpExporter docstring said timeout_ms "bounds the whole request". It does not: http_send passes it to urllib, which applies it to each blocking socket operation. An endpoint that stops reading or goes silent fails at timeout_ms, but one that keeps consuming or sending bytes slowly can hold the request open for longer (measured in review of #720: 37 s with timeout_ms=2000 against a 1 byte/s server). - HttpExporter docstring: name the phases the timeout applies to (the connect, each write of the request, each read of the status line, headers, and a non-2xx error body; a 2xx body is never read), note that a future release may enforce timeout_ms as a whole-request deadline, and tell callers to size the function timeout with this in mind - http_send docstring: same per-operation description - README HttpExporter section: same caveat and reservation - rename test_timeout_is_enforced to test_timeout_applies_to_a_silent_peer, which is the case it exercises; no assertion changed No behaviour change.
wangyb-A
force-pushed
the
docs/insight-http-timeout-semantics
branch
from
September 16, 2026 18:17
6c6f2d7 to
b843146
Compare
Contributor
Codex AI reviewNo actionable findings. Residual test risk: the renamed test covers only a peer silent before responding; slow-drip responses and blocked writes remain untested. Reviewed commit |
nvasiu
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Corrects the
HttpExporterdocumentation oftimeout_ms. The docstring said the value "bounds the whole request". It does not:http_sendpasses it tourllib, which applies it to each blocking socket operation (the connect and every read). An endpoint that goes silent fails attimeout_ms; one that keeps sending bytes slowly can hold the request open for longer. Review of #720 measured 37 s withtimeout_ms=2000against a server sending one byte per second.This PR documents the behaviour rather than changing it. The wording reserves the right to enforce
timeout_msas a whole-request deadline in a future release and tells callers not to rely on a request being allowed to exceed it, so a later fix can ship as a bug fix against a documented reservation rather than as a behaviour change.Changes:
HttpExporterclass docstring: describes the per-operation semantics, the silent-vs-slow distinction, the future-deadline reservation, and the advice to size the function timeout accordingly.http_senddocstring: states thattimeoutisurllib's per-socket-operation timeout.### HttpExporter: the same one-sentence caveat and reservation.test_timeout_is_enforcedrenamed totest_timeout_applies_to_a_silent_peer, which is what the test exercises (a server that delays before sending anything). No assertion changed.No code path changes.
OTelExporterandOpenSearchExporterare not affected; they pass no timeout.Testing
hatch fmt --checkinpackages/aws-durable-execution-sdk-python-insight: ruff check and format clean.hatch run test:all packages/aws-durable-execution-sdk-python-insight/tests/test_http_exporter.py: 13 passed.Follows up on the review comment in #720 (#720 (comment)).