ST-10: per-attempt retry_attempt sibling-span emission across framework + direct-SDK wrappers - #23
Merged
Conversation
LiteLLM / LangChain / LlamaIndex wrappers emit one fortifyroot.<framework>.retry_attempt sibling span per HTTP attempt under the safety_wrapper / Traceloop parent. Shared §4.7.1 token-based framework-attempt registry under opentelemetry-instrumentation-fortifyroot. Bounded TTL + cap eviction defends against framework crashes leaving attempts open. LiteLLM emitter inherits CustomLogger (dispatch isinstance gate). LangChain handler hooks both on_chat_model_start (F1) and on_llm_start; receives Traceloop handler at construction time so multi-attempt retries under shared parent_run_id become OTel siblings under one parent (review-round-2 Major 5 / Blocker 2). SpanHolder.tokens list with LIFO detach in _end_span fixes the context-token leak that bled OTel ambient across pytest tests (review-round-2 Blocker 1). _OpenAITracingWrapper.__call__ now detaches its suppression token in finally (review-round-2 Blocker 3). LiteLLM emitter adds async_log_pre_api_call defensive hook (review-round-2 Blocker 4). LlamaIndex SpanHandler registered BEFORE OpenLLMetrySpanHandler; F4 outer-method dedup filter prevents double-emission across outer chat() and inner _chat(). Per-wrapper unit tests + end-to-end regression guard (test_no_leaked_ambient_context_after_simulated_workflow) included.
- Stabilize LangChain retry handler ordering and context cleanup - Filter FR implementation spans from legacy LangChain upstream assertions - Add retry-attempt prompt attribution coverage for LangChain, LiteLLM, and LlamaIndex - Fix LiteLLM retry emitter dispatch by ensuring CustomLogger compatibility - Infer bare Claude model IDs as Anthropic in LiteLLM retry-attempt routing - Update retry-attempt tests for token usage, prompt attrs, and instrumentation symmetry
manas-fortifyroot
changed the title
… Anthropic / Bedrock)
Add direct-SDK retry interception per RETRY_LOOP.md §4. Each direct SDK has its
own retry mechanism (max_retries=N, internal httpx retries, botocore Retryer);
this commit hooks them so every HTTP attempt fires the per-call instrumentor
and emits a fortifyroot.<provider>.retry_attempt sibling span the backend's
§4.5 dedup promotes to the canonical LLMUsageEvent.
New files (one per provider package):
- opentelemetry-instrumentation-openai/.../retry_handler.py
- opentelemetry-instrumentation-anthropic/.../retry_handler.py
- opentelemetry-instrumentation-bedrock/.../retry_handler.py
Per-SDK hook surface (pinned in ST-10.0 hook-table addendum):
- OpenAI + Anthropic: wrap _base_client.SyncHttpxClientWrapper.send (private
API; import-time wrappability check + WARNING log + skip on missing or
incompatible symbol; never crashes normal instrumentation).
- Bedrock: register public botocore event hooks before-send.bedrock-runtime.*
+ response-received.bedrock-runtime.* (NOT after-call.* — absent in
botocore 1.35.x).
All emitters check SUPPRESS_LANGUAGE_MODEL_INSTRUMENTATION_KEY and the §4.7.1
framework-attempt registry before emitting; an endpoint allow-list scopes
emission to OpenAI {chat/completions, completions, embeddings, responses,
messages}, Anthropic {messages, complete}, and Bedrock implicit.
OpenAI/Anthropic _finalize_success reads the non-streaming response body via
response.json() (httpx caches via _content; SDK re-read works) and copies id,
model, and usage.{prompt,completion,input,output}_tokens to the retry_attempt
span — required because the backend makes a qualifying retry_attempt the
canonical LLMUsageEvent (even single-attempt) and reads tokens from that
span. Streaming retry_attempt is skipped (detected via kwargs["stream"]); see
ST-10.4-FOLLOWUP-streaming-usage in SYSTEM_TESTS_PLAN.md for the deferred
SSE-chunk follow-up.
Anthropic-specific: _wrap / _awrap now wrap the underlying call in
`with trace.use_span(span, end_on_exit=False):` so retry_handler can find the
parent span via trace.get_current_span() — previously the default no-op
tracer was returned and no_parent_guard silently skipped emission, which is
why anthropic looked "green" while openai's retry_attempt did fire (parity
bug, not a real anthropic-side feature). retry_handler also uses
gen_ai.system="Anthropic" (title case) to match __init__.py's casing for
backend canonicalization.
OpenAI-specific: new OPENAI_DIRECT_RETRY_PARENT_ACTIVE_KEY context key
distinguishes chat_wrappers' own internal suppression (allow retry_attempt)
from external user/framework suppression (skip emission). Anthropic + Bedrock
don't have this collision — they only check the suppression key at entry and
never set it during the wrapped call.
Bedrock-specific: streaming _instrumented_model_invoke_with_response_stream
and _instrumented_converse_stream wrap the boto3 call in
`with trace.use_span(span, end_on_exit=False):` so the streaming span is
ambient for the duration of botocore event-hook firing. Event-hook level
skip for InvokeModelWithResponseStream / ConverseStream (response stream
isn't a finalized body at the callback site).
Test coverage:
- 80 new retry_attempt unit tests (37 openai + 27 anthropic + 16 bedrock):
tracer_provider plumbing, override-key bypass, external-suppression-still-
suppresses, streaming-skip behaviour, two-concurrent-async-sends regression
guard for the §4.7.1 self-registration C1 fix, endpoint allow-list,
no_parent_guard, importable-symbol check.
- Per-package _NoFortifyRootSpanExporter in conftest filters spans with
fortifyroot.span.role=retry_attempt from upstream exact-span-list
assertions (role-based not name-based, so legitimate fortifyroot.*.safety
/ .llm_wrapper / .has_native_otel_child spans remain visible).
- 18 OpenAI context-propagation tests updated to spy
_base_client.SyncHttpxClientWrapper.send after ST-10.4 wraps that surface;
test_responses_trace_context_propagation_unit now uses
provider.get_tracer(...) directly instead of mutating the global.
- 5 traceloop-sdk test files (test_association_properties, test_privacy_no_
prompts, test_prompt_management, test_sdk_initialization, test_workflows)
inline the same role-based retry_attempt filter so the new fortifyroot.*
spans don't trip upstream span-count / span-order assertions.
- instrument_retry_emitter(tracer_provider=...) accepts and stashes the
provider in all three packages so retry_attempt spans land on the same
exporter as the parent openai / anthropic / bedrock span — previously
they fell through to the global no-op when consumers passed an explicit
provider.
Validation:
- ./scripts/run-tests.sh --all → 31 executed packages, 1412/1412 passed,
0 failed.
- Companion fortifyroot-sdk-py poetry run pytest → 458 passed, 0 failed
(vendored locally per AGENT.md §17; SDK PR ships in ST-10.5).
- Companion fr-system-tests against local docker fr-backend carrying the
ST-10.0b §4.5 trace-summary parity fix → 10/10 representative scenarios
PASS (openai sync/streaming/async ×2, anthropic sync/streaming, bedrock
sync, litellm sync, llamaindex streaming, safety_openai streaming smoke).
…ived param names
The ``_response_received_hook`` in the Bedrock retry_handler was written
against an older botocore event signature (``http_response=`` +
``parsed=``). Current botocore (verified on 1.42.75) emits the
``response-received.bedrock-runtime.*`` event with ``response_dict=`` +
``parsed_response=`` kwargs instead — discovered during ST-10.6
implementation against dev-api.fortifyroot.com via the fr-system-tests
test_retry_loop.py probe.
Symptom under newer botocore: ``_response_received_hook`` received
``http_response=None`` and ``parsed=None`` because the matching kwargs
went into ``**_kwargs`` instead. ``_finalize_success`` then saw
``status_code=None`` → no OTel status set on the retry_attempt span →
backend's IsError returned False for every attempt → RetryDetector
skipped the sibling group on its ``failedCount == 0`` check → no
RetryLoopEvent for any Bedrock retry chain.
Fix:
- Add ``response_dict`` and ``parsed_response`` kwargs to the hook
signature so they bind by name instead of falling into
``**_kwargs``.
- Reconcile to the existing ``http_response`` / ``parsed`` body via
a new ``_ResponseDictAdapter`` class that exposes ``.status_code``
and ``.headers`` over a botocore-shaped ``{'status_code': int,
'headers': dict}`` dict. ``_finalize_success`` continues to work
unchanged because it reads from those two attributes.
- The existing ``http_response`` / ``parsed`` path is preserved
intact — the fork-side unit tests in
``tests/test_retry_attempt_emission.py`` that directly invoke the
hook with the old param names still pass.
This is PART 1 of the Bedrock retry-loop fix. PART 2 — re-designing the
``before-send`` ↔ ``response-received`` correlation mechanism so it
works under current botocore where ``before-send`` doesn't receive a
``context`` kwarg and ``request.context`` is ``None`` at fire time — is
tracked separately as ``ST-10.4-FOLLOWUP-bedrock-context-correlation``
in fr-system-tests/SYSTEM_TESTS_PLAN.md. Until PART 2 lands,
fr-system-tests/tests/sdk_telemetry/test_retry_loop.py::
test_bedrock_botocore_retries_429_then_200 remains
``@pytest.mark.xfail(strict=False)``.
Validation:
- ``./scripts/run-tests.sh --all``: 1412 passed, 0 failed, 32 skipped
(skips are pre-existing platform issues unrelated to ST-10).
- opentelemetry-instrumentation-bedrock: 116/116 passed (no
regression in the direct-invoke unit-test path).
Files:
packages/opentelemetry-instrumentation-bedrock/opentelemetry/
instrumentation/bedrock/retry_handler.py
manas-fortifyroot
marked this pull request as ready for review
May 20, 2026 05:49
manas-fortifyroot
changed the title
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
1 participant
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
Cross-package fork implementation of ST-10 retry-loop detection (per
fr-backend/docs/development/RETRY_LOOP.md§4): each HTTP attempt in a customer's retry chain now emits a dedicatedfortifyroot.<framework_or_provider>.retry_attemptsibling span under the existing parent span, sofr-backend'sproc_retry_detector.gocan group them by(parent_span_id, provider, model)and emit aRetryLoopEvent.Branch:
phase-st-10/framework-wrappers→fr-v0.52.6.x.Diff: 38 files, +8,512 / −107 (~5,500 LOC of new instrumentation + ~3,900 LOC of unit tests across 7 packages).
What landed by package
opentelemetry-instrumentation-fortifyroot— shared §4.7.1 framework-attempt registryNew
retry_registry.py(+284 LOC) — module-level token-based registry shared across all framework retry emitters:register_framework_attempt() → tokencalled at attempt-start by framework wrappers (LiteLLM/LangChain/LlamaIndex).unregister_framework_attempt(token)at attempt-end.is_framework_owned(tid=None) → boolconsulted by direct-SDK wrappers (OpenAI/Anthropic/Bedrock); if True, they SUPPRESS their own retry_attempt emission (§4.7 suppression discipline — prevents framework + direct double-emission)._REGISTRY_MAX=4096,_REGISTRY_STALE_TTL_SEC=60) defends against framework crashes leaving tokens orphaned._TOKEN_TO_TIDindex so terminal callbacks running on different threads than the start callback still unregister against the originating TID (review-round-1 blocker fix).tests/test_retry_registry.py(+304 LOC) covering re-entrancy, stale-entry-with-no-subsequent-registration (load-bearing read-path eviction), cap-eviction, thread-ID reuse, and parent-end cleanup.opentelemetry-instrumentation-litellm— framework retry emitter_FortifyRootRetryEmitter(_LiteLLMCustomLoggerBase)registered at instrumentor init (litellm.callbacks index 1, after the existing safety logger).log_pre_api_call/async_log_pre_api_call/log_success_event/async_log_success_event/log_failure_event/async_log_failure_event— defensive both-sync-and-async to handle LiteLLM's dispatch quirks (review-round-2 Blocker 4)._FortifyRootRetryEmitterinherits fromlitellm.integrations.custom_logger.CustomLogger(not duck-typed) — LiteLLM's dispatch loop gates every callback hook onisinstance(callback, CustomLogger), so a duck-typed class is silently skipped (verified end-to-end during review-batch-1 2026-05-10).fortifyroot.litellm.retry_attempt;litellm_call_idis the correlation key; bounded-size correlation map mirrors the registry's eviction policy._resolve_routed_provider()normalises LiteLLM'scustom_llm_providerto RETRY_LOOP.md §4.2 canonical form (e.g.bedrock/bedrock_converse/amazon/aws→"AWS";vertex_ai/gemini→"google"); special-case for bareclaude-*model strings →"anthropic"(review-batch-1 Minor 4).+543 LOCin package code; companion test file+565 LOC.opentelemetry-instrumentation-langchain— framework retry emitter_FortifyRootRetryHandler(BaseCallbackHandler)— hooks BOTHon_chat_model_start(F1 finding from ST-10.0 C2 POC: chat models fire ONLY this hook, NOTon_llm_start) ANDon_llm_start(legacy completion LLMs)._resolve_parent_span()uses a two-strategy approach to find the workflow parent across multi-attempt retries:_FortifyRootRetryHandlerconstruction time (NOT via mutable shared back-reference — review-round-2 Major-5 fix);parent_run_idkeys intotraceloop_handler.spansto recover the workflow SpanHolder.parent_run_id is None(legitimate root) OR no Traceloop handler is wired (standalone/test).parent_run_idis set + Traceloop handler is provided + lookup fails, refuse to fall back to ambient (which would be Traceloop's per-LLM span and would break sibling-grouping). Return None; emission skipped.callback_handler.py:SpanHolder.tokenslist + LIFO detach in_end_spanfixes the OTel-context-token leak that was bleeding ambient context across pytest tests (review-round-2 Blocker 1).+95 LOCto__init__.pyfor instrumentor wiring;+65 / -11 LOCtocallback_handler.py; newretry_handler.py+653 LOC; companion test file+638 LOC.opentelemetry-instrumentation-llamaindex— framework retry emitter_FortifyRootRetryHandler(BaseSpanHandler)registered on LlamaIndex's dispatcher BEFOREOpenLLMetrySpanHandlerso the retry handler sees the user's enclosing workflow span (not the per-call Traceloop span) for sibling-grouping.new_span/prepare_to_exit_span/prepare_to_drop_span.chat()AND inner_chat()methods, which would emit 2× retry_attempts per HTTP attempt without de-dup. Filtered via_OUTER_LLM_METHODSwhitelist (chat/achat/stream_chat/astream_chat/complete/acomplete/stream_complete/astream_complete/predict/apredict/structured_predict/etc.) — inner methods with leading_are rejected.dispatcher_wrapper.py +15 LOCfor instrumentor wiring; newretry_handler.py+488 LOC; companion test file+487 LOC.opentelemetry-instrumentation-openai— direct-SDK retry emitterretry_handler.py(+691 LOC) wrappingopenai._base_client.SyncHttpxClientWrapper.send+AsyncHttpxClientWrapper.send. Fires per HTTP attempt inside the SDK's internal retry loop (no global httpx monkey-patch)./chat/completions,/completions,/embeddings,/responses,/messages) so non-LLM SDK traffic (e.g./v1/models) doesn't emit retry_attempt.SUPPRESS_LANGUAGE_MODEL_INSTRUMENTATION_KEY(contextvar) ANDis_framework_owned()(TID registry); if EITHER signals owned-by-framework, emit is skipped.OPENAI_DIRECT_RETRY_PARENT_ACTIVE_KEYoverride key lets OpenAI's ownchat_wrapperkeep emitting retry_attempt under its own logical span (it setsSUPPRESS_KEY=Trueto prevent OTHER instrumentors from double-counting, but is itself the openai instrumentor and wants retry_attempt to emit)._base_clientdoesn't crash instrumentation init — it just logs and skips retry-attempt emission for that variant.stream=Trueinsend()kwargs): retry_attempt emission disabled because the wrapper can't consume the SSE body for usage extraction without breaking the SDK; deferred underST-10.4-FOLLOWUP-streaming-usage.chat_wrappers.py +26 / -10 LOCto set the override key +instrument_retry_emitter/uninstrument_retry_emitterlifecycle inv1/__init__.py +16 LOC.+936 LOCcovering 4-failure-shape verification (429→200, timeout→200, 5xx→200, all-fail) per RETRY_LOOP.md §4.4.1 + framework-owned suppression + private-symbol guard + idempotent install/uninstall._OpenAITracingWrapper.__call__finally-block (review-round-2 Blocker 3) ensures the suppression token is released even on exception.opentelemetry-instrumentation-anthropic— direct-SDK retry emitteranthropic._base_client.SyncHttpxClientWrapper.send+AsyncHttpxClientWrapper.send.gen_ai.system="Anthropic"(title case —proc_retry_detector.gonormalises to lowercase)._wrap/_awrapuseswith trace.use_span(span, end_on_exit=False)— important not to undo when consolidating with OpenAI shared code.+558 LOCnewretry_handler.py;+651 LOCcompanion tests.opentelemetry-instrumentation-bedrock— direct-SDK retry emitterbefore-send.bedrock-runtime.*(attempt-start) +response-received.bedrock-runtime.*(attempt-end) on the boto3 client._response_received_hooknow accepts BOTH oldhttp_response=/parsed=and newresponse_dict=/parsed_response=param names (botocore 1.42.x renamed). Necessary but not sufficient — the correlation-mechanism rework is tracked separately asST-10.4-FOLLOWUP-bedrock-context-correlation(current botocore doesn't passcontext=kwarg to before-send andrequest.contextis None at fire time, so the existingrequest.context[_CTX_SPAN_KEY]correlation never creates a retry_attempt span end-to-end). The unit tests intest_retry_attempt_emission.pystill pass because they invoke hooks directly with manually-preparedrequest.context = {}.+542 LOCnewretry_handler.py;+569 LOCcompanion tests.Cross-cutting
traceloop-sdk/tests/test_*.py(5 files): filterfortifyroot.*retry-attempt spans from upstream/non-FR exact-span assertions (role-based filter, not name-prefix filter — robust against future fortifyroot-prefixed spans being added).openai/tests/traces/test_*.py(4 files): same upstream-test hardening.Review history captured in the diff
Five rounds of review fed back into this PR:
CustomLogger;_TOKEN_TO_TIDreverse index needed so cross-thread terminal callbacks unregister against the originating TID.parent_run_idset + Traceloop lookup fails;_OpenAITracingWrapper.__call__finally-block detach;async_log_pre_api_calldefensive hook.set[int]); in-band stale-eviction on theis_framework_owned()read path so orphans are bounded by TTL even with no subsequent registration on that TID.fallback_b_child_emission_proofevidence requirement in the hook-table.Plus a 2026-05-18 botocore 1.42.x compat fix (Bedrock
response-receivedparam-name reconciliation) that landed late.Test plan
nx run-many -t testacross all 7 modified packages — passes locally.nx run opentelemetry-instrumentation-fortifyroot:test— covers registry re-entrancy / stale-eviction / cap-eviction / thread-ID reuse / parent-end cleanup.nx run opentelemetry-instrumentation-{litellm,langchain,llamaindex,openai,anthropic,bedrock}:test— covers per-package retry_attempt emission against the 4 failure shapes (429/timeout/5xx/all-fail).nx run opentelemetry-instrumentation-langchain:test::test_no_leaked_ambient_context_after_simulated_workflow— end-to-end regression guard for the SpanHolder.tokens LIFO detach fix (review-round-2 Blocker 1).fr-system-tests(companion PR fix: Disable image uploader for non-tl endpoints #19): direct-SDK matrix (ST-10.6) PASSES on dev-api fixture mode6 passed + 1 xfail Bedrock; framework matrix (ST-10.7) is scaffolded but skipped pending deferred follow-ups.