Conversation
HolmesGPT delegates every LLM call to LiteLLM, but `completion()` never forwarded LiteLLM's reserved `metadata` field. As a result the observability backend behind LiteLLM (Langfuse, Langsmith, Arize, Datadog LLM Obs, a LiteLLM proxy, ...) cannot attribute a trace to the end user or group a conversation into a session — every trace shows up only under the API-key alias. The identity is already available: `/api/chat` puts `user_id`, `conversation_id` and `cluster_name` into `request_context`, and `ChatRequest` carries `user_email` / `request_type`. The only missing link was between `request_context` and the `litellm.completion()` call. This wires that link in a vendor-agnostic way: - `LLM.completion()` / `DefaultLLM.completion()` accept an optional `metadata` dict and forward it to `litellm.completion()`. `metadata` is LiteLLM's reserved logging field: it reaches the configured callbacks / proxy and is stripped before the provider request, so it never reaches the model. Per-call metadata merges over any statically-configured metadata. - `holmes/core/llm_observability.build_llm_metadata()` is the single, narrow place that maps `request_context` to LiteLLM's documented metadata keys (`trace_user_id`, `session_id`, `tags`). It is a whitelist by design: only known identity fields are mapped and tags are bounded, so nothing arbitrary leaks into traces. It returns `None` when there is nothing to attribute, so behaviour is unchanged for callers that carry no identity (e.g. the CLI). - `call_stream()` passes `build_llm_metadata(self._request_context)` to `completion()`. - `server.py` surfaces `user_email` and `request_type` into `request_context` so attribution prefers the email and tags carry the request classification. Because attribution is keyed entirely off `request_context`, every entry point that builds one (chat, scheduled prompts, agui, health checks) benefits without further changes. Default behaviour is unchanged when no identity is present. Complements HolmesGPT#1969 (HolmesUsageEvents), which only feeds the hosted Supabase DAL; this works for any self-hosted LiteLLM observability backend. Signed-off-by: mdecalf <maxime.decalf@ledger.fr>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds LiteLLM observability attribution for server-side LLM calls. A new helper maps ChangesLLM Observability Attribution
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/core/test_llm_completion_metadata.py (1)
82-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest gap (optional): repeated-call persistence not covered.
test_metadata_is_not_passed_twicecallscompletiononce, so it passes even though configuredmetadataispopped and lost on a second call (see thellm.pyfinding). A secondllm.completion(...)on the same instance assertingmetadatais still forwarded would guard the regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/test_llm_completion_metadata.py` around lines 82 - 87, The current test only covers a single completion call, so it misses the regression where configured metadata is removed by the first pop in the LLM instance. Update test_metadata_is_not_passed_twice to call llm.completion twice on the same llm object and assert that mock_completion still receives the configured metadata on the second call, using the _make_llm setup and completion method as the key symbols to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holmes/core/llm.py`:
- Around line 763-772: The metadata handling in completion() is mutating
self.args by popping "metadata", which causes configured metadata to disappear
after the first call. Update completion() to read the configured metadata
non-destructively, merge it with per-call metadata as before, and exclude
"metadata" from the **self.args expansion so it is not passed twice; use the
completion() method and the self.args merge block as the places to adjust.
---
Nitpick comments:
In `@tests/core/test_llm_completion_metadata.py`:
- Around line 82-87: The current test only covers a single completion call, so
it misses the regression where configured metadata is removed by the first pop
in the LLM instance. Update test_metadata_is_not_passed_twice to call
llm.completion twice on the same llm object and assert that mock_completion
still receives the configured metadata on the second call, using the _make_llm
setup and completion method as the key symbols to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 808da2eb-201a-404e-b6d8-462962886865
📒 Files selected for processing (6)
holmes/core/llm.pyholmes/core/llm_observability.pyholmes/core/tool_calling_llm.pyserver.pytests/core/test_llm_completion_metadata.pytests/core/test_llm_observability.py
…w fixes Make end-user attribution provider-neutral and actually effective regardless of how Holmes reaches its model: - `completion()` now forwards the standard `user` field (understood across providers and carried in the request body) in addition to `metadata`. `user` is what reliably reaches a remote LLM endpoint / proxy; `metadata` (session id, tags) is consumed by whichever logging callback is configured. Attribution no longer depends solely on the more vendor-specific `metadata` channel. - `build_llm_metadata()` becomes `build_trace_attribution()`, returning a small `TraceAttribution(user, metadata)` value; the mapping helper is documented in provider-neutral terms (no implementation-specific wording). - `call_stream()` passes both `user` and `metadata` from the request context. Review fix (CodeRabbit): read `user`/`metadata` from `self.args` non-destructively and exclude them from the `**self.args` spread, instead of `pop()`-ing them. A reused `DefaultLLM` previously lost statically-configured `metadata` after the first call (completion() runs once per call_stream iteration). Added a repeated-call regression test plus `user` coverage. Signed-off-by: mdecalf <maxime.decalf@ledger.fr>
Author
|
Thanks @coderabbitai — both addressed in 780d52a:
This revision also generalises the feature to forward the standard provider-neutral |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git fetch origin pull/2250/head:pr2250 2>&1 | tail -5
git checkout pr2250 2>&1 | tail -5
git log --oneline -10
echo "---"
git show 780d52a --stat 2>&1 | head -30
echo "---"
sed -n '1,80p' holmes/core/llm.pyLength of output: 5637 🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
ast-grep run --pattern 'def completion($$$) {
$$$
}' --lang python holmes/core/llm.py 2>/dev/null | head -100
echo "---direct grep---"
rg -n "def completion" -A 60 holmes/core/llm.py | sed -n '1,140p'
echo "---llm_observability---"
cat holmes/core/llm_observability.py
echo "---tool_calling_llm usage---"
rg -n "build_trace_attribution|build_llm_metadata|metadata=|user=" holmes/core/tool_calling_llm.py
echo "---tests---"
sed -n '1,200p' tests/core/test_llm_completion_metadata.pyLength of output: 14851 🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
sed -n '710,800p' holmes/core/llm.pyLength of output: 4954 This confirms the fix in 780d52a is correct and complete:
This looks good to me — nice work, [resolve] ヽ(•‿•)ノ |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@holmes/core/llm_observability.py`:
- Around line 51-64: Hash end-user identifiers before they are assigned in
build_trace_attribution() and forwarded by DefaultLLM.completion(), since
TraceAttribution.user currently carries raw user_email/user_id into
completion(...). Update the attribution path so TraceAttribution.user stores a
stable hashed value instead of the original identifier, while preserving the
existing fallback behavior when no identity is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9542c793-11b7-4d2c-a25e-dad803dd7c1d
📒 Files selected for processing (5)
holmes/core/llm.pyholmes/core/llm_observability.pyholmes/core/tool_calling_llm.pytests/core/test_llm_completion_metadata.pytests/core/test_llm_observability.py
`user` is forwarded all the way to the model provider (DefaultLLM.completion()
passes it straight to litellm.completion()), so sending the raw email/user_id
would leak it to whichever provider serves the request. Hash it instead — the
same guidance OpenAI gives for this field ("we recommend hashing their
username or email to avoid sending us any identifying information").
The hash is a plain deterministic sha256, so a given user still maps to a
single, stable, filterable trace identity in observability backends. Only
`user` needs this: `metadata` (session id, tags) is a logging-only field
consumed by whichever process makes the call, never sent to the provider.
Addresses CodeRabbit review comment on HolmesGPT#2250.
Signed-off-by: mdecalf <maxime.decalf@ledger.fr>
…ribution' into feat/observability-end-user-attribution
Author
|
Thanks @coderabbitai — good catch, fixed in 1938c9a.
Added |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git fetch origin pull/2250/head:pr2250 2>&1 | tail -5
git checkout pr2250 2>&1 | tail -5
git log --oneline -5
echo "---llm_observability---"
cat holmes/core/llm_observability.py
echo "---tests---"
sed -n '1,250p' tests/core/test_llm_observability.pyLength of output: 8178
Nice work — this closes out the observability attribution feature nicely. [resolve] ヽ(´▽`)/ |
This branch has not been deployed
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.
What & why
Closes #2249.
When Holmes runs as a server, LLM calls are not attributed to the end user or the conversation/session in any LLM observability backend. Traces show up only under the API key — no user, no session — so you can't answer "what did user X ask Holmes?" or "show me this conversation" from your observability tool.
This is a provider-neutral gap (same with OpenAI, Anthropic, Bedrock, ...). Holmes already knows the user and the conversation; the identity just never reached the LLM call. Complementary to #1969 (HolmesUsageEvents), which only feeds the hosted Robusta Supabase DAL — this path works for any LLM observability stack (Langfuse, Langsmith, Arize, Datadog LLM Obs, a LiteLLM proxy, ...).
How
Attribution is expressed with provider-neutral fields on the completion call:
user— the standard end-user identifier. It is understood across providers and is carried in the request body, so it reaches a remote model endpoint / proxy and is logged by any observability callback. This is the primary, always-effective attribution field.metadata— optional observability fields (session_id,tags). It is a logging-only field consumed by the configured logging callback; it is never sent to the model. Useful for grouping a conversation into a session and tagging requests.Changes:
LLM.completion()/DefaultLLM.completion()accept optionaluserandmetadataand forward them. Explicit call arguments win over any statically-configured values in the model args (metadatais merged key-by-key,userreplaced). Both are read non-destructively and excluded from the**self.argsspread, so a reusedDefaultLLMkeeps its configured values across calls.holmes/core/llm_observability.build_trace_attribution()(new) is the single, narrow place that mapsrequest_contextto attribution:user←user_email(preferred) oruser_idmetadata.session_id←conversation_idmetadata.tags←request_type:<...>,cluster:<...>It's a whitelist by design — only known identity fields are mapped and tags are bounded — so nothing arbitrary from
request_contextleaks into traces. Returns an emptyTraceAttributionwhen there's nothing to attribute.call_stream()passesuser+metadatafromself._request_context.server.pysurfacesuser_emailandrequest_typeintorequest_context.Because attribution is keyed entirely off
request_context, every entry point that builds one (chat, scheduled prompts, agui, health checks) benefits with no further changes.Backwards compatibility
Default behaviour is unchanged when no identity is present (e.g. the CLI):
userandmetadataareNone, so neither kwarg is sent tocompletion().Tests
tests/core/test_llm_observability.py— the mapping helper (precedence, blanks/None ignored, stringify+strip, tag bounding, empty case).tests/core/test_llm_completion_metadata.py—user/metadataforwarding, default no-op, per-call-over-configured merge, no-double-pass, and repeated-call persistence (configured values survive reuse).(the two new files plus the existing
test_llm_completion_*suite, no regressions)Summary by CodeRabbit
userandmetadatasupport for LLM completions and tool-driven LLM flows.user_emailandrequest_typefor attribution.