Repository navigation
refactor(llmobs): abstract llmobs from llama_index integration code - #20860
gh-worker-dd-mergequeue-cf854d[bot] merged 35 commits into
Conversation
base_stream_handler.py has no LLM Observability dependencies, but it lived under ddtrace/llmobs/_integrations, so every contrib that traces streamed responses (and AI Guard) imported from the LLMObs product. Move it to ddtrace/contrib/internal/stream_handler.py and update all importers. The unit tests are renamed to tests/llmobs/test_stream_handler.py, and the new path is added to the llmobs suitespec component so changes to it keep triggering the LLM integration suites. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Applies the same inversion the Anthropic integration received to LlamaIndex, so the contrib no longer imports LLMObs. The contrib previously constructed a LlamaIndexIntegration in patch(), stashed it on llama_index.core._datadog_integration, and hung it off every LlmRequestEvent, which meant ddtrace/contrib/internal/llama_index imported ddtrace.llmobs. Now the contrib dispatches the shared LlmEvents span lifecycle and LLMObs attaches its own subscribers on the llama_index.patch core event, mirroring ddtrace/llmobs/_contrib/anthropic. - patch(): drop the integration instance and _get_integration(); take integration_config/service from config.llama_index; own the llama_index.request.model and llama_index.request.provider APM tags via event.tags so they are set whether or not LLMObs is present; dispatch llama_index.patch / llama_index.unpatch. - _streaming.py: drop the integration parameter; the stream handlers only need the context to dispatch the deferred ended event. - LlamaIndexIntegration._set_base_span_tags is now a no-op: the contrib owns the APM tags and LlamaIndex has no per-request base_url to record. - The LLMObs subscribers register off the patch event, with the already-patched fallback keyed on llama_index.core rather than the top-level package. The dependency direction detector drops from 183 to 181 violations (both contrib.internal.llama_index -> llmobs._integrations edges removed, no new violations, no uncovered modules). Testing: llmobs::llama_index (61 passed) and llmobs::anthropic (108 passed) on py3.14; llmobs::llmobs matches the branch baseline exactly (1563 passed, same 11 pre-existing RC/sampling failures with and without this change). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis📈 Existing violations got worse1 pre-existing violation(s) increased in severity (e.g. their target became more depended-on, or got pulled into an import cycle), though the edge itself isn't new:
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
emmettbutler
left a comment
There was a problem hiding this comment.
Pretty much matches what we discussed. Nice work.
fix in-flight requests after unpatch()
0d51d5d to
90733a6
Compare
dubloom
left a comment
There was a problem hiding this comment.
I'll make an experiment but I still doubt a bit the usefullned of the mlobs specific span lifecycle events. Like maybe we can leverage the ones that were pre-existing
Yun-Kim
left a comment
There was a problem hiding this comment.
small nits but otherwise lgtm
wconti27
left a comment
There was a problem hiding this comment.
PR definitely moving in the right direction. Sorry for all the repeated "can we centralize" but id love to strip all those small things someone has to do when writing an integration, and only require things that are truly 1 off per integration
|
Deferring remaining comments to follow up #21333 for broader subscriber refactor (shared lifecycle handling, centralized product registration and patch notifications, and removal of per-integration LLMObs subscriber helpers). |
Description
Stacked on PR #20760 which introduced the
LlmEventsspan-lifecycle events and first LLMObs subscriber.Changes:
Decouple llama-index instrumentation from LLMObs by moving integration handling into shared lifecycle subscribers.
LlmRequestEventinstances and owns APM model/provider tags without importing LLMObs.ddtrace.integration_hooks, including when patched manually.Testing
Existing
llama_indexandllmobssuites are the behavioral contract and pass unchanged.Risks
Additional Notes