feat(logs): Add tool call, agent logs #24
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to cb234b1 in 2 minutes and 16 seconds. Click for details.
- Reviewed
415lines of code in5files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sample-app/generate_joke_workflow_example.go:244
- Draft comment:
Before accessingresp.Choices[0].TextingenerateSignature, consider checking thatresp.Choicesis not empty to avoid a potential panic. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a code quality suggestion about defensive programming. The comment is technically correct - accessingresp.Choices[0]without checking the length could panic. However, looking at the broader context, this same pattern appears consistently throughout the entire file in all four functions that call OpenAI APIs. This suggests it's either: (1) an intentional pattern where the author trusts the OpenAI API contract, or (2) a systematic issue that should be addressed everywhere, not just in one location. The rules state I should only comment on actionable, clear code quality refactors. While this is actionable, the fact that it's only pointing out one instance of a pattern that appears 4 times in the file makes it less useful. Additionally, this is a new file (the entire file is new code), and the author has consistently used this pattern, suggesting they're aware of it and made a deliberate choice. The comment is technically correct and identifies a real potential issue. However, it only addresses one of four identical instances in the file, which makes it inconsistent. If this is truly a problem, all four instances should be flagged. The fact that only one is flagged suggests the tool may have missed the pattern, or this might be considered acceptable in the context of this codebase. While the comment is inconsistent in only flagging one instance, it's still pointing out a valid code quality issue. Even if the pattern is repeated, that doesn't make it correct. However, given the rules emphasize removing comments unless there's STRONG EVIDENCE they're correct and useful, and given that this is a speculative concern (the OpenAI API might always return at least one choice on success), this falls into the category of defensive programming suggestions that may not be necessary. This comment should be deleted. While technically correct, it's a speculative concern about a potential panic that may never occur in practice if the OpenAI API guarantees at least one choice on success. The comment also inconsistently flags only one of four identical patterns in the file. Without strong evidence that this is actually a problem (e.g., documentation showing the API can return empty choices), this is just a defensive programming suggestion that the author may have intentionally chosen not to implement.
2. sample-app/generate_joke_workflow_example.go:130
- Draft comment:
Consider using a structured logging mechanism instead offmt.Printffor warning messages (e.g. when logging the history jokes tool error) to improve consistency and log management. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a code quality suggestion about using structured logging. However, the entire file is a sample/example application (it's in "sample-app/" directory) that consistently uses fmt.Printf throughout for all output. The comment singles out one specific fmt.Printf usage without acknowledging that the entire file uses the same approach. This appears to be a sample/demo application, not production code, where fmt.Printf is commonly acceptable. The comment is also not actionable in a specific way - it says "consider" which is speculative and doesn't clearly require a change. According to the rules, I should not keep comments that are obvious, unimportant, or about code quality unless they are clearly actionable. This seems like a minor style preference for a sample application. This could be valid feedback if this were production code where structured logging is important. The comment does point to a legitimate best practice. However, I might be missing context about whether this sample app is meant to demonstrate best practices or just functionality. Even if this is meant to demonstrate best practices, the comment is inconsistent - it only targets line 130 when the entire file uses fmt.Printf. If structured logging were truly important here, the comment should address the pattern throughout the file, not just one instance. Additionally, the comment uses "Consider" which makes it non-actionable and speculative, violating the rules about clear action items. This comment should be deleted. It's a non-actionable suggestion ("Consider") about code quality in a sample application that consistently uses fmt.Printf throughout. The comment singles out one usage without addressing the broader pattern, making it inconsistent and not clearly actionable.
3. sample-app/generate_joke_workflow_example.go:73
- Draft comment:
Typo: Consider capitalizing 'english' to 'English' in the prompt string "Translate the below joke to pirate-like english:" for consistency and correct grammar. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is about capitalizing a word in a natural language prompt string. While technically correct from a grammar perspective, this is extremely minor and arguably unimportant. The rules explicitly state "Do NOT make comments that are obvious or unimportant." This is a stylistic nitpick about grammar in a prompt string that will be sent to an LLM - the LLM will understand it either way. This doesn't affect functionality, correctness, or code quality in any meaningful way. It's the kind of trivial comment that adds noise to a PR review without providing real value. Could this be important for consistency if there's a style guide that requires proper capitalization in all user-facing strings? Perhaps the team has standards about prompt quality that include proper grammar? Even if there were such standards, this would be an extremely minor issue that doesn't warrant a PR comment. The rules explicitly say not to make obvious or unimportant comments. A single lowercase word in a prompt string falls squarely into the "unimportant" category. There's no evidence of a style guide being violated, and this is a sample app, not production code. This comment should be deleted. It's a trivial grammar nitpick about capitalizing "english" in a natural language prompt string. This falls under "obvious or unimportant" comments that should not be made according to the rules.
4. sample-app/generate_joke_workflow_example.go:146
- Draft comment:
Typo: The prompt "get some history jokes" might be better as "Get some history jokes" to maintain proper capitalization. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is about a very minor stylistic issue - capitalizing the first letter of a prompt sent to an LLM. Looking at other prompts in the file: line 25 has "Tell me a joke about opentelemetry" (capitalized), line 73 has "Translate the below joke to pirate-like english" (capitalized), and line 198 has "add a signature to the joke" (lowercase). So there's already inconsistency in the file. However, this is an extremely minor issue - the capitalization of a prompt string sent to an LLM won't affect functionality at all. The rules state "Do NOT make comments that are obvious or unimportant" and this seems to fall into the "unimportant" category. It's a nitpick about style that doesn't affect code quality or correctness. Could this be part of a broader consistency issue that matters? Perhaps maintaining consistent prompt formatting is important for this codebase's standards. Also, I might be wrong that it's unimportant - maybe there are style guidelines that require this. Even if there are style guidelines, this is such a minor issue that it falls below the threshold of what should be flagged in a code review. The file already has inconsistent capitalization (line 198 also starts lowercase), so this isn't even consistently applied. This is the kind of thing that could be caught by a linter if it really mattered, not a manual code review comment. This comment should be deleted. It's flagging an extremely minor stylistic issue (capitalizing the first letter of a prompt string) that doesn't affect functionality, and the file already has inconsistent capitalization in other prompts. This falls under "obvious or unimportant" comments that should not be made.
Workflow ID: wflow_KOOxLxBZK1u930NL
You can customize 
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request introduces a refactored Traceloop SDK API with new abstractions for agents and tools, updated method signatures that eliminate error returns from logging functions, and two new sample applications demonstrating multi-step workflows and agent-based tool orchestration. Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as Traceloop SDK
participant Agent
participant Tool
participant OpenAI
participant Tracer
Agent->>SDK: NewAgent(name, props)
SDK->>Tracer: Create Agent Span
Tracer-->>SDK: Agent Context
SDK-->>Agent: Agent Instance
Agent->>Agent: LogPrompt(prompt)
Agent->>SDK: LogPrompt(prompt, ContextAttributes)
SDK->>Tracer: Create LLM Span
Tracer-->>SDK: LLMSpan
SDK-->>Agent: LLMSpan
Agent->>Agent: NewTool(name, type, func)
Agent->>Tracer: Create Tool Span
Tracer-->>Agent: Tool Context
Agent-->>Agent: Tool Instance
Tool->>Tool: LogPrompt(prompt)
Tool->>Agent: Merge AssociationProperties
Tool->>SDK: LogPrompt(prompt, merged ContextAttributes)
SDK->>OpenAI: API Call
OpenAI-->>SDK: Response
SDK-->>Tool: LLMSpan
Tool->>Tracer: End Tool Span
Agent->>Tracer: End Agent Span
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (6)
🧰 Additional context used🧬 Code graph analysis (5)traceloop-sdk/agent.go (7)
traceloop-sdk/sdk.go (5)
sample-app/recipe_agent_example.go (5)
traceloop-sdk/tracing_types.go (1)
traceloop-sdk/workflow.go (5)
⏰ 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)
🔇 Additional comments (8)
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: 0
♻️ Duplicate comments (6)
sample-app/generate_joke_workflow_example.go (6)
68-68: Guard against empty response choices.Direct access to
resp.Choices[0]without checking if the slice is non-empty can cause a panic.As previously noted, add a guard:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("no choices returned from API") + } return resp.Choices[0].Message.Content, nil
91-91: Correct the error message to reflect LogAgent call.The error message incorrectly references "LogPrompt" when the actual method being called is
LogAgent.Apply this fix:
- return "", fmt.Errorf("LogPrompt error: %w", err) + return "", fmt.Errorf("LogAgent error: %w", err)
155-155: Correct the error message to reflect LogToolCall.The error message incorrectly references "LogPrompt" when calling
LogToolCall.Apply this fix:
- return "", fmt.Errorf("LogPrompt error: %w", err) + return "", fmt.Errorf("LogToolCall error: %w", err)
244-244: Guard against empty response choices.Direct access to
resp.Choices[0]can panic if the API returns no choices.Add a bounds check:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("no choices returned from API") + } return resp.Choices[0].Text, nil
191-191: Guard against empty response choices.Direct access to
resp.Choices[0]can panic if no choices are returned.Add a bounds check:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("no choices returned from API") + } return resp.Choices[0].Message.Content, nil
133-133: Guard against empty response choices.Direct access to
resp.Choices[0]can panic if the API returns no choices.Add a bounds check:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("no choices returned from API") + } return resp.Choices[0].Message.Content, nil
🧹 Nitpick comments (2)
traceloop-sdk/sdk.go (1)
187-241: Consider extracting common span creation logic.The
LogToolCallandLogAgentmethods share substantial code with minor variations (span name format, span kind, and one attribute). Consider extracting the common logic into a private helper method.Example refactor:
func (instance *Traceloop) logEntity(ctx context.Context, name, entityType string, entityAttrKey attribute.Key, prompt Prompt, workflowAttrs WorkflowAttributes) (LLMSpan, error) { spanName := fmt.Sprintf("%s.%s", name, entityType) _, span := instance.getTracer().Start(ctx, spanName) spanAttrs := []attribute.KeyValue{ semconvai.LLMVendor.String(prompt.Vendor), semconvai.LLMRequestModel.String(prompt.Model), semconvai.LLMRequestType.String(prompt.Mode), semconvai.TraceloopWorkflowName.String(workflowAttrs.Name), semconvai.TraceloopSpanKind.String(entityType), entityAttrKey.String(name), } for key, value := range workflowAttrs.AssociationProperties { spanAttrs = append(spanAttrs, attribute.String("traceloop.association.properties."+key, value)) } span.SetAttributes(spanAttrs...) setMessagesAttribute(span, "llm.prompts", prompt.Messages) setToolsAttribute(span, prompt.Tools) return LLMSpan{span: span}, nil }Then simplify the public methods:
func (instance *Traceloop) LogToolCall(ctx context.Context, attrs ToolCallAttributes, prompt Prompt, workflowAttrs WorkflowAttributes) (LLMSpan, error) { return instance.logEntity(ctx, attrs.Name, "tool", semconvai.TraceloopEntityName, prompt, workflowAttrs) } func (instance *Traceloop) LogAgent(ctx context.Context, attrs AgentAttributes, prompt Prompt, workflowAttrs WorkflowAttributes) (LLMSpan, error) { return instance.logEntity(ctx, attrs.Name, "agent", semconvai.LLMAgentName, prompt, workflowAttrs) }sample-app/generate_joke_workflow_example.go (1)
12-245: Consider extracting common LLM call pattern.The four functions (
createJoke,translateJokeToPirate,historyJokesTool,generateSignature) share substantial boilerplate for prompt logging, API calls, and completion logging. Consider extracting a helper to reduce duplication and centralize error handling.Example helper:
func callLLMWithLogging( ctx context.Context, client *openai.Client, llmSpan sdk.LLMSpan, model string, prompt string, useChat bool, ) (string, error) { var content string var usage openai.Usage var responseModel string if useChat { resp, err := client.CreateChatCompletion(ctx, openai.ChatCompletionRequest{ Model: model, Messages: []openai.ChatCompletionMessage{ {Role: "user", Content: prompt}, }, }) if err != nil { return "", err } if len(resp.Choices) == 0 { return "", fmt.Errorf("no choices returned from API") } content = resp.Choices[0].Message.Content usage = resp.Usage responseModel = resp.Model } else { resp, err := client.CreateCompletion(ctx, openai.CompletionRequest{ Model: model, Prompt: prompt, }) if err != nil { return "", err } if len(resp.Choices) == 0 { return "", fmt.Errorf("no choices returned from API") } content = resp.Choices[0].Text usage = resp.Usage responseModel = resp.Model } // Log completion llmSpan.LogCompletion(ctx, sdk.Completion{ Model: responseModel, Messages: []sdk.Message{{Index: 0, Role: "assistant", Content: content}}, }, sdk.Usage{ TotalTokens: usage.TotalTokens, CompletionTokens: usage.CompletionTokens, PromptTokens: usage.PromptTokens, }) return content, nil }This would centralize the bounds checking and reduce each function to its core logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.gitignore(0 hunks)sample-app/generate_joke_workflow_example.go(1 hunks)sample-app/main.go(1 hunks)semconv-ai/attributes.go(1 hunks)traceloop-sdk/sdk.go(1 hunks)traceloop-sdk/tracing_types.go(1 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
traceloop-sdk/sdk.go (2)
traceloop-sdk/tracing_types.go (4)
ToolCallAttributes(61-63)Prompt(10-21)WorkflowAttributes(28-31)AgentAttributes(65-67)semconv-ai/attributes.go (7)
LLMVendor(7-7)LLMRequestModel(9-9)LLMRequestType(8-8)TraceloopWorkflowName(34-34)TraceloopSpanKind(33-33)TraceloopEntityName(35-35)LLMAgentName(26-26)
sample-app/generate_joke_workflow_example.go (4)
traceloop-sdk/workflow.go (1)
Workflow(11-15)traceloop-sdk/tracing_types.go (5)
Completion(23-26)Usage(33-37)AgentAttributes(65-67)ToolCallAttributes(61-63)WorkflowAttributes(28-31)traceloop-sdk/sdk.go (2)
Traceloop(24-30)NewClient(36-49)traceloop-sdk/config.go (1)
Config(9-17)
🪛 GitHub Actions: CI
traceloop-sdk/sdk.go
[error] 226-226: Go test failed: undefined: semconvai.LLMAgentName. Command: 'GOWORK=off find . -name go.mod -execdir go test ./... ;'
🔇 Additional comments (5)
semconv-ai/attributes.go (1)
26-26: LGTM! Clean addition of agent name attribute.The new
LLMAgentNameconstant follows the established pattern and provides proper semantic convention support for agent logging.sample-app/main.go (1)
27-28: LGTM! Default workflow switched to joke example.The change cleanly switches from the previous workflow to the new joke workflow demonstration.
traceloop-sdk/tracing_types.go (1)
60-67: LGTM! Well-structured attribute types.The new
ToolCallAttributesandAgentAttributestypes follow the established pattern and provide clean interfaces for the new logging methods.sample-app/generate_joke_workflow_example.go (1)
247-300: LGTM! Clean workflow orchestration.The
runJokeWorkflowfunction properly orchestrates the workflow steps with appropriate error handling and cleanup. The defer statements ensure proper resource cleanup.traceloop-sdk/sdk.go (1)
226-226: The original review comment is incorrect.The constant
LLMAgentNameis properly defined insemconv-ai/attributes.go(line 26), exported as a public identifier, correctly imported via thesemconvaialias, and used with the same pattern as other working constants (LLMVendor,LLMRequestModel,LLMRequestType) in the same code section. The code at line 226 is valid and builds successfully. No undefined error exists.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed aa91b3e in 1 minute and 39 seconds. Click for details.
- Reviewed
24lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. traceloop-sdk/agent.go:24
- Draft comment:
Refactored Agent.LogPrompt now directly calls sdk.LogPrompt with context and workflow attributes. Ensure that agent.workflow.Attributes is properly initialized when non-nil. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. traceloop-sdk/tool.go:21
- Draft comment:
Tool.LogPrompt uses tool.agent.workflow.Attributes without checking if tool.agent.workflow is nil, which risks a nil pointer dereference. Consider adding a nil check similar to Agent.LogPrompt. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is about the changed code (line 22), so it passes the "is_about_changes" test. The concern is about a potential nil pointer dereference. However, to properly evaluate this, I would need to see: 1) How Tool objects are constructed - does workflow always exist? 2) The Agent.LogPrompt implementation that supposedly has the nil check. 3) Whether there are any guarantees about workflow being non-nil. Without this context, I cannot verify if this is a real issue or if the tool is incorrectly flagging something that's guaranteed to be safe by construction. The comment references Agent.LogPrompt as evidence, but I don't have access to that code. I might be too quick to dismiss this. The comment specifically mentions that Agent.LogPrompt has a similar nil check, which suggests the reviewer has seen evidence that workflow can be nil. If Agent.LogPrompt checks for nil workflow, that's a strong signal this is a real issue. However, I can't verify this claim without seeing that code. While the reference to Agent.LogPrompt is suggestive, I cannot verify the claim without seeing that code. The rules state I should only keep comments with STRONG EVIDENCE that they are correct, and that if understanding requires more context from other files, I should delete the comment. This falls into that category. Delete this comment. While it raises a potentially valid concern about nil pointer dereference, I cannot verify the claim without seeing the Agent.LogPrompt implementation or how Tool objects are constructed. This requires cross-file context that isn't available.
Workflow ID: wflow_p8CsguIiWTXJbxa1
You can customize
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
sample-app/generate_joke_workflow_example.go (2)
65-65: Guard against empty chat completion responses.
resp.Choices[0]will panic whenever OpenAI returns no choices (e.g., filtered content or rate limits). Please short-circuit with a length check before indexing so we can surface a clean error instead.resp, err := client.CreateChatCompletion(ctx, openai.ChatCompletionRequest{ Model: "gpt-3.5-turbo", Messages: []openai.ChatCompletionMessage{ @@ if err != nil { return "", fmt.Errorf("CreateChatCompletion error: %w", err) } + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + }
188-188: Prevent the tool call from panicking on empty choices.
historyJokesToolalso assumes at least one choice; add the same defensive check so the workflow returns an error rather than crashing.resp, err := client.CreateChatCompletion(ctx, openai.ChatCompletionRequest{ Model: "gpt-3.5-turbo", Messages: []openai.ChatCompletionMessage{ @@ if err != nil { return "", fmt.Errorf("CreateChatCompletion error: %w", err) } + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + }
🧹 Nitpick comments (1)
sample-app/main.go (1)
65-83: Remove the stale LogPrompt error check.
After switching to the new API,LogPromptno longer returns an error, so thiserrcheck can never trip and only reads the previous value. Drop the block to avoid confusion for future readers.llmSpan := traceloop.LogPrompt( ctx, sdk.Prompt{ Vendor: "openai", Mode: "chat", Model: request.Model, Messages: promptMsgs, }, sdk.WorkflowAttributes{ Name: "example-workflow", AssociationProperties: map[string]string{ "user_id": "demo-user", }, }, ) - if err != nil { - fmt.Printf("LogPrompt error: %v\n", err) - return - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
sample-app/generate_joke_workflow_example.go(1 hunks)sample-app/main.go(3 hunks)sample-app/tool_calling.go(2 hunks)sample-app/workflow_example.go(2 hunks)traceloop-sdk/agent.go(1 hunks)traceloop-sdk/llm_span.go(1 hunks)traceloop-sdk/model/types.go(1 hunks)traceloop-sdk/sdk.go(3 hunks)traceloop-sdk/sdk_test.go(2 hunks)traceloop-sdk/task.go(1 hunks)traceloop-sdk/tool.go(1 hunks)traceloop-sdk/tracing_types.go(1 hunks)traceloop-sdk/workflow.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
traceloop-sdk/llm_span.go (2)
semconv-ai/attributes.go (6)
LLMRequestModel(9-9)LLMRequestType(8-8)LLMResponseModel(10-10)LLMUsageTotalTokens(12-12)LLMUsageCompletionTokens(13-13)LLMUsagePromptTokens(14-14)traceloop-sdk/tracing_types.go (2)
Completion(23-26)Usage(33-37)
traceloop-sdk/sdk.go (3)
traceloop-sdk/tracing_types.go (4)
Prompt(10-21)WorkflowAttributes(28-31)ToolCallAttributes(57-59)AgentAttributes(61-63)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)semconv-ai/attributes.go (4)
TraceloopWorkflowName(34-34)TraceloopSpanKind(33-33)TraceloopEntityName(35-35)LLMAgentName(26-26)
sample-app/generate_joke_workflow_example.go (5)
traceloop-sdk/workflow.go (1)
Workflow(12-16)traceloop-sdk/tracing_types.go (6)
Prompt(10-21)Message(3-8)Completion(23-26)Usage(33-37)ToolFunction(39-43)WorkflowAttributes(28-31)traceloop-sdk/agent.go (1)
Agent(12-17)traceloop-sdk/sdk.go (1)
NewClient(38-51)traceloop-sdk/config.go (1)
Config(9-17)
traceloop-sdk/agent.go (7)
traceloop-sdk/sdk.go (1)
Traceloop(29-35)traceloop-sdk/workflow.go (1)
Workflow(12-16)traceloop-sdk/tracing_types.go (3)
Prompt(10-21)WorkflowAttributes(28-31)ToolFunction(39-43)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)traceloop-sdk/tool.go (1)
Tool(9-15)semconv-ai/attributes.go (3)
LLMAgentName(26-26)TraceloopSpanKind(33-33)TraceloopEntityName(35-35)traceloop-sdk/model/types.go (1)
SpanKindTool(6-6)
sample-app/workflow_example.go (1)
traceloop-sdk/tracing_types.go (1)
Prompt(10-21)
traceloop-sdk/task.go (3)
traceloop-sdk/workflow.go (1)
Workflow(12-16)traceloop-sdk/tracing_types.go (3)
Prompt(10-21)AgentAttributes(61-63)ToolCallAttributes(57-59)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
traceloop-sdk/tool.go (3)
traceloop-sdk/agent.go (1)
Agent(12-17)traceloop-sdk/tracing_types.go (2)
ToolFunction(39-43)Prompt(10-21)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
traceloop-sdk/workflow.go (5)
semconv-ai/attributes.go (3)
TraceloopSpanKind(33-33)TraceloopWorkflowName(34-34)TraceloopEntityName(35-35)traceloop-sdk/model/types.go (3)
SpanKindWorkflow(9-9)SpanKindTask(8-8)SpanKindAgent(7-7)traceloop-sdk/tracing_types.go (1)
Prompt(10-21)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)traceloop-sdk/agent.go (1)
Agent(12-17)
sample-app/main.go (1)
traceloop-sdk/tracing_types.go (1)
Completion(23-26)
🪛 GitHub Actions: CI
traceloop-sdk/agent.go
[error] 33-33: undefined: semconvai.LLMAgentName
🔇 Additional comments (3)
sample-app/workflow_example.go (1)
50-57: LogPrompt signature usage looks consistent.Capturing the returned
LLMSpanand dropping the old error-path matches the updated SDK API, and the follow-up completion logging still flows correctly.Also applies to: 90-101
traceloop-sdk/model/types.go (1)
5-10: Typed span kind constants look good.Having a shared
SpanKindenum will keep span annotations consistent across workflow, agent, tool, and task code paths.traceloop-sdk/task.go (1)
19-29: Task logging delegation aligns with the new SDK flow.Forwarding through the workflow keeps the existing attribute propagation intact for prompts, agents, and tool calls.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 9cf4d61 in 3 minutes and 9 seconds. Click for details.
- Reviewed
195lines of code in7files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sample-app/generate_joke_workflow_example.go:267
- Draft comment:
Pass 'nil' for ABTest in NewWorkflow call per the new signature. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. sample-app/recipe_agent_example.go:246
- Draft comment:
Update NewAgent call to use AgentAttributes and pass the abTest variable; remove hardcoded 'ab_testing_variant'. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. sample-app/workflow_example.go:29
- Draft comment:
Include 'nil' for the ABTest parameter in the NewWorkflow call as per the updated constructor. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. traceloop-sdk/model/types.go:13
- Draft comment:
Typo: Consider renaming 'VarientsKeys' to 'VariantsKeys' for clarity. - Reason this comment was not posted:
Marked as duplicate.
5. traceloop-sdk/workflow.go:38
- Draft comment:
Missing ABTest assignment in Workflow return; add 'ABTest: abTest' to propagate the AB test configuration. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. sample-app/recipe_agent_example.go:18
- Draft comment:
Typo found: 'VarientsKeys' appears to be misspelled. It should likely be 'VariantsKeys'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is about new code added in the diff (theabTestvariable). The typo appears legitimate - "VarientsKeys" should be "VariantsKeys". However, I need to consider: 1) This is a struct field name from themodel.ABTesttype, which is imported from an external package. 2) If this is a field defined in the external package, the author cannot change it - they must use whatever field name the package defines. 3) If the package itself has a typo, the author still has to use that typo to make the code compile. 4) The code appears to compile (or at least is being submitted), which suggestsVarientsKeysis the actual field name in the package. This would be caught by the build if it were wrong. I'm assuming this is a field from an external package, but I don't have definitive proof. If this is actually a field the author is defining themselves (not from the imported model package), then the typo comment would be valid and actionable. However, the fact that it's using&model.ABTest{}strongly suggests this is a struct type from the imported package. The rules state "Do NOT comment on anything that would be obviously caught by the build, such as variable renames, file renames, or changed imports." IfVarientsKeysis the wrong field name, the Go compiler would catch this immediately. Since the code is being submitted, it must compile, which meansVarientsKeysis the actual field name in themodel.ABTeststruct. The author cannot change field names in external packages. This comment should be deleted. The field nameVarientsKeysis from an external package (model.ABTest), and if it were incorrect, the build would fail. The author cannot change field names in external dependencies, even if they contain typos.
7. traceloop-sdk/sdk.go:156
- Draft comment:
Typo detected: 'activeVarient' should likely be 'activeVariant' and 'VarientsKeys' should be corrected to 'VariantsKeys'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a spelling/typo comment. The comment correctly identifies that "Varient" should be "Variant". However, I need to consider: 1) Is this actionable? Yes, it's a clear typo fix. 2) Is it about changed code? Yes, this code was added in the diff. 3) The fieldVarientsKeysis part of themodel.ABTeststruct which is defined elsewhere - if that's the actual field name in the struct, then changing the variable name won't help and the struct field itself needs to be changed. 4) IfVarientsKeysis the actual field name in the model, then this might be a cross-file issue that requires changing the model definition, not just this file. 5) The rules say "Ignore cross-file issues. Only think about the file you are reviewing." This could be a cross-file issue if the field name is defined elsewhere. The fieldVarientsKeysappears to be coming fromabTest.VarientsKeyswhereabTestis of type*model.ABTest. This suggests the field is defined in another file/package. If that's the case, fixing just the variable nameactiveVarientin this file won't fix the underlying issue withVarientsKeys, and this becomes a cross-file issue which I should ignore per the rules. WhileVarientsKeysmay be defined elsewhere, the variable nameactiveVarientis defined locally in this file on line 156. The author could at least fix the local variable name toactiveVarianteven if they can't change the struct field name in this PR. However, the comment suggests fixing both, and fixing the struct field would be a cross-file change. The comment identifies a legitimate typo, but part of it (VarientsKeys) likely requires changes to another file/package, making it a cross-file issue. According to the rules, I should ignore cross-file issues. Even though the local variable could be renamed, the comment as written suggests both changes, making it not fully actionable within this file alone.
8. traceloop-sdk/workflow.go:30
- Draft comment:
Typographical error: The variable name 'activeVarient' and the field 'VarientsKeys' appear to be misspelled. Consider renaming them to 'activeVariant' and 'VariantsKeys' for clarity and consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is about a typo in variable/field names. Looking at the diff, the code using "activeVarient" and "VarientsKeys" is newly added in this PR. However, the field "VarientsKeys" is part of themodel.ABTesttype which is defined elsewhere (not in this file). The variable name "activeVarient" is defined locally in this file's new code. The comment suggests renaming both, but we can only control the local variable name here. The field name "VarientsKeys" would need to be changed in the model definition, which is cross-file. According to the rules, I should "Ignore cross-file issues. Only think about the file you are reviewing." The local variable "activeVarient" could be renamed to "activeVariant" in this file, but that would still be using a field called "VarientsKeys" from the model, creating an inconsistency. The comment identifies a legitimate typo in newly added code. While the field name is defined elsewhere, the local variable name "activeVarient" is defined in this file and could be corrected. This is a code quality improvement that is clear and actionable for the local variable at least. While the local variable could be renamed, the comment suggests renaming both the variable and the field. Since the field is defined in another file (model.ABTest), this becomes a cross-file issue which I should ignore per the rules. Additionally, renaming just the local variable while keeping the field name as "VarientsKeys" would create inconsistency. The comment isn't clearly actionable within the scope of just this file. The comment addresses both a local variable and a field from another file. Since it involves cross-file changes and the rules state to ignore cross-file issues, this comment should be deleted. The author would need to see the model definition to understand the full context and make a coordinated change.
9. traceloop-sdk/workflow.go:83
- Draft comment:
Typographical error: The word "Varients" inworkflow.ABTest.VarientsKeysappears to be misspelled. Consider renaming it to "Variants" (and similarly updating "activeVarient" to "activeVariant"). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a legitimate code quality issue pointing out a spelling mistake in variable names. The comment is actionable and clear - it identifies specific misspellings and suggests corrections. However, I need to consider: 1) The fieldVarientsKeyscomes frommodel.ABTest, which is defined in another file/package, so changing it here might not be possible without changing the model definition. 2) The local variableactiveVarientcan definitely be fixed. 3) This is about code that was changed/added in the diff, so it's relevant. 4) The rules say code quality refactors are good if actionable and clear, which this is. But there's a concern: ifVarientsKeysis defined in the model package, the PR author may not be able to fix it in this file alone, making the comment less actionable. The fieldVarientsKeysappears to be part of themodel.ABTeststruct from another package. If this is defined elsewhere, the PR author cannot fix it in this file, making the comment potentially not actionable. The comment might require cross-file changes, which violates the "ignore cross-file issues" rule. WhileVarientsKeysmay be defined in another file, the local variableactiveVarientis defined in this file and can be fixed. However, the comment suggests fixing both, and if the struct field can't be changed here, the comment becomes only partially actionable. More importantly, the rule states to "ignore cross-file issues" and "only think about the file you are reviewing." Since the root cause (the struct field name) is in another file, this might fall under that rule. The comment identifies a legitimate typo, but the primary issue (VarientsKeys) is in themodel.ABTeststruct from another package, which cannot be fixed in this file. This makes it a cross-file issue. While the local variableactiveVarientcould be fixed, the comment's main focus is on the struct field. Given the rule to ignore cross-file issues, this comment should be deleted.
Workflow ID: wflow_Nxvc1gpbw6QcQYPH
You can customize
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
traceloop-sdk/workflow.go (1)
38-42: Assign theabTestparameter to the returnedWorkflowstruct.The
abTestparameter is accepted but never assigned toworkflow.ABTest, so agents created viaworkflow.NewAgent(line 82) will always see a nil ABTest field.Apply this diff to fix the issue:
return &Workflow{ sdk: instance, ctx: wCtx, Attributes: attrs, + ABTest: abTest, }
♻️ Duplicate comments (6)
traceloop-sdk/workflow.go (1)
73-105: LGTM! Agent name attribute correctly set.The
LLMAgentNamesemantic attribute is properly set on line 79, addressing the concern from the previous review.traceloop-sdk/tool.go (1)
21-41: LGTM! Nil workflow guard prevents panic.The nil check on line 28 properly handles agents created without a workflow, addressing the concern from the previous review. The context attributes are correctly merged from workflow and agent properties.
traceloop-sdk/sdk.go (1)
242-246: Add missingTraceloopEntityNameattribute to agent spans.Agent spans created via
LogAgentdon't settraceloop.entity.name, while tool spans (line 222) and standalone agent spans (line 151) do. This breaks consistency for downstream filtering.Apply this diff:
spanAttrs := []attribute.KeyValue{ semconvai.TraceloopWorkflowName.String(workflowAttrs.Name), semconvai.TraceloopSpanKind.String(string(model.SpanKindAgent)), + semconvai.TraceloopEntityName.String(attrs.Name), semconvai.LLMAgentName.String(attrs.Name), }sample-app/generate_joke_workflow_example.go (3)
42-65: Add guard for emptyresp.Choicesbefore indexing.Line 65 accesses
resp.Choices[0]without verifying the slice is non-empty, which can panic if the API returns no choices.Apply this diff:
if err != nil { return "", fmt.Errorf("CreateChatCompletion error: %w", err) } + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + }
92-130: Add guard for emptyresp.Choicesbefore indexing.Line 130 accesses
resp.Choices[0]without checking if the slice has elements.Apply this diff:
if err != nil { return "", fmt.Errorf("CreateChatCompletion error: %w", err) } + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + }
218-242: Add guard for emptyresp.Choicesbefore indexing.Lines 233 and 242 access
resp.Choices[0]without checking the slice length, which can panic if no choices are returned.Apply this diff:
if err != nil { return "", fmt.Errorf("CreateCompletion error: %w", err) } + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateCompletion returned no choices") + }
🧹 Nitpick comments (3)
sample-app/main.go (1)
73-93: Remove the dead error check afterLogPrompt.
LogPromptno longer returns an error, so thisif err != nilblock is now unreachable. Keeping it only adds confusion about which call could fail. Please drop the block so we don’t guard on a stale value.- if err != nil { - fmt.Printf("LogPrompt error: %v\n", err) - return - }traceloop-sdk/workflow.go (1)
29-36: Fix typo: "Varient" should be "Variant".The variable name
activeVarientcontains a typo and should beactiveVariant. This typo also appears in themodel.ABTest.VarientsKeysfield name.Apply this diff:
- for key, activeVarient := range abTest.VarientsKeys { - if activeVarient { + for key, activeVariant := range abTest.VarientsKeys { + if activeVariant { span.SetAttributes(attribute.String("traceloop.association.properties.ab_testing_variant", key)) break }- for key, activeVarient := range workflow.ABTest.VarientsKeys { - if activeVarient { + for key, activeVariant := range workflow.ABTest.VarientsKeys { + if activeVariant { associationProperties["ab_testing_variant"] = key }Note: You'll also need to update
model.ABTest.VarientsKeystoVariantsKeysintraceloop-sdk/model/types.go.Also applies to: 82-88
sample-app/recipe_agent_example.go (1)
25-228: Consider extracting common tool execution logic.The three tool functions share substantial boilerplate for prompt construction, API calls, completion logging, and error handling. Extracting a helper function would reduce duplication.
Example helper function:
func executeTool( ctx context.Context, agent *sdk.Agent, client *openai.Client, toolName, description, promptContent string, params map[string]interface{}, assocProps map[string]string, ) (string, error) { tool := agent.NewTool(toolName, "function", sdk.ToolFunction{ Name: toolName, Description: description, Parameters: params, }, assocProps) defer tool.End() prompt := sdk.Prompt{ Vendor: "openai", Mode: "chat", Model: "gpt-3.5-turbo", Messages: []sdk.Message{{Index: 0, Role: "user", Content: promptContent}}, } llmSpan := tool.LogPrompt(prompt) resp, err := client.CreateChatCompletion(ctx, openai.ChatCompletionRequest{ Model: "gpt-3.5-turbo", Messages: []openai.ChatCompletionMessage{{Role: "user", Content: promptContent}}, }) if err != nil { return "", fmt.Errorf("CreateChatCompletion error: %w", err) } var completionMsgs []sdk.Message for _, choice := range resp.Choices { completionMsgs = append(completionMsgs, sdk.Message{ Index: choice.Index, Content: choice.Message.Content, Role: choice.Message.Role, }) } llmSpan.LogCompletion(ctx, sdk.Completion{Model: resp.Model, Messages: completionMsgs}, sdk.Usage{TotalTokens: resp.Usage.TotalTokens, CompletionTokens: resp.Usage.CompletionTokens, PromptTokens: resp.Usage.PromptTokens}) return resp.Choices[0].Message.Content, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
sample-app/generate_joke_workflow_example.go(1 hunks)sample-app/main.go(3 hunks)sample-app/recipe_agent_example.go(1 hunks)sample-app/tool_calling.go(2 hunks)sample-app/workflow_example.go(3 hunks)traceloop-sdk/agent.go(1 hunks)traceloop-sdk/llm_span.go(1 hunks)traceloop-sdk/model/types.go(1 hunks)traceloop-sdk/sdk.go(2 hunks)traceloop-sdk/sdk_test.go(2 hunks)traceloop-sdk/task.go(1 hunks)traceloop-sdk/tool.go(1 hunks)traceloop-sdk/tracing_types.go(2 hunks)traceloop-sdk/workflow.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- traceloop-sdk/llm_span.go
🧰 Additional context used
🧬 Code graph analysis (11)
traceloop-sdk/agent.go (7)
traceloop-sdk/sdk.go (1)
Traceloop(25-31)traceloop-sdk/workflow.go (1)
Workflow(13-18)traceloop-sdk/tracing_types.go (4)
AgentAttributes(66-69)Prompt(10-21)ContextAttributes(33-37)ToolFunction(45-49)traceloop-sdk/model/types.go (2)
ABTest(13-15)SpanKindTool(6-6)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)traceloop-sdk/tool.go (1)
Tool(9-15)semconv-ai/attributes.go (3)
LLMAgentName(26-26)TraceloopSpanKind(33-33)TraceloopEntityName(35-35)
sample-app/tool_calling.go (1)
traceloop-sdk/tracing_types.go (1)
ContextAttributes(33-37)
traceloop-sdk/sdk_test.go (1)
traceloop-sdk/tracing_types.go (1)
ContextAttributes(33-37)
traceloop-sdk/task.go (3)
traceloop-sdk/workflow.go (1)
Workflow(13-18)traceloop-sdk/tracing_types.go (2)
Prompt(10-21)ContextAttributes(33-37)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
sample-app/generate_joke_workflow_example.go (5)
traceloop-sdk/workflow.go (1)
Workflow(13-18)traceloop-sdk/tracing_types.go (6)
Prompt(10-21)Message(3-8)Completion(23-26)Usage(39-43)ToolFunction(45-49)WorkflowAttributes(28-31)traceloop-sdk/agent.go (1)
Agent(13-19)traceloop-sdk/sdk.go (1)
NewClient(34-47)traceloop-sdk/config.go (1)
Config(9-17)
traceloop-sdk/tool.go (3)
traceloop-sdk/agent.go (1)
Agent(13-19)traceloop-sdk/tracing_types.go (3)
ToolFunction(45-49)Prompt(10-21)ContextAttributes(33-37)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
sample-app/main.go (1)
traceloop-sdk/tracing_types.go (3)
Prompt(10-21)ContextAttributes(33-37)Completion(23-26)
traceloop-sdk/sdk.go (5)
traceloop-sdk/tracing_types.go (5)
AgentAttributes(66-69)Prompt(10-21)ContextAttributes(33-37)ToolCallAttributes(62-64)WorkflowAttributes(28-31)traceloop-sdk/model/types.go (3)
ABTest(13-15)SpanKindAgent(7-7)SpanKindTool(6-6)traceloop-sdk/agent.go (1)
Agent(13-19)semconv-ai/attributes.go (7)
TraceloopSpanKind(33-33)TraceloopEntityName(35-35)LLMAgentName(26-26)LLMVendor(7-7)LLMRequestModel(9-9)LLMRequestType(8-8)TraceloopWorkflowName(34-34)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
sample-app/recipe_agent_example.go (5)
traceloop-sdk/model/types.go (1)
ABTest(13-15)traceloop-sdk/agent.go (1)
Agent(13-19)traceloop-sdk/tracing_types.go (4)
ToolFunction(45-49)Completion(23-26)Usage(39-43)AgentAttributes(66-69)traceloop-sdk/sdk.go (1)
NewClient(34-47)traceloop-sdk/config.go (1)
Config(9-17)
sample-app/workflow_example.go (1)
traceloop-sdk/tracing_types.go (1)
Prompt(10-21)
traceloop-sdk/workflow.go (5)
traceloop-sdk/tracing_types.go (3)
WorkflowAttributes(28-31)ContextAttributes(33-37)AgentAttributes(66-69)traceloop-sdk/model/types.go (4)
ABTest(13-15)SpanKindWorkflow(9-9)SpanKindTask(8-8)SpanKindAgent(7-7)semconv-ai/attributes.go (3)
TraceloopWorkflowName(34-34)TraceloopSpanKind(33-33)TraceloopEntityName(35-35)traceloop-sdk/task.go (1)
Task(9-13)traceloop-sdk/agent.go (1)
Agent(13-19)
🪛 GitHub Actions: CI
traceloop-sdk/agent.go
[error] 50-50: undefined: semconvai.LLMAgentName
🔇 Additional comments (4)
traceloop-sdk/tracing_types.go (2)
33-37: LGTM! New attribute structs support context propagation.The new
ContextAttributes,ToolCallAttributes, andAgentAttributesstructs properly support the refactored logging APIs with appropriate use of pointer fields for optional values.Also applies to: 62-64, 66-69
20-20: I see the Tool type is confirmed to exist attraceloop-sdk/tool.go:9. However, the build output was truncated. Let me verify this more thoroughly by checking the actual struct definitions:No issues found—the
Tooltype is properly defined and correctly used.The verification confirms that the
Tooltype is defined intraceloop-sdk/tool.go:9. ThePrompt.Toolsfield correctly references this existing type, and the code builds without errors related to undefined types. The original concern about theTooltype being removed is not applicable to the current codebase.traceloop-sdk/sdk.go (2)
145-178: LGTM! Standalone agent creation properly configured.The
NewAgentmethod correctly sets all required span attributes including entity name and agent name. Note that theactiveVarienttypo on line 156 should be addressed as part of the fix mentioned in theworkflow.goreview.
181-212: LGTM! ContextAttributes properly integrated.The refactored
LogPromptcorrectly handles optional workflow and agent names using pointer checks and properly propagates association properties.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 95f583d in 2 minutes and 15 seconds. Click for details.
- Reviewed
218lines of code in8files - Skipped
0files when reviewing. - Skipped posting
14draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sample-app/generate_joke_workflow_example.go:267
- Draft comment:
Removed the extra nil parameter from the NewWorkflow call to match the updated SDK signature. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change that was made to match an updated SDK signature. It does not provide a suggestion, ask for confirmation, or point out a potential issue.
2. sample-app/main.go:39
- Draft comment:
Ensure the file ends with a newline for POSIX compliance. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. sample-app/recipe_agent_example.go:240
- Draft comment:
Moved ABTest into AgentAttributes instead of passing it as a separate parameter for better consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change that was made. It does not provide a suggestion, ask for confirmation, or point out a potential issue. According to the rules, purely informative comments should be removed.
4. sample-app/recipe_agent_example.go:54
- Draft comment:
Removed redundant logging comments (e.g., 'Make API call' and 'Log completion') in the tool functions to streamline the code. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating what was done without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements.
5. sample-app/workflow_example.go:27
- Draft comment:
Removed the unnecessary nil parameter from the NewWorkflow call, ensuring consistency with the updated SDK API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change that was made without suggesting any action or asking for clarification. It doesn't provide a specific code suggestion or ask for tests to be written.
6. traceloop-sdk/agent.go:17
- Draft comment:
Removed the ABTest field from the Agent struct; ABTest information is now part of AgentAttributes. Ensure related usage is updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is informative and suggests ensuring that related usage is updated accordingly. However, it does not provide a specific suggestion or ask for a specific test to be written. It is more of a general reminder, which violates the rule against asking the PR author to ensure something is done.
7. traceloop-sdk/sdk.go:146
- Draft comment:
Updated NewAgent signature to remove the separate ABTest parameter; ABTest is now provided via AgentAttributes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, explaining a change in the code without suggesting any action or asking for clarification. It doesn't provide any specific guidance or raise any issues that need to be addressed.
8. traceloop-sdk/tracing_types.go:29
- Draft comment:
Added the ABTest field to both WorkflowAttributes and AgentAttributes to support comprehensive AB-test propagation. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. traceloop-sdk/workflow.go:28
- Draft comment:
NewWorkflow now propagates ABTest from WorkflowAttributes into the span attributes for AB-test tracking. Verify consistency with Agent and SDK updates. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify consistency with other parts of the system, which violates the rule against asking for confirmation or verification. It doesn't provide a specific suggestion or point out a specific issue with the code.
10. traceloop-sdk/sdk.go:156
- Draft comment:
Typo detected: 'activeVarient' should be corrected to 'activeVariant'. - Reason this comment was not posted:
Comment was on unchanged code.
11. traceloop-sdk/sdk.go:156
- Draft comment:
Typo detected: 'VarientsKeys' appears to be a misspelling. Consider renaming it to 'VariantsKeys' if that's the intended meaning. - Reason this comment was not posted:
Comment was on unchanged code.
12. traceloop-sdk/tracing_types.go:3
- Draft comment:
Typographical error: The import path appears to have a typo. 'go-openllmetry' might be intended to be 'go-opentelemetry' (note the extra 'l'). Please confirm and correct if necessary. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment assumes "go-openllmetry" is a typo for "go-opentelemetry", but this is likely incorrect. Given that this is a traceloop package dealing with LLM tracing (as evidenced by the types like Message, Prompt, Completion, etc.), "openllmetry" is almost certainly intentional - it's about LLM telemetry, not general OpenTelemetry. The package name makes semantic sense in context. This comment is speculative and asks the author to "confirm and correct if necessary", which violates the rule about not asking authors to confirm their intention. There's no strong evidence this is actually a typo. Could there be a legitimate typo here? Maybe the package was supposed to use OpenTelemetry but was misspelled. Without seeing the actual repository or knowing the intended package name, I can't be 100% certain this isn't a real typo. While I can't be absolutely certain, the context strongly suggests this is intentional. The package is about LLM tracing ("traceloop"), and "openllmetry" (Open LLM Telemetry) is a logical name. The comment is speculative and asks for confirmation, which violates the review rules. Without strong evidence of an actual error, this comment should be deleted. This comment should be deleted. It's speculative, asks the author to confirm something, and there's no strong evidence that "go-openllmetry" is actually a typo rather than an intentional package name for "Open LLM Telemetry".
13. traceloop-sdk/workflow.go:29
- Draft comment:
Typographical error: "activeVarient" should likely be "activeVariant", and similarly, "VarientsKeys" might be intended as "VariantsKeys". - Reason this comment was not posted:
Comment was on unchanged code.
14. traceloop-sdk/workflow.go:82
- Draft comment:
There appear to be some typos: the variable nameactiveVarientmight be intended asactiveVariant, andVarientsKeysshould likely beVariantsKeys. Please double-check these for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_tLjUcSAya9cK01XM
You can customize
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (10)
traceloop-sdk/sdk.go (1)
238-255: Includetraceloop.entity.nameon agent spans.Agent spans still omit
TraceloopEntityName, so downstream filters relying on that attribute break. Please add it alongsideLLMAgentName.spanAttrs := []attribute.KeyValue{ semconvai.TraceloopWorkflowName.String(workflowAttrs.Name), semconvai.TraceloopSpanKind.String(string(model.SpanKindAgent)), + semconvai.TraceloopEntityName.String(attrs.Name), semconvai.LLMAgentName.String(attrs.Name), }traceloop-sdk/agent.go (1)
24-43: PopulateContextAttributes.AgentNamebefore logging prompts.Without setting
AgentName, agent prompt spans never receive the newllm.agent.nameattribute. Please set the pointer toagent.Attributes.Namewhen buildingcontextAttrs.- contextAttrs := ContextAttributes{ - AssociationProperties: make(map[string]string), - } + contextAttrs := ContextAttributes{ + AgentName: &agent.Attributes.Name, + AssociationProperties: make(map[string]string), + }sample-app/recipe_agent_example.go (3)
170-172: Use theassociationPropertiesvariable for consistency.Lines 170-172 create an inline map literal instead of using the package-level
associationPropertiesvariable that's used elsewhere (lines 38, 104).Apply this diff:
- }, map[string]string{ - "user_id": "user_67890", - }) + }, associationProperties)
87-87: Guard against emptyresp.Choicesbefore indexing.Line 87 accesses
resp.Choices[0]without checking the slice length, which will panic if no choices are returned.Apply this guard:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nil
221-221: Guard against emptyresp.Choicesbefore indexing.Line 221 accesses
resp.Choices[0]without checking the slice length, which will panic if no choices are returned.Apply this guard:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nilsample-app/generate_joke_workflow_example.go (4)
65-65: Guard against emptyresp.Choicesbefore indexing.Accessing
resp.Choices[0]without verifying the slice is non-empty will panic if the API returns no choices.Apply this guard before line 65:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nil
130-130: Guard against emptyresp.Choicesbefore indexing.Line 130 accesses
resp.Choices[0]without checking the slice length, which will panic if no choices are returned.Apply this guard:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nil
192-192: Guard against emptyresp.Choicesbefore indexing.Line 192 accesses
resp.Choices[0]without verifying the slice has elements, risking a panic.Apply this guard:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nil
233-233: Guard against emptyresp.Choicesbefore indexing.Lines 233 and 242 access
resp.Choices[0].Textwithout checking if the slice is non-empty, which will panic if no choices are returned.Apply this guard after line 224:
if err != nil { return "", fmt.Errorf("CreateCompletion error: %w", err) } + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateCompletion returned no choices") + }Also applies to: 242-242
traceloop-sdk/workflow.go (1)
75-79: AddLLMAgentNameattribute to agent spans.The agent span is missing the
semconvai.LLMAgentNameattribute that was introduced in this PR. This attribute should be set for consistency with other agent logging paths.Apply this diff:
attrs := []attribute.KeyValue{ semconvai.TraceloopWorkflowName.String(workflow.Attributes.Name), semconvai.TraceloopSpanKind.String(string(model.SpanKindAgent)), + semconvai.LLMAgentName.String(name), semconvai.TraceloopEntityName.String(name), }
🧹 Nitpick comments (1)
sample-app/main.go (1)
90-93: Drop staleerrcheck afterLogPrompt.
LogPromptno longer returns an error, so this block only re-checks the olderrvalue (always nil here) and adds confusion. Please remove it for clarity.- if err != nil { - fmt.Printf("LogPrompt error: %v\n", err) - return - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
sample-app/generate_joke_workflow_example.go(1 hunks)sample-app/main.go(3 hunks)sample-app/recipe_agent_example.go(1 hunks)sample-app/workflow_example.go(2 hunks)traceloop-sdk/agent.go(1 hunks)traceloop-sdk/sdk.go(2 hunks)traceloop-sdk/tracing_types.go(3 hunks)traceloop-sdk/workflow.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
traceloop-sdk/tracing_types.go (1)
traceloop-sdk/model/types.go (1)
ABTest(13-15)
traceloop-sdk/sdk.go (5)
traceloop-sdk/tracing_types.go (5)
AgentAttributes(69-73)Prompt(12-23)ContextAttributes(36-40)ToolCallAttributes(65-67)WorkflowAttributes(30-34)traceloop-sdk/agent.go (1)
Agent(13-18)semconv-ai/attributes.go (7)
TraceloopSpanKind(33-33)TraceloopEntityName(35-35)LLMAgentName(26-26)LLMVendor(7-7)LLMRequestModel(9-9)LLMRequestType(8-8)TraceloopWorkflowName(34-34)traceloop-sdk/model/types.go (3)
SpanKindAgent(7-7)ABTest(13-15)SpanKindTool(6-6)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
sample-app/main.go (1)
traceloop-sdk/tracing_types.go (5)
Prompt(12-23)ContextAttributes(36-40)Completion(25-28)Usage(42-46)Message(5-10)
sample-app/recipe_agent_example.go (5)
traceloop-sdk/model/types.go (1)
ABTest(13-15)traceloop-sdk/agent.go (1)
Agent(13-18)traceloop-sdk/tracing_types.go (6)
ToolFunction(48-52)Prompt(12-23)Message(5-10)Completion(25-28)Usage(42-46)AgentAttributes(69-73)traceloop-sdk/sdk.go (1)
NewClient(34-47)traceloop-sdk/config.go (1)
Config(9-17)
traceloop-sdk/agent.go (7)
traceloop-sdk/sdk.go (1)
Traceloop(25-31)traceloop-sdk/workflow.go (1)
Workflow(13-17)traceloop-sdk/tracing_types.go (4)
AgentAttributes(69-73)Prompt(12-23)ContextAttributes(36-40)ToolFunction(48-52)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)traceloop-sdk/tool.go (1)
Tool(9-15)semconv-ai/attributes.go (3)
LLMAgentName(26-26)TraceloopSpanKind(33-33)TraceloopEntityName(35-35)traceloop-sdk/model/types.go (1)
SpanKindTool(6-6)
sample-app/generate_joke_workflow_example.go (5)
traceloop-sdk/workflow.go (1)
Workflow(13-17)traceloop-sdk/tracing_types.go (6)
Prompt(12-23)Message(5-10)Completion(25-28)Usage(42-46)ToolFunction(48-52)WorkflowAttributes(30-34)traceloop-sdk/agent.go (1)
Agent(13-18)traceloop-sdk/sdk.go (1)
NewClient(34-47)traceloop-sdk/config.go (1)
Config(9-17)
traceloop-sdk/workflow.go (4)
semconv-ai/attributes.go (3)
TraceloopSpanKind(33-33)TraceloopEntityName(35-35)TraceloopWorkflowName(34-34)traceloop-sdk/model/types.go (4)
SpanKindWorkflow(9-9)ABTest(13-15)SpanKindTask(8-8)SpanKindAgent(7-7)traceloop-sdk/tracing_types.go (3)
Prompt(12-23)ContextAttributes(36-40)AgentAttributes(69-73)traceloop-sdk/agent.go (1)
Agent(13-18)
sample-app/workflow_example.go (1)
traceloop-sdk/tracing_types.go (1)
Prompt(12-23)
🪛 GitHub Actions: CI
traceloop-sdk/agent.go
[error] 49-49: undefined: semconvai.LLMAgentName. Command causing failure: 'find . -name go.mod -execdir go test ./...'.
🔇 Additional comments (5)
sample-app/generate_joke_workflow_example.go (1)
245-297: LGTM!The workflow orchestration is well-structured with proper error handling, early returns on errors, and appropriate cleanup using defer.
sample-app/recipe_agent_example.go (1)
224-330: LGTM overall!The agent orchestration is well-structured with proper initialization, cleanup, and non-fatal error handling for tools. Good use of defer and clear step-by-step execution flow.
traceloop-sdk/workflow.go (3)
28-35: LGTM!The ABTest variant propagation logic is correct: it checks for nil, finds the first active variant, sets the appropriate span attribute, and breaks early.
48-54: LGTM!The updated
LogPromptsignature simplifies the API by removing the error return, and correctly constructsContextAttributeswith workflow name and association properties.
56-70: LGTM!Using
string(model.SpanKindTask)instead of a hardcoded string is a good improvement for consistency and maintainability.
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 5d3cf19 in 2 minutes and 25 seconds. Click for details.
- Reviewed
163lines of code in5files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sample-app/recipe_agent_example.go:18
- Draft comment:
Corrected 'VarientsKeys' to 'VarientKeys'. Consider renaming further to 'VariantKeys' for clearer intent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is about a change that was made in the diff (line 18). However, the fieldVarientKeysis part of themodel.ABTeststruct which is imported from an external package. The PR author cannot rename this field unless they also modify the external package definition. This appears to be a struct field that's defined elsewhere, not in this file. The comment is suggesting a rename that the author likely cannot implement without changing the underlying model definition. This violates the rule about making actionable comments - the author can't act on this suggestion in this PR. I might be wrong about whether the author controls themodel.ABTesttype. Perhaps this is defined in the same codebase and the author could change it. However, based on the import path, it appears to be from an external SDK package. Even if the model is in the same codebase, the comment is asking for a code quality improvement (fixing a typo in a field name) that would require changes beyond this file. The rules state to ignore cross-file issues and only think about the file being reviewed. Additionally, if this were a real issue, it would be caught by the build since the field name would need to match the struct definition. This comment should be deleted. The field nameVarientKeysis part of an external struct type (model.ABTest), and the author cannot rename it without modifying the model definition elsewhere. This is a cross-file issue that's out of scope for this review, and any mismatch would be caught by the build.
2. sample-app/recipe_agent_example.go:170
- Draft comment:
Replaced inline association properties with the global 'associationProperties' variable for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only describes what was done without providing any actionable feedback or suggestions. It does not ask for confirmation or suggest any improvements.
3. traceloop-sdk/agent.go:27
- Draft comment:
Including the agent name in the context attributes (AgentName) improves trace context clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that including the agent name improves clarity, which is not aligned with the rules for comments.
4. traceloop-sdk/workflow.go:29
- Draft comment:
Updated loop to iterate over ABTest's 'VarientKeys' consistently; ensure spelling aligns with related changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the spelling aligns with related changes. This falls under the category of asking the author to double-check things, which is against the rules. Therefore, this comment should be removed.
5. sample-app/recipe_agent_example.go:18
- Draft comment:
Typo: 'VarientKeys' appears to be a misspelling. It likely should be 'VariantKeys'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The diff shows the field was changed fromVarientsKeystoVarientKeys(removing the 's' but keeping the misspelling). This is clearly about a change in the diff. The comment is pointing out that "Varient" is a misspelling of "Variant". However, this is a field name in the model.ABTest struct, which is defined in an external package (github.com/traceloop/go-openllmetry/traceloop-sdk/model). If this is the actual field name in that struct, then changing it would cause a compilation error. The code currently compiles (or at least, we assume it does), so either the field really is named 'VarientKeys' in the external package, or this would be caught by the build. Since this involves an external struct field name, I cannot verify if this is actually a typo without seeing the model package definition. I'm assuming this would be caught by the build if it's wrong, but what if the external package actually has this typo in its API? In that case, the comment would be pointing out a real issue that the author should be aware of. However, the rules state I should not comment on things that would be obviously caught by the build, and a wrong field name would definitely fail compilation. While the external package might have a typo in its API, if the field name is wrong, the code won't compile, which means this would be caught by the build. The rules explicitly state not to comment on things that would be obviously caught by the build. Since we can't see the external package definition and a wrong field name would cause a compilation error, this comment should be deleted. This comment should be deleted because if the field name is incorrect, it would cause a compilation error and be caught by the build. The rules explicitly state not to comment on issues that would be obviously caught by the build.
6. traceloop-sdk/model/types.go:14
- Draft comment:
Typographical error: The word 'VarientKeys' (and corresponding JSON tag 'varient_keys') appears to be misspelled. It should likely be 'VariantKeys' (and 'variant_keys'). Please verify and correct the spelling. - Reason this comment was not posted:
Marked as duplicate.
7. traceloop-sdk/sdk.go:154
- Draft comment:
Typographical error: The variable name "activeVarient" appears to be misspelled. Consider renaming it to "activeVariant" for clarity. Also, "VarientKeys" might be intended as "VariantKeys"; please verify the correct spelling. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment is pointing out a typo inactiveVarientandVarientKeys. Looking at the diff, the only change on line 154 is fromVarientsKeystoVarientKeys(removing an 's'). The variable nameactiveVarientis not being changed - it already existed. The comment is suggesting to fix spelling issues, but these are in code that wasn't modified by this PR (the variable name), or the field name which WAS changed but the comment doesn't acknowledge that change correctly. The comment treats this as if it's existing code that should be fixed, but the rule states comments should only be about changes made in the diff. SinceactiveVarientwasn't changed, and the field name change fromVarientsKeystoVarientKeysisn't what the comment is really addressing (it's suggestingVariantKeysinstead), this comment is not about the actual changes made. Wait, the field name WAS changed in the diff fromVarientsKeystoVarientKeys. So the comment could be interpreted as being about that change, suggesting it should have been changed toVariantKeysinstead. That would make it about the changes. However, the comment also mentionsactiveVarientwhich wasn't changed at all. While the field name was changed in the diff, the comment is primarily framed as a general typo correction ("appears to be misspelled", "please verify") rather than specifically addressing the change that was made. The comment also focuses onactiveVarientwhich wasn't changed. This seems more like a general code quality comment about existing code rather than a comment specifically about the PR changes. The rules state not to comment on things that aren't being changed. This comment should be deleted. While there is a change to the field name in the diff (VarientsKeys -> VarientKeys), the comment is framed as a general typo correction and also mentions the variableactiveVarientwhich wasn't changed in this PR. The comment doesn't clearly address the specific change made.
8. traceloop-sdk/workflow.go:29
- Draft comment:
Typographical note: The variable name "activeVarient" appears to contain a misspelling. Consider renaming it to "activeVariant", and also verify if "VarientKeys" should be "VariantKeys" for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
9. traceloop-sdk/workflow.go:82
- Draft comment:
Typo(s) detected on this line: 1. The variable name "activeVarient" likely should be "activeVariant" (note the misplaced 'i'). 2. The field name "VarientKeys" might be intended to be "VariantKeys". Please verify the correct spelling. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_dHyctQS1NVRt6rpj
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
traceloop-sdk/model/types.go (1)
12-14: Fix spelling: "varient" should be "variant".The comment and struct field use "varient" instead of the correct spelling "variant". This typo appears in the comment (line 12), the field name
VarientKeys(line 14), and the JSON tagvarient_keys.Apply this diff to correct the spelling:
-// The varient that is active will be added to the trace. +// The variant that is active will be added to the trace. type ABTest struct { - VarientKeys map[string]bool `json:"varient_keys"` + VariantKeys map[string]bool `json:"variant_keys"` }traceloop-sdk/sdk.go (3)
153-160: Guard against nilAssociationPropertiesbefore writing.If
agentAttrs.AssociationPropertiesis nil, line 156 will panic when attempting to write the"ab_testing_variant"key. This issue was flagged in a previous review but remains unresolved.Apply this diff to initialize the map if nil:
if agentAttrs.ABTest != nil { + if agentAttrs.AssociationProperties == nil { + agentAttrs.AssociationProperties = make(map[string]string) + } for key, activeVarient := range agentAttrs.ABTest.VarientKeys { if activeVarient { agentAttrs.AssociationProperties["ab_testing_variant"] = key
238-242: Add missingTraceloopEntityNameattribute to agent spans.The
LogAgentmethod emits spans withTraceloopWorkflowName,TraceloopSpanKind, andLLMAgentName, but is missingTraceloopEntityName. Tool spans (line 218) and the standaloneNewAgent(line 149) both include this attribute. Without it, agent spans logged viaLogAgentlack consistency with other span types.Apply this diff:
spanAttrs := []attribute.KeyValue{ semconvai.TraceloopWorkflowName.String(workflowAttrs.Name), semconvai.TraceloopSpanKind.String(string(model.SpanKindAgent)), + semconvai.TraceloopEntityName.String(attrs.Name), semconvai.LLMAgentName.String(attrs.Name), }
154-154: Fix typo: "activeVarient" should be "activeVariant".Apply this diff:
- for key, activeVarient := range agentAttrs.ABTest.VarientKeys { + for key, activeVariant := range agentAttrs.ABTest.VarientKeys { - if activeVarient { + if activeVariant {traceloop-sdk/workflow.go (2)
81-87: Avoid mutating the caller'sassociationPropertiesmap.Line 84 directly writes to the input
associationPropertiesmap, creating an unexpected side effect for callers who may reuse the map. This issue was flagged in a previous review. Additionally, line 82 contains the typo "activeVarient" instead of "activeVariant".Apply this diff to create a copy before modification:
+ // Create a copy to avoid mutating the caller's map + agentAssociationProps := make(map[string]string, len(associationProperties)) + for k, v := range associationProperties { + agentAssociationProps[k] = v + } + if workflow.Attributes.ABTest != nil { - for key, activeVarient := range workflow.Attributes.ABTest.VarientKeys { - if activeVarient { - associationProperties["ab_testing_variant"] = key + for key, activeVariant := range workflow.Attributes.ABTest.VariantKeys { + if activeVariant { + agentAssociationProps["ab_testing_variant"] = key } } } // Add agent-specific association properties to the span - for key, value := range associationProperties { + for key, value := range agentAssociationProps { attrs = append(attrs, attribute.String("traceloop.association.properties."+key, value)) } span.SetAttributes(attrs...) return &Agent{ sdk: workflow.sdk, workflow: workflow, ctx: aCtx, Attributes: AgentAttributes{ Name: name, - AssociationProperties: associationProperties, + AssociationProperties: agentAssociationProps, }, }
75-79: Add missingLLMAgentNameattribute to workflow agent spans.Agent spans created via
NewAgenton the workflow should include theLLMAgentNamesemantic attribute for consistency with the standaloneNewAgent(sdk.go line 150) andLogAgent(sdk.go line 241). Without it, workflow-based agent spans lack the agent name label.Apply this diff:
attrs := []attribute.KeyValue{ semconvai.TraceloopWorkflowName.String(workflow.Attributes.Name), semconvai.TraceloopSpanKind.String(string(model.SpanKindAgent)), + semconvai.LLMAgentName.String(name), semconvai.TraceloopEntityName.String(name), }sample-app/recipe_agent_example.go (4)
87-87: Guard against emptyresp.Choicesbefore indexing.Line 87 accesses
resp.Choices[0]without verifying the slice is non-empty, which will panic if the API returns no choices.Apply this diff:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nil
153-153: Guard against emptyresp.Choicesbefore indexing.Line 153 accesses
resp.Choices[0]without verifying the slice is non-empty, which will panic if the API returns no choices.Apply this diff:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nil
219-219: Guard against emptyresp.Choicesbefore indexing.Line 219 accesses
resp.Choices[0]without verifying the slice is non-empty, which will panic if the API returns no choices.Apply this diff:
+ if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } + return resp.Choices[0].Message.Content, nil
297-298: Guard against emptyresp.Choicesbefore indexing.Line 297 accesses
resp.Choices[0]without verifying the slice is non-empty, which will panic if the API returns no choices.Apply this diff:
+ if len(resp.Choices) == 0 { + fmt.Println("Error: CreateChatCompletion returned no choices") + return + } + recipe := resp.Choices[0].Message.Content
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
sample-app/recipe_agent_example.go(1 hunks)traceloop-sdk/agent.go(1 hunks)traceloop-sdk/model/types.go(1 hunks)traceloop-sdk/sdk.go(2 hunks)traceloop-sdk/workflow.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- traceloop-sdk/agent.go
🧰 Additional context used
🧬 Code graph analysis (3)
traceloop-sdk/sdk.go (5)
traceloop-sdk/tracing_types.go (5)
AgentAttributes(69-73)Prompt(12-23)ContextAttributes(36-40)ToolCallAttributes(65-67)WorkflowAttributes(30-34)traceloop-sdk/agent.go (1)
Agent(13-18)semconv-ai/attributes.go (7)
TraceloopSpanKind(33-33)TraceloopEntityName(35-35)LLMAgentName(26-26)LLMVendor(7-7)LLMRequestModel(9-9)LLMRequestType(8-8)TraceloopWorkflowName(34-34)traceloop-sdk/model/types.go (3)
SpanKindAgent(7-7)ABTest(13-15)SpanKindTool(6-6)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
traceloop-sdk/workflow.go (4)
semconv-ai/attributes.go (3)
TraceloopSpanKind(33-33)TraceloopEntityName(35-35)TraceloopWorkflowName(34-34)traceloop-sdk/model/types.go (4)
SpanKindWorkflow(9-9)ABTest(13-15)SpanKindTask(8-8)SpanKindAgent(7-7)traceloop-sdk/tracing_types.go (3)
Prompt(12-23)ContextAttributes(36-40)AgentAttributes(69-73)traceloop-sdk/agent.go (1)
Agent(13-18)
sample-app/recipe_agent_example.go (4)
traceloop-sdk/model/types.go (1)
ABTest(13-15)traceloop-sdk/tracing_types.go (6)
ToolFunction(48-52)Prompt(12-23)Message(5-10)Completion(25-28)Usage(42-46)AgentAttributes(69-73)traceloop-sdk/sdk.go (1)
NewClient(32-45)traceloop-sdk/config.go (1)
Config(9-17)
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2e9a52c in 1 minute and 22 seconds. Click for details.
- Reviewed
71lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. traceloop-sdk/agent.go:55
- Draft comment:
Good inclusion: The workflow name is now added as an attribute when agent.workflow is non-nil. This ensures tool spans correctly reference the parent workflow. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. traceloop-sdk/sdk.go:153
- Draft comment:
Nice defensive programming: Checking if AssociationProperties is nil before using it prevents nil map writes when setting AB test variants. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. traceloop-sdk/workflow.go:85
- Draft comment:
Consider breaking out of the loop after setting 'ab_testing_variant' to mirror behavior in sdk.NewAgent. Without a break, multiple active variants (if present) will result in the last one overwriting previous values. - Reason this comment was not posted:
Comment was on unchanged code.
4. traceloop-sdk/workflow.go:82
- Draft comment:
Using maps.Copy to duplicate the associationProperties map is neat; just ensure that the project’s Go version (1.21+) supports the 'maps' package. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_MsI0X4C3b2PAnknX
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
traceloop-sdk/workflow.go (1)
76-80: Add missingLLMAgentNameattribute to workflow agent spans.For consistency with
sdk.NewAgent(line 150) andsdk.LogAgent(line 244), agent spans created through the workflow should also include thesemconvai.LLMAgentName.String(name)attribute. This ensures uniform agent labeling across both standalone and workflow-bound agent creation paths.Apply this diff:
attrs := []attribute.KeyValue{ semconvai.TraceloopWorkflowName.String(workflow.Attributes.Name), semconvai.TraceloopSpanKind.String(string(model.SpanKindAgent)), + semconvai.LLMAgentName.String(name), semconvai.TraceloopEntityName.String(name), }sample-app/recipe_agent_example.go (3)
135-154: Guard against emptyChoicesslice before indexing.Line 153 accesses
resp.Choices[0]without verifying the slice is non-empty. If the OpenAI API returns no choices, this will panic at runtime.Apply this guard before line 153:
}) + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } return resp.Choices[0].Message.Content, nil
279-298: Guard against emptyChoicesslice before indexing.Line 297 accesses
resp.Choices[0]without verifying the slice is non-empty. If the OpenAI API returns no choices, this will panic at runtime during the main recipe generation flow.Apply this guard before line 297:
}) + if len(resp.Choices) == 0 { + fmt.Println("Error: CreateChatCompletion returned no choices") + return + } + recipe := resp.Choices[0].Message.Content
69-88: Guard against emptyChoicesslice before indexing.Line 87 accesses
resp.Choices[0]without verifying the slice is non-empty. If the OpenAI API returns no choices, this will panic at runtime.Apply this guard before line 87:
}) + + if len(resp.Choices) == 0 { + return "", fmt.Errorf("CreateChatCompletion returned no choices") + } return resp.Choices[0].Message.Content, niltraceloop-sdk/sdk.go (1)
241-245: Add missingTraceloopEntityNameattribute to agent spans.
LogAgentshould includesemconvai.TraceloopEntityName.String(attrs.Name)for consistency withNewAgent(line 149) andLogToolCall(line 221). Without it, downstream filters and dashboards that rely on the entity key will not work correctly for agents logged via this method.Apply this diff:
spanAttrs := []attribute.KeyValue{ semconvai.TraceloopWorkflowName.String(workflowAttrs.Name), semconvai.TraceloopSpanKind.String(string(model.SpanKindAgent)), + semconvai.TraceloopEntityName.String(attrs.Name), semconvai.LLMAgentName.String(attrs.Name), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
sample-app/recipe_agent_example.go(1 hunks)traceloop-sdk/agent.go(1 hunks)traceloop-sdk/model/types.go(1 hunks)traceloop-sdk/sdk.go(2 hunks)traceloop-sdk/workflow.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- traceloop-sdk/agent.go
🧰 Additional context used
🧬 Code graph analysis (3)
traceloop-sdk/sdk.go (5)
traceloop-sdk/tracing_types.go (5)
AgentAttributes(69-73)Prompt(12-23)ContextAttributes(36-40)ToolCallAttributes(65-67)WorkflowAttributes(30-34)traceloop-sdk/agent.go (1)
Agent(13-18)semconv-ai/attributes.go (7)
TraceloopSpanKind(33-33)TraceloopEntityName(35-35)LLMAgentName(26-26)LLMVendor(7-7)LLMRequestModel(9-9)LLMRequestType(8-8)TraceloopWorkflowName(34-34)traceloop-sdk/model/types.go (3)
SpanKindAgent(7-7)ABTest(13-15)SpanKindTool(6-6)traceloop-sdk/llm_span.go (1)
LLMSpan(10-12)
sample-app/recipe_agent_example.go (5)
traceloop-sdk/model/types.go (1)
ABTest(13-15)traceloop-sdk/agent.go (1)
Agent(13-18)traceloop-sdk/tracing_types.go (6)
ToolFunction(48-52)Prompt(12-23)Message(5-10)Completion(25-28)Usage(42-46)AgentAttributes(69-73)traceloop-sdk/sdk.go (1)
NewClient(32-45)traceloop-sdk/config.go (1)
Config(9-17)
traceloop-sdk/workflow.go (5)
semconv-ai/attributes.go (3)
TraceloopSpanKind(33-33)TraceloopEntityName(35-35)TraceloopWorkflowName(34-34)traceloop-sdk/model/types.go (4)
SpanKindWorkflow(9-9)ABTest(13-15)SpanKindTask(8-8)SpanKindAgent(7-7)traceloop-sdk/tracing_types.go (2)
ContextAttributes(36-40)AgentAttributes(69-73)traceloop-sdk/task.go (1)
Task(9-13)traceloop-sdk/agent.go (1)
Agent(13-18)
🔇 Additional comments (8)
traceloop-sdk/sdk.go (3)
143-178: LGTM: Standalone agent creation properly handles ABTest and attributes.The nil guard for
AssociationProperties, correct variant extraction with break, and complete attribute set (including both entity and agent name) are all properly implemented.
181-211: LGTM: LogPrompt correctly implements the new ContextAttributes-based API.The conditional workflow and agent name handling, along with association property propagation, aligns well with the refactored signature.
214-234: LGTM: LogToolCall correctly sets all required attributes.Entity name, span kind, and workflow name are all properly set, with association properties correctly propagated.
traceloop-sdk/model/types.go (1)
1-15: LGTM: Clean type definitions for span classification and AB testing.The
SpanKindconstants andABTeststruct provide a solid foundation for the tracing enhancements. Spelling has been corrected from previous reviews.traceloop-sdk/workflow.go (3)
29-36: LGTM: ABTest variant propagation correctly implements early-exit pattern.The loop correctly identifies the first active variant and breaks, avoiding potential overwrites.
49-55: LGTM: LogPrompt correctly delegates with workflow context.The
ContextAttributesconstruction properly carries workflow name and association properties to the SDK logging method.
82-83: LGTM: Proper map cloning avoids caller side effects.Using
maps.Copywith a pre-allocated capacity correctly prevents mutation of the caller'sassociationPropertiesmap.sample-app/recipe_agent_example.go (1)
17-22: LGTM: ABTest configuration uses correct field names.The
VariantKeysfield name correctly matches themodel.ABTesttype definition.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6f6e7c0 in 58 seconds. Click for details.
- Reviewed
153lines of code in6files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. sample-app/recipe_agent_example.go:17
- Draft comment:
Refactor: Updated 'abTest' variable to use semconvai.ABTest instead of the old model.ABTest. Ensure that the JSON field 'variant_keys' remains compatible with prior data. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. traceloop-sdk/agent.go:50
- Draft comment:
Refactor: Replaced model.SpanKindTool with semconvai.SpanKindTool for setting span kind. Confirm that the string conversion is appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. traceloop-sdk/sdk.go:148
- Draft comment:
Update: Changed span attribute in NewAgent to use semconvai.SpanKindAgent instead of model.SpanKindAgent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. traceloop-sdk/sdk.go:220
- Draft comment:
Update: In LogToolCall, replaced model.SpanKindTool with semconvai.SpanKindTool for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. traceloop-sdk/sdk.go:240
- Draft comment:
Update: In LogAgent, replaced model.SpanKindAgent with semconvai.SpanKindAgent to align with the refactor. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. traceloop-sdk/tracing_types.go:33
- Draft comment:
Refactor: Changed the ABTest field type in WorkflowAttributes from model.ABTest to semconvai.ABTest. Verify JSON compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. traceloop-sdk/tracing_types.go:70
- Draft comment:
Refactor: Updated AgentAttributes to use semconvai.ABTest instead of model.ABTest, ensuring consistency across types. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. traceloop-sdk/workflow.go:24
- Draft comment:
Update: In NewWorkflow, replaced model.SpanKindWorkflow with semconvai.SpanKindWorkflow for the span attribute. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. traceloop-sdk/workflow.go:57
- Draft comment:
Update: In NewTask, replaced model.SpanKindTask with semconvai.SpanKindTask to ensure consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. traceloop-sdk/workflow.go:73
- Draft comment:
Update: In the workflow's NewAgent method, updated the span attribute to use semconvai.SpanKindAgent for consistency with the new type. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_dpv6w7Z9GUTAS27s
You can customize
Fixes TLP-1096
Important
Enhances logging and observability in workflows and agents with new examples and refactored SDK.
generate_joke_workflow_example.goandrecipe_agent_example.goto demonstrate multi-step LLM flows and tool usage.runJokeWorkflow()andrunRecipeAgent()inmain.goto execute new examples.traceloop-sdkby introducingAgent,Tool,Task, andWorkflowabstractions.LLMSpanfor logging prompts and completions inllm_span.go.LogPrompt()andLogCompletion()methods acrossAgent,Tool,Task, andWorkflowclasses.ContextAttributesandWorkflowAttributesfor better context management.LLMAgentNameattribute insemconv-ai/attributes.go..gitignore).tool_calling.goandworkflow_example.goto align with new logging structure.This description was created by
for 6f6e7c0. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Release Notes
New Features
Refactor