Skip to content

Commit 7a38203

Browse files
OmarAlJarrahclaude
andcommitted
fix: correct per-operation tracing and tidy HTTP transport edge cases
Pipeline tracing: - The per-operation HttpTracer lifecycle (operation_started / operation_succeeded / operation_failed) was emitted from TracingPolicy, which runs inside the retry and redirect wrappers and is re-entered once per attempt/hop. Gated on the first attempt, a call that failed once and then succeeded on a retry reported operation_failed and never operation_succeeded, and a redirect whose later hop failed reported operation_succeeded. - Add OperationTracingPolicy at a new outermost Stage.OPERATION that brackets the whole call from outside the wrappers, emitting operation_started before the chain and exactly one operation_succeeded / operation_failed on the final outcome. TracingPolicy keeps the per-attempt span and the per-request events (request_sent / response_*); default_pipeline wires both. Transports and utilities: - urllib: stop dropping a valid Content-Length under Content-Encoding. http.client does not decode content encodings, so the served body is the wire payload whose length is the upstream Content-Length; the header is accurate and is now surfaced (the decompressing requests/httpx/aiohttp adapters still drop it). - urllib and asyncio: report "Invalid status code" for an out-of-range status, matching the other adapters. - asyncio: match the chunked transfer-coding by token rather than substring so a coding such as "x-chunked" is not mistaken for chunked framing. - Share one cross-origin helper (http.common.url._origin) between the redirect and auth policies instead of duplicating it. Docs and the committed API surface baseline are updated accordingly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 7dcca56 commit 7a38203

17 files changed

Lines changed: 369 additions & 149 deletions

File tree

docs/pipelines.md

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,31 +37,44 @@ two parallel variants:
3737
```python
3838
from dexpace.sdk.http.stdlib import UrllibHttpClient
3939
from dexpace.sdk.core.pipeline import Pipeline
40-
from dexpace.sdk.core.pipeline.policies import LoggingPolicy, RetryPolicy, TracingPolicy
40+
from dexpace.sdk.core.pipeline.policies import (
41+
LoggingPolicy,
42+
OperationTracingPolicy,
43+
RetryPolicy,
44+
TracingPolicy,
45+
)
4146

4247
with Pipeline(
4348
UrllibHttpClient(),
44-
policies=[TracingPolicy(), LoggingPolicy(), RetryPolicy()],
49+
policies=[OperationTracingPolicy(), TracingPolicy(), LoggingPolicy(), RetryPolicy()],
4550
) as pipeline:
4651
response = pipeline.run(request, dispatch_ctx)
4752
```
4853

4954
The chain runs in declaration order. For the example above:
5055

51-
1. `TracingPolicy` opens a span.
52-
2. `LoggingPolicy` logs the request.
53-
3. `RetryPolicy` invokes the transport; retries on transient failures.
54-
4. The transport runner calls `UrllibHttpClient.execute(request)`.
55-
5. The unwinding mirror order: logging emits the response, tracing
56-
closes the span.
56+
1. `OperationTracingPolicy` emits `operation_started` for the whole call.
57+
2. `TracingPolicy` opens a span.
58+
3. `LoggingPolicy` logs the request.
59+
4. `RetryPolicy` invokes the transport; retries on transient failures.
60+
5. The transport runner calls `UrllibHttpClient.execute(request)`.
61+
6. The unwinding mirrors that order: logging emits the response, tracing
62+
closes the span, and operation-tracing emits the single
63+
`operation_succeeded` / `operation_failed` once the call settles.
64+
65+
In the canonical `default_pipeline`, `OperationTracingPolicy` sits *outside*
66+
the redirect and retry wrappers (so the per-operation lifecycle fires once and
67+
reflects the final outcome), while `TracingPolicy` sits *inside* them and opens
68+
one span per attempt / hop.
5769

5870
## Built-in policies
5971

6072
| Policy | Purpose |
6173
|-------------------------------------|----------------------------------------------------------|
6274
| `RetryPolicy` / `AsyncRetryPolicy` | Retry transient failures with backoff + `Retry-After`. Auto-buffers single-use request bodies when `total_retries > 0`. |
6375
| `LoggingPolicy` | Structured request/response logs with URL redaction. |
64-
| `TracingPolicy` | Open a span per request; OTel semantic-conv attributes. |
76+
| `OperationTracingPolicy` | Emit the per-operation lifecycle (`operation_started`, then one `operation_succeeded`/`operation_failed`) from outside the retry/redirect loop. |
77+
| `TracingPolicy` | Open a span per attempt and emit per-request tracer events; OTel semantic-conv attributes. |
6578
| `BearerTokenPolicy` (auth) | Acquire + cache + apply OAuth bearer tokens. |
6679
| `KeyCredentialPolicy` (auth) | Stamp an API key into a configurable header. |
6780
| `BasicAuthPolicy` (auth) | `Authorization: Basic <base64>`. |
@@ -88,16 +101,19 @@ Per-call opt-outs follow a convention:
88101
second `iter_bytes()` call raises `RuntimeError`. To keep retries safe
89102
without forcing every caller to remember `to_replayable()`, both
90103
`RetryPolicy.send` and `AsyncRetryPolicy.send` inspect the body up front
91-
and, when `total_retries > 0`, swap in a buffered replayable copy before
92-
the first attempt:
104+
and, when the *effective* retry total for the call is positive, swap in a
105+
buffered replayable copy before the first attempt:
93106

94107
```python
95-
if total_retries > 0 and request.body is not None and not request.body.is_replayable():
108+
settings = self._configure_settings(ctx.options)
109+
if settings["total"] > 0 and request.body is not None and not request.body.is_replayable():
96110
request = request.with_body(request.body.to_replayable())
97111
```
98112

99-
`total_retries == 0` (e.g. `RetryPolicy.no_retries()`) skips the buffering
100-
step so callers who explicitly opt out of retries pay no memory cost for a
101-
copy they will never use. Already-replayable bodies (`from_bytes`,
102-
`from_string`, `from_form`) flow through untouched because
103-
`to_replayable()` returns `self`.
113+
The decision reads the effective total *after* merging any per-call
114+
`retry_total` override (from `ctx.options`) with the constructor default, so a
115+
per-call `retry_total=3` over an instance built with `total_retries=0` still
116+
buffers, and a per-call `retry_total=0` (or `RetryPolicy.no_retries()`) skips
117+
the buffering so callers who opt out of retries pay no memory cost for a copy
118+
they will never use. Already-replayable bodies (`from_bytes`, `from_string`,
119+
`from_form`) flow through untouched because `to_replayable()` returns `self`.

packages/dexpace-sdk-core/src/dexpace/sdk/core/http/auth/policies.py

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
from ...pipeline.policy import Policy
1515
from ...pipeline.stage import Stage
1616
from ...util.clock import ASYNC_SYSTEM_CLOCK, SYSTEM_CLOCK, AsyncClock, Clock
17+
from ..common.url import _origin
1718
from .access_token import AccessTokenInfo, TokenRequestOptions
1819
from .challenge import parse_challenges
1920
from .challenge_handler import ChallengeHandler
@@ -439,29 +440,9 @@ async def _authorize(
439440
return request.with_header("Authorization", f"{token.token_type} {token.token}")
440441

441442

442-
_DEFAULT_PORTS: dict[str, int] = {"https": 443, "http": 80}
443443
_AUTH_ORIGIN_KEY: str = "_auth_origin"
444444

445445

446-
def _origin(url: Url) -> tuple[str, str, int | None]:
447-
"""Return the ``(scheme, host, port)`` origin tuple for ``url``.
448-
449-
The scheme and host are lower-cased and the port is resolved to its
450-
scheme default (443 for https, 80 for http) when not explicit, so two
451-
URLs that differ only in an implied/explicit default port compare equal.
452-
453-
Args:
454-
url: The URL to derive an origin from.
455-
456-
Returns:
457-
A ``(scheme, host, effective_port)`` tuple suitable for equality
458-
comparison.
459-
"""
460-
scheme = url.scheme.lower()
461-
port = url.port if url.port is not None else _DEFAULT_PORTS.get(scheme)
462-
return scheme, url.host.lower(), port
463-
464-
465446
def _crosses_recorded_origin(request: Request, ctx: PipelineContext) -> bool:
466447
"""Report whether ``request`` left the origin recorded for this operation.
467448

packages/dexpace-sdk-core/src/dexpace/sdk/core/http/common/url.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -320,4 +320,28 @@ def parse(cls, raw: str) -> Self:
320320
)
321321

322322

323+
_DEFAULT_PORTS: dict[str, int] = {"https": 443, "http": 80}
324+
325+
326+
def _origin(url: Url) -> tuple[str, str, int | None]:
327+
"""Return the ``(scheme, host, effective_port)`` origin tuple for ``url``.
328+
329+
The scheme and host are lower-cased and the port is resolved to its scheme
330+
default (443 for https, 80 for http) when not explicit, so two URLs that
331+
differ only in an implied/explicit default port compare equal. Shared by
332+
the redirect and auth policies for their cross-origin checks so the origin
333+
definition stays in one place.
334+
335+
Args:
336+
url: The URL to derive an origin from.
337+
338+
Returns:
339+
A ``(scheme, host, effective_port)`` tuple suitable for equality
340+
comparison.
341+
"""
342+
scheme = url.scheme.lower()
343+
port = url.port if url.port is not None else _DEFAULT_PORTS.get(scheme)
344+
return scheme, url.host.lower(), port
345+
346+
323347
__all__ = ["QueryParams", "Url"]

packages/dexpace-sdk-core/src/dexpace/sdk/core/pipeline/defaults.py

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
from .policies.redirect import RedirectPolicy
2020
from .policies.retry import RetryPolicy
2121
from .policies.set_date import SetDatePolicy
22-
from .policies.tracing_policy import TracingPolicy
22+
from .policies.tracing_policy import OperationTracingPolicy, TracingPolicy
2323
from .staged_builder import StagedPipelineBuilder
2424

2525
if TYPE_CHECKING:
@@ -44,15 +44,21 @@ def default_pipeline(
4444
"""Pre-configured `StagedPipelineBuilder` with the canonical stack.
4545
4646
Wires the policies that most consumers want by default in the order their
47-
stages dictate: redirectidempotencyretryset-date
48-
client-identity → auth → logging → tracing. Each policy is opt-out (pass
49-
``None``) or opt-in-with-override (pass a pre-configured instance to
50-
replace the default).
47+
stages dictate: operation-tracingredirectidempotencyretry
48+
set-date → client-identity → auth → logging → tracing. Each policy is
49+
opt-out (pass ``None``) or opt-in-with-override (pass a pre-configured
50+
instance to replace the default).
5151
5252
Idempotency sits before retry so a write request's ``Idempotency-Key`` is
5353
minted once and reused across every retry; ``set-date`` and
5454
``client-identity`` sit just inside the retry wrapper.
5555
56+
Two tracing policies cooperate: `OperationTracingPolicy` brackets the whole
57+
operation from outside the redirect / retry wrappers (so the per-operation
58+
lifecycle fires once and reflects the final outcome), while `TracingPolicy`
59+
opens a span and emits per-request events inside the wrappers, once per hop
60+
/ attempt.
61+
5662
Args:
5763
client: Terminal HTTP transport.
5864
redirect: Override for `RedirectPolicy`. ``None`` uses defaults.
@@ -73,6 +79,9 @@ def default_pipeline(
7379
immediate ``.build()``.
7480
"""
7581
builder = StagedPipelineBuilder(client)
82+
# Sorts to Stage.OPERATION (outermost), bracketing every hop / attempt so
83+
# the per-operation lifecycle fires once on the final outcome.
84+
builder.append(OperationTracingPolicy())
7685
builder.append(redirect or RedirectPolicy())
7786
builder.append(idempotency or IdempotencyPolicy())
7887
builder.append(retry or RetryPolicy())

packages/dexpace-sdk-core/src/dexpace/sdk/core/pipeline/policies/__init__.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
from .redirect import RedirectPolicy
1818
from .retry import RetryMode, RetryPolicy
1919
from .set_date import SetDatePolicy
20-
from .tracing_policy import TracingPolicy
20+
from .tracing_policy import OperationTracingPolicy, TracingPolicy
2121

2222
__all__ = [
2323
"AsyncClientIdentityPolicy",
@@ -28,6 +28,7 @@
2828
"ClientIdentityPolicy",
2929
"IdempotencyPolicy",
3030
"LoggingPolicy",
31+
"OperationTracingPolicy",
3132
"RedirectPolicy",
3233
"RequestHistory",
3334
"RetryMode",

packages/dexpace-sdk-core/src/dexpace/sdk/core/pipeline/policies/redirect.py

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
from typing import TYPE_CHECKING, ClassVar, Literal, cast
3030
from urllib.parse import urljoin
3131

32-
from ...http.common.url import Url
32+
from ...http.common.url import Url, _origin
3333
from ...http.request.method import Method
3434
from ..policy import Policy
3535
from ..stage import Stage
@@ -43,26 +43,6 @@
4343

4444
_REDIRECT_STATUSES: frozenset[int] = frozenset({301, 302, 303, 307, 308})
4545
_CONTENT_HEADER_PREFIX: str = "content-"
46-
_DEFAULT_PORTS: dict[str, int] = {"https": 443, "http": 80}
47-
48-
49-
def _origin(url: Url) -> tuple[str, str, int | None]:
50-
"""Return the ``(scheme, host, port)`` origin tuple for ``url``.
51-
52-
The scheme and host are lower-cased and the port is resolved to its
53-
scheme default (443 for https, 80 for http) when not explicit, so two
54-
URLs that differ only in an implied/explicit default port compare equal.
55-
56-
Args:
57-
url: The URL to derive an origin from.
58-
59-
Returns:
60-
A ``(scheme, host, effective_port)`` tuple suitable for equality
61-
comparison.
62-
"""
63-
scheme = url.scheme.lower()
64-
port = url.port if url.port is not None else _DEFAULT_PORTS.get(scheme)
65-
return scheme, url.host.lower(), port
6646

6747

6848
#: ``ctx.data`` key holding the per-operation ``HttpTracer``. The first policy

0 commit comments

Comments
 (0)