fix(logging): classify async anthropic_messages and generate_content as async - #33589
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR fixes duplicate success-event logging for async
Confidence Score: 4/5Safe to merge; the change is narrowly scoped to async-marker planting and the classifier check, with no effect on sync request paths or the core logging dispatch logic. The production code changes are minimal and follow a well-established pattern already used by tests/test_litellm/litellm_core_utils/test_litellm_logging.py — the sleep-based synchronization in
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/litellm_logging.py | Adds three new async markers (aanthropic_messages, agenerate_content, agenerate_content_stream) to _is_sync_litellm_request using the existing is not True guard pattern — straightforward and correct. |
| litellm/llms/anthropic/experimental_pass_through/messages/handler.py | Plants aanthropic_messages marker in litellm_params before the mock-response short-circuit and before any success handler runs; guarded by if litellm_logging_obj is not None, consistent with the existing agentic_loop_params pattern. |
| litellm/google_genai/main.py | Introduces _mark_async_entrypoint helper and calls it in generate_content, agenerate_content_stream, and generate_content_stream before mock-response or network paths; the @client decorator already guarantees litellm_logging_obj is in kwargs when the function body runs, so the helper is always a no-op only for None-logger cases. |
| litellm/types/utils.py | Adds aanthropic_messages to both CallTypes enum and CallTypesLiteral; straightforward enum extension. |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Good regression coverage for the three new markers; test_anthropic_messages_marks_litellm_params_async uses a bare asyncio.sleep(1) to wait for the async logger — potentially flaky on slow CI and raises KeyError (not an assertion failure) if captured is still empty when the assertion runs. |
| ui/litellm-dashboard/src/lib/http/schema.d.ts | Regenerated schema adds aanthropic_messages to the CallTypes union literal; machine-generated change, no issues. |
Reviews (1): Last reviewed commit: "fix(logging): classify async anthropic_m..." | Re-trigger Greptile
|
|
||
| @pytest.mark.asyncio | ||
| async def test_agenerate_content_marks_litellm_params_async(): | ||
| """LIT-4475: the async ``agenerate_content`` entrypoint must plant | ||
| ``agenerate_content`` in ``litellm_params`` so ``_is_sync_litellm_request`` |
There was a problem hiding this comment.
Unreliable sleep-based synchronization
await asyncio.sleep(1) is a fragile way to wait for the async logger callback to populate captured. If async_log_success_event hasn't executed by the time the sleep returns (e.g., on a loaded CI runner), captured["litellm_params"] raises a KeyError rather than a descriptive assertion failure, making the failure hard to diagnose. Consider replacing with an asyncio.Event set inside async_log_success_event, or at minimum guard the final assertions with assert "litellm_params" in captured, "async logger never fired".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Live verification of the merged #33604 against real DataDog (us5) exposed three read-back defects that the local-sink tests could never see; all three fixes are verified against the real API: - Marker search: DataDog consumes the shipped JSON message into the event's attributes and leaves the indexed message EMPTY, so the full-text '"marker"' query matched nothing and every test failed with zero events. The query is now '*:*marker*', which scans all attributes (the marker sits in messages.content); verified to return exactly the event for the call. - Rate limit: the Logs Search API budget is 2 requests per 10s org-wide (x-ratelimit-name logs_public_search_api). Polling at POLL_INTERVAL=5s sat exactly at the limit and the reader hard-failed on the first 429. Searches now pace at DD_SEARCH_INTERVAL (10s default) and a 429 backs off and retries up to 5 times; only non-429 failures stay hard fails. - Envelope status: DataDog re-derives the indexed event status from the parsed payload's status attribute ('success') and normalizes it to its OK severity, so the assertion expects 'ok', not the shipped 'info'. Live run: chat_completions and responses pass every assertion including the exact response-cost cross-check; messages red-pins the LIT-4447 duplicate for real (one call -> two sync-sweep copies + one async batch copy, same request id, confirmed in proxy debug logs). The duplicate is race-dependent, so the pin flickers until #33589 lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…as async Async anthropic_messages, agenerate_content, and agenerate_content_stream did not plant an async marker in litellm_params, so _is_sync_litellm_request classified them as sync. The async success path then dispatched both the async and sync CustomLogger hooks, producing duplicate success events (LIT-4447, LIT-4475). Follow the same kwargs-marker convention used by /chat/completions (acompletion) and /responses (aresponses): the async entrypoints now record their call type in litellm_params and _is_sync_litellm_request recognizes aanthropic_messages, agenerate_content, and agenerate_content_stream.
…trization The Azure initialize_azure_sdk_client parametrization iterates every async CallTypes member and excludes non-Azure routes such as anthropic_messages, agenerate_content, and agenerate_content_stream. The new aanthropic_messages marker is likewise not an Azure route, so add it to the exclusion list.
54b7981 to
5a97264
Compare
…he real datadog api (#33566) * fix(e2e): make the datadog read-back find what DataDog actually indexes Live verification of the merged #33604 against real DataDog (us5) exposed three read-back defects that the local-sink tests could never see; all three fixes are verified against the real API: - Marker search: DataDog consumes the shipped JSON message into the event's attributes and leaves the indexed message EMPTY, so the full-text '"marker"' query matched nothing and every test failed with zero events. The query is now '*:*marker*', which scans all attributes (the marker sits in messages.content); verified to return exactly the event for the call. - Rate limit: the Logs Search API budget is 2 requests per 10s org-wide (x-ratelimit-name logs_public_search_api). Polling at POLL_INTERVAL=5s sat exactly at the limit and the reader hard-failed on the first 429. Searches now pace at DD_SEARCH_INTERVAL (10s default) and a 429 backs off and retries up to 5 times; only non-429 failures stay hard fails. - Envelope status: DataDog re-derives the indexed event status from the parsed payload's status attribute ('success') and normalizes it to its OK severity, so the assertion expects 'ok', not the shipped 'info'. Live run: chat_completions and responses pass every assertion including the exact response-cost cross-check; messages red-pins the LIT-4447 duplicate for real (one call -> two sync-sweep copies + one async batch copy, same request id, confirmed in proxy debug logs). The duplicate is race-dependent, so the pin flickers until #33589 lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(e2e): datadog log delivery for streamed chat, messages, and responses Rewritten from the dd-sink version (original #33566) to judge delivery on what real DataDog ingested, matching the merged #33604 conversion: the dd_logs reader searches events back through the Logs Search API and the assertions validate the indexed envelope (source:litellm tag, ok status) and the StandardLoggingPayload fields under the event's attributes. Each streamed test drives one STREAMED call per route, asserts the stream actually streamed (event-stream content type, >0 chunks, no upstream error event), then pins exactly one DataDog event whose payload records stream=true, the aggregated token count, and a response_cost equal to the /spend/logs row for the call - a stream's headers ship before its cost exists, so the spend row is the cross-check anchor, and the spend row and DataDog event must also agree on total_tokens. Coverage registry: adds logging.datadog.stream.exports_metric exercised on chat_completions, messages, and responses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Update test_datadog_log_e2e.py --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the fixed asyncio.sleep(1) in test_anthropic_messages_marks_litellm_params_async with an asyncio.Event set inside the async logger and awaited via asyncio.wait_for, so the assertion runs the instant the callback fires and a missed callback surfaces as a clear TimeoutError instead of a KeyError.
dfb3ee4 to
fbe8f7e
Compare
9cae6fa
into
litellm_internal_staging
Relevant issues
Linear ticket
Resolves LIT-4447
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Live proxy proof against real provider APIs is pending; can be captured on request. The regression coverage below fails on the current base and passes with this change.
Type
🐛 Bug Fix
Changes
Async requests can invoke both
CustomLogger.async_log_success_eventand, via the sync compatibility sweep,CustomLogger.log_success_event. Whether the sweep runs the sync hook is decided by_is_sync_litellm_request, which infers sync vs async from async markers inlitellm_params(acompletion,aresponses,aembedding,aimage_generation,atranscription,allm_passthrough_route).anthropic_messagesplanted no such marker, so an async/v1/messagescall was classified sync and the sweep fired the sync CustomLogger hook in addition to the async one, producing duplicate success events (LIT-4447).agenerate_contentandagenerate_content_streamset a kwarg flag but it was never recognized by the classifier, the same class of bug (LIT-4475).This follows the existing kwargs-marker convention that
/chat/completionsand/responsesalready use. In pseudocode:The marker is recorded before the mock short-circuit and before any success or failure handler runs, and it reflects the real
is_asyncvalue so the sync entrypoints stay classified sync.aanthropic_messagesis added toCallTypes/CallTypesLiteral(and the regenerated dashboardschema.d.ts).Files:
litellm/litellm_core_utils/litellm_logging.py:_is_sync_litellm_requestrecognizesaanthropic_messages,agenerate_content,agenerate_content_streamlitellm/llms/anthropic/experimental_pass_through/messages/handler.py: plantaanthropic_messagesinlitellm_paramslitellm/google_genai/main.py: plantagenerate_content/agenerate_content_streammarkers via a small helperlitellm/types/utils.py: addaanthropic_messagesenum + literaltests/test_litellm/litellm_core_utils/test_litellm_logging.py: regression coveragetests/test_litellm/llms/azure/test_azure_common_utils.py: exclude the newaanthropic_messagesmarker from the Azure SDK client parametrization, consistent with howanthropic_messages/agenerate_contentare already excluded (they are not Azure routes)This supersedes the sweep-flag mechanism merged in #33577 by moving the async classification back onto the request markers, consistent with how the other endpoints work.
How it was validated
tests/test_litellm/litellm_core_utils/test_litellm_logging.pypasses (121 tests). New/extended coverage:test_is_sync_litellm_requestasserts the three new markers classify async and thataanthropic_messages: Falsestays synctest_success_handler_skips_sync_callbacks_for_async_requestsis parametrized over the new markers to prove the sync CustomLogger hook is skippedtest_anthropic_messages_marks_litellm_params_asyncdrives the reallitellm.anthropic_messagesentrypoint and asserts the marker lands inlitellm_params, the request classifies async, and the sync hook is not calledtest_agenerate_content_marks_litellm_params_asyncdriveslitellm.agenerate_contentand asserts the marker lands inlitellm_paramsand classifies asyncFinal Attestation
Link to Devin session: https://app.devin.ai/sessions/f79a6ba850c74a1c8d2bfe3f9aa3665c
Requested by: @yucheng-berri