Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Claude finished @olesho's task —— View job Code Review: Added tracing metadata for LiteLLM providerTodo List:
SummaryThis PR implements tracing metadata support for Langfuse integration through LiteLLM provider. The changes create a metadata flow from the evaluation framework through various application layers to enable LLM call tracking and observability. Code Quality & Best Practices ✅Strengths:
Architecture Assessment ✅The implementation follows a clean layered approach:
Potential Issues & Recommendations🔴 API Key Security Concern// agent-server/nodejs/src/api-server.js:646-650
if (!apiKey && tierConfig.provider === 'litellm') {
apiKey = process.env.LITELLM_API_KEY;
}Issue: Fallback to environment variable could be logged inadvertently. 🟡 Type Safety Improvements
🟡 Error Handling
Performance Considerations ✅
Security Assessment
|
📝 WalkthroughWalkthroughThis change adds comprehensive tracing metadata support across the agent system. Tracing metadata is extracted from requests, propagated through the LLM client, agent framework, and evaluation system, and finally attached to provider payloads for Langfuse integration. The backend API server is extended to support tracing metadata in nested model configurations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
agent-server/nodejs/src/lib/BrowserAgentServer.js (1)
730-745: Tracing metadata forwarding into evaluation RPC looks good (optionally omit empty objects)Forwarding
request.tracing || {}into the JSON‑RPCparamsand logging only keys/session_id is a clean way to propagate Langfuse metadata without leaking payload contents. If you ever care about keeping the wire payload minimal, you could conditionally includetracingonly whenrequest.tracingis defined, but it’s not required for correctness.front_end/panels/ai_chat/tools/LLMTracingWrapper.ts (1)
88-91: Ensure tracingContext metadata wins over caller-supplied optionsRight now you set
tracingMetadata: tracingContext?.metadataand then spreadllmCallConfig.options. If a caller ever passesoptions.tracingMetadata, it will override the context‑derived metadata, which is the opposite of what a tracing wrapper is usually expected to do.I’d flip the order so context metadata takes precedence (while still allowing options to provide it when there’s no tracing context):
Proposed diff
- const llmResponse = await llm.call({ - provider: llmCallConfig.provider, - model: llmCallConfig.model, - messages: llmCallConfig.messages, - systemPrompt: llmCallConfig.systemPrompt || '', - temperature: llmCallConfig.temperature, - // Pass tracing metadata explicitly for Langfuse integration - tracingMetadata: tracingContext?.metadata, - ...llmCallConfig.options - }); + const llmResponse = await llm.call({ + provider: llmCallConfig.provider, + model: llmCallConfig.model, + messages: llmCallConfig.messages, + systemPrompt: llmCallConfig.systemPrompt || '', + temperature: llmCallConfig.temperature, + ...llmCallConfig.options, + // Pass tracing metadata explicitly for Langfuse integration + tracingMetadata: tracingContext?.metadata ?? llmCallConfig.options?.tracingMetadata, + });front_end/panels/ai_chat/LLM/LLMTypes.ts (1)
204-212: LLMCallOptions.tracingMetadata matches usage; consider sharing a common metadata typeAdding
tracingMetadatahere lines up with how callers passtracingContext.metadatathrough to providers, and the index signature keeps it future‑proof for extra Langfuse fields.To avoid type drift between this and
TracingContext.metadata, you might eventually want a sharedTracingMetadatatype exported from the tracing module and reused here, but that’s a nicety rather than a blocker.front_end/panels/ai_chat/LLM/LLMClient.ts (1)
212-242: Consider reducing logging verbosity for production.The tracing metadata resolution logic is correct and properly prioritizes explicit request metadata over global context. However, the extensive logging (10+ log statements) may be verbose for production environments. Consider:
- Moving some logs to debug level
- Consolidating related logs into single statements
- Removing logs that expose internal implementation details in production
The logic itself is sound and backward-compatible.
🔎 Example consolidation
- // Get tracing metadata - prefer explicit request metadata over global context - // This ensures metadata flows correctly even when async context is lost let tracingMetadata = request.tracingMetadata; if (!tracingMetadata || Object.keys(tracingMetadata).length === 0) { - // Fall back to global tracing context const tracingContext = getCurrentTracingContext(); - logger.info('LLMClient.call() - Checking tracing context (fallback):', { - hasContext: !!tracingContext, - hasMetadata: !!tracingContext?.metadata, - metadataKeys: tracingContext?.metadata ? Object.keys(tracingContext.metadata) : [], - sessionId: tracingContext?.metadata?.session_id, - traceId: tracingContext?.metadata?.trace_id - }); + logger.debug('Falling back to global tracing context:', { + sessionId: tracingContext?.metadata?.session_id, + traceId: tracingContext?.metadata?.trace_id + }); if (tracingContext?.metadata && Object.keys(tracingContext.metadata).length > 0) { tracingMetadata = tracingContext.metadata; } - } else { - logger.info('LLMClient.call() - Using explicit tracingMetadata from request:', { - metadataKeys: Object.keys(tracingMetadata), - sessionId: tracingMetadata.session_id, - traceId: tracingMetadata.trace_id - }); } if (tracingMetadata && Object.keys(tracingMetadata).length > 0) { options.tracingMetadata = tracingMetadata; - logger.info('Passing tracing metadata to provider:', tracingMetadata); - } else { - logger.info('No tracing metadata available'); + logger.debug('Attached tracing metadata to provider options'); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
agent-server/nodejs/src/api-server.jsagent-server/nodejs/src/lib/BrowserAgentServer.jsfront_end/panels/ai_chat/LLM/LLMClient.tsfront_end/panels/ai_chat/LLM/LLMTypes.tsfront_end/panels/ai_chat/LLM/LiteLLMProvider.tsfront_end/panels/ai_chat/agent_framework/AgentRunner.tsfront_end/panels/ai_chat/core/AgentNodes.tsfront_end/panels/ai_chat/core/AgentService.tsfront_end/panels/ai_chat/evaluation/EvaluationAgent.tsfront_end/panels/ai_chat/evaluation/EvaluationProtocol.tsfront_end/panels/ai_chat/evaluation/remote/EvaluationAgent.tsfront_end/panels/ai_chat/evaluation/remote/EvaluationProtocol.tsfront_end/panels/ai_chat/tools/LLMTracingWrapper.tsfront_end/panels/ai_chat/tracing/TracingProvider.ts
🧰 Additional context used
📓 Path-based instructions (1)
agent-server/nodejs/src/api-server.js
📄 CodeRabbit inference engine (agent-server/nodejs/CLAUDE.md)
agent-server/nodejs/src/api-server.js: Expose REST endpoint POST /v1/responses that accepts task input, URL, timeout, and model configuration, and returns OpenAI-compatible response with metadata
Use formatResponse() method to convert agent responses to OpenAI-compatible format and include metadata with clientId and tabId for screenshot capture
Model configuration must use canonical nested format with main_model, mini_model, and nano_model tiers, each containing provider, model, and api_key fields
POST /page/screenshot endpoint must accept clientId and tabId, use CDP Page.captureScreenshot, and return base64-encoded PNG with metadata and timestamp
POST /page/content endpoint must accept clientId, tabId, format (html or text), and includeIframes parameters; recursively capture iframe content when includeIframes is true
POST /page/execute endpoint must accept clientId, tabId, expression, returnByValue, and awaitPromise; use CDP Runtime.evaluate and return result with type and value
Accept POST /v1/responses input as either string format (simple message) or conversation array format with role and content fields; enforce at least one user message and max 100 messages/10,000 characters per message
Files:
agent-server/nodejs/src/api-server.js
🧠 Learnings (6)
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/rpc-client.js : Implement JSON-RPC 2.0 protocol for bidirectional communication with request/response correlation using unique IDs, timeout handling, and error conditions
Applied to files:
agent-server/nodejs/src/lib/BrowserAgentServer.js
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Implement WebSocket server for browser agent connections with client lifecycle management (connect, ready, disconnect)
Applied to files:
agent-server/nodejs/src/lib/BrowserAgentServer.js
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/api-server.js : Use formatResponse() method to convert agent responses to OpenAI-compatible format and include metadata with clientId and tabId for screenshot capture
Applied to files:
agent-server/nodejs/src/lib/BrowserAgentServer.js
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget
Applied to files:
agent-server/nodejs/src/lib/BrowserAgentServer.jsfront_end/panels/ai_chat/evaluation/remote/EvaluationAgent.tsfront_end/panels/ai_chat/evaluation/EvaluationAgent.ts
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/api-server.js : Model configuration must use canonical nested format with main_model, mini_model, and nano_model tiers, each containing provider, model, and api_key fields
Applied to files:
agent-server/nodejs/src/api-server.js
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/api-server.js : POST /page/execute endpoint must accept clientId, tabId, expression, returnByValue, and awaitPromise; use CDP Runtime.evaluate and return result with type and value
Applied to files:
front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.tsfront_end/panels/ai_chat/evaluation/EvaluationAgent.ts
🧬 Code graph analysis (4)
agent-server/nodejs/src/api-server.js (2)
front_end/panels/ai_chat/LLM/LiteLLMProvider.ts (1)
getEndpoint(48-64)front_end/panels/ai_chat/core/LLMConfigurationManager.ts (1)
getEndpoint(222-229)
front_end/panels/ai_chat/core/AgentNodes.ts (1)
front_end/panels/ai_chat/ui/message/MessageList.ts (2)
state(23-23)state(24-24)
front_end/panels/ai_chat/LLM/LLMClient.ts (1)
front_end/panels/ai_chat/tracing/TracingConfig.ts (1)
getCurrentTracingContext(299-301)
front_end/panels/ai_chat/evaluation/EvaluationAgent.ts (1)
front_end/panels/ai_chat/tracing/TracingProvider.ts (1)
TracingContext(8-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (13)
front_end/panels/ai_chat/core/AgentService.ts (2)
279-288: AUTOMATED_MODE apiKey defaulting looks correct; verify provider behavior with empty stringAllowing the default branch to proceed without an apiKey when
BUILD_CONFIG.AUTOMATED_MODEis true and normalizing toapiKey: apiKey || ''matches the “keys come from request body” design. Just make sure all providers (andLLMClient) treat''as “no key” and that dynamic per-request keys always override this config value so you don’t end up sending bad auth headers in automated runs.
600-603: Good: evaluation tracing metadata now carried into stategraph tracingContextForwarding
existingContext?.metadataintostate.context.tracingContext.metadatais consistent with the new tracing model and lets downstream nodes/LLM calls attach Langfuse grouping metadata that originated in the evaluation flow. No issues spotted here.front_end/panels/ai_chat/evaluation/EvaluationProtocol.ts (1)
94-103: Tracing envelope on EvaluationParams is well-shaped and backward compatibleAdding the optional
tracingfield onEvaluationParamskeeps existing JSON‑RPC clients valid while providing a strongly‑typed spot for Langfuse/LiteLLM metadata (session/trace/eval IDs, tags, plus arbitrary extras). This aligns cleanly with how the rest of the PR consumesparams.tracing.front_end/panels/ai_chat/evaluation/EvaluationAgent.ts (1)
324-341: Nice reuse of incoming tracing plus safe fallbacks for eval runsUsing
params.tracingwhen present and falling back toeval-*/eval-session-*IDs gives stable trace IDs for evaluations while still honoring upstream Langfuse metadata. Attaching the fullrequestTracingobject totracingContext.metadatakeeps the context rich without changing existing behavior whentracingis absent.front_end/panels/ai_chat/agent_framework/AgentRunner.ts (1)
744-754: AgentRunner LLM calls now correctly emit tracing metadataPassing
tracingMetadata: tracingContext?.metadataalongside the AgentRunner LLM call cleanly aligns agent generations with the active trace/eval context. As long asLLMClient.callforwards this field into provider options (which your other PR changes cover), this should plug AgentRunner neatly into the Langfuse pipeline.front_end/panels/ai_chat/core/AgentNodes.ts (1)
243-244: Stategraph LLM calls now participate in the shared trace contextPlumbing
tracingMetadata: state.context?.tracingContext?.metadatainto the AgentNode LLM call cleanly ties stategraph generations back to whatever tracing context AgentService set up (including eval‑origin metadata), without impacting non‑traced runs.front_end/panels/ai_chat/LLM/LiteLLMProvider.ts (1)
212-216: LGTM! Clean tracing metadata integration.The optional propagation of tracing metadata to the LiteLLM payload is well-implemented. The conditional check ensures backward compatibility, and the comment clearly documents the Langfuse integration purpose.
front_end/panels/ai_chat/tracing/TracingProvider.ts (1)
21-30: LGTM! Well-structured tracing metadata support.The optional
metadatafield is cleanly integrated intoTracingContextwith appropriate typing for Langfuse fields and an index signature for extensibility. The change is backward-compatible.front_end/panels/ai_chat/evaluation/remote/EvaluationAgent.ts (1)
429-448: LGTM! Proper tracing metadata extraction and propagation.The code correctly extracts tracing metadata from request parameters with safe fallbacks, derives trace/session IDs with appropriate priority, and constructs the
TracingContextwith complete metadata for downstream propagation. The logging provides good visibility into the tracing flow.front_end/panels/ai_chat/evaluation/remote/EvaluationProtocol.ts (1)
107-116: LGTM! Protocol extension for tracing metadata.The optional
tracingfield cleanly extends theEvaluationParamsinterface to support Langfuse integration. The structure aligns with theTracingContext.metadataschema and maintains backward compatibility.agent-server/nodejs/src/api-server.js (3)
645-653: LGTM! Safe API key fallback for LiteLLM provider.The conditional fallback to
LITELLM_API_KEYenvironment variable is well-scoped to only thelitellmprovider and doesn't affect other providers' API key handling.
767-813: LGTM! Tracing metadata properly threaded through request creation.The function signature update (lines 767-770) and the attachment of tracing metadata to the evaluation request object (lines 810-813) are well-implemented with:
- Clear JSDoc documentation
- Safe default parameter value
- Descriptive comments explaining the Langfuse integration purpose
The tracing metadata will now flow correctly from the API request through to the evaluation execution.
592-616: Verify endpoint fallback behavior for mixed-provider configurations.The
getEndpointhelper at line 597 usestierConfig?.endpoint || requestBody.model.endpoint, which means a top-levelmodel.endpointwill be used for all tiers before checking for the LiteLLM env var fallback. Since each tier can specify a differentprovider, a scenario like this is possible:{ model: { endpoint: "http://openai-proxy", main_model: { provider: "litellm", model: "qwen" } } }The litellm tier would use
"http://openai-proxy"instead ofLITELLM_ENDPOINT. Is this intentional, or should each provider use its own endpoint? If intentional, consider documenting that top-level endpoint applies to all tiers unless overridden at the tier level.
| // Extract tracing metadata for Langfuse integration | ||
| const tracingMetadata = requestBody.metadata || {}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for frontend protocol definition
echo "=== Frontend EvaluationProtocol.ts (lines around 107-116) ==="
if [ -f "front_end/panels/ai_chat/evaluation/remote/EvaluationProtocol.ts" ]; then
sed -n '100,130p' "front_end/panels/ai_chat/evaluation/remote/EvaluationProtocol.ts" | cat -n
else
echo "File not found at that path, searching for it..."
fd "EvaluationProtocol.ts" --exec cat -n {} \; | head -50
fi
echo -e "\n=== Backend api-server.js - search for metadata/tracing field ==="
rg -n "metadata|tracing" agent-server/nodejs/src/api-server.js -A 2 -B 2 | head -100
echo -e "\n=== Backend api-server.js lines 551-560 ==="
if [ -f "agent-server/nodejs/src/api-server.js" ]; then
sed -n '545,560p' "agent-server/nodejs/src/api-server.js" | cat -n
fiRepository: BrowserOperator/browser-operator-core
Length of output: 3960
Extract tracing metadata from requestBody.tracing, not requestBody.metadata.
The frontend protocol (EvaluationProtocol.ts) defines two separate fields:
metadata: tags, retries, priority (lines 103-107)tracing: session_id, trace_id, eval_id, etc. for Langfuse (lines 108-117)
Line 552 incorrectly extracts from requestBody.metadata instead of requestBody.tracing. This causes all tracing metadata (session IDs, trace IDs, etc.) sent by the frontend to be lost. Change line 552 to:
const tracingMetadata = requestBody.tracing || {};🤖 Prompt for AI Agents
In agent-server/nodejs/src/api-server.js around lines 551 to 552, the code
extracts tracing metadata from requestBody.metadata which is incorrect; update
the extraction to read from requestBody.tracing instead (i.e., replace the
metadata reference with tracing) so tracing fields (session_id, trace_id,
eval_id, etc.) sent by the frontend are preserved and used for Langfuse
integration.
1 participant
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.