fix: correct tool calling span attributes to match Python implementation #19
Conversation
- Fix tool call attribute naming to match Python OpenLLMetry - Use llm.completions.X.tool_calls.Y.name instead of function.name - Use llm.completions.X.tool_calls.Y.arguments instead of function.arguments - Remove incorrect 'type' attribute from tool calls - Fix double https:// issue in OTLP exporter URL construction - Add comprehensive test suite for span attribute validation - Add working tool calling sample applications - Fix OpenAI tool definition to include required Type field - Implement proper span duration using actual API call timing 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis update introduces tool calling sample applications and comprehensive support for tool calls in the Traceloop Go OpenLLMetry SDK. New sample code and documentation demonstrate tool integration with OpenAI's Go SDK. The tracing SDK's data model and logging are extended to capture tool definitions and tool call interactions, with expanded test coverage and upgraded dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SampleApp
participant OpenAI
participant Traceloop
User->>SampleApp: Run tool-calling example
SampleApp->>Traceloop: Log initial prompt (with tools)
SampleApp->>OpenAI: Send chat completion request (with tool)
OpenAI-->>SampleApp: Respond (may include tool call)
SampleApp->>SampleApp: Parse tool call, execute local function
SampleApp->>OpenAI: Send follow-up with tool result
OpenAI-->>SampleApp: Respond with final answer
SampleApp->>Traceloop: Log completion and tool call details
SampleApp->>User: Display assistant response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 3663aa4 in 1 minute and 58 seconds. Click for details.
- Reviewed
1629lines of code in16files - Skipped
0files when reviewing. - Skipped posting
5draft 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. tool-calling-sample/go.mod:3
- Draft comment:
The Go version in this module is set to 1.21 while the main sample uses 1.23.0. Consider aligning the Go versions across samples for consistency. - 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% Go version changes can have implications for compatibility and build requirements. Without seeing the other sample's go.mod, we can't verify if consistency is actually needed. Go 1.21 is a valid and recent version. Version alignment across samples isn't strictly necessary as each module can have its own requirements. The comment could be valid if there's a project-wide requirement to use Go 1.23.0. The suggestion might help maintain consistency across the codebase. However, we don't have enough context to verify this requirement, and Go 1.21 is a perfectly valid version choice. Each module can have its own Go version based on its specific needs. The comment should be deleted as it makes assumptions about version alignment requirements without clear evidence, and Go 1.21 is a valid choice.
2. traceloop-sdk/sdk.go:41
- Draft comment:
Avoid using panic in library initialization (e.g. in newTracerProvider). Instead, return an error so that client code can gracefully handle initialization issues. - 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.
3. traceloop-sdk/sdk.go:172
- Draft comment:
The JSON marshaling of prompts and completions ignores errors. It is advisable to handle potential errors from json.Marshal instead of silently discarding them. - 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 telemetry/tracing code, not core business logic. The data being marshaled is simple message structs. JSON marshaling of basic structs rarely fails unless there are cycles or channels. Even if marshaling fails, the span would still be created with all other attributes - just missing these two optional fields. The impact of failure would be minimal. The comment is technically correct - we are ignoring errors. In some contexts, ignoring JSON marshaling errors could hide real problems. While technically correct, this is an extremely low-risk scenario since we're marshaling simple structs for optional telemetry fields. Error handling would add complexity for little benefit. The comment should be removed. While technically accurate, handling these errors would add unnecessary complexity for minimal benefit in this telemetry context.
4. traceloop-sdk/utils.go:52
- Draft comment:
Consider handling the error returned by time.ParseDuration when setting the polling interval, rather than ignoring it with '_'. - 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.
5. traceloop-sdk/sdk_test.go:146
- Draft comment:
There's no newline at the end of the file. Consider adding a trailing newline for consistency. - 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% While having a newline at the end of files is a common convention in Unix-like systems and can prevent certain edge cases, this is a very minor stylistic issue. It doesn't affect the functionality of the code and would likely be caught by standard formatting tools like gofmt. The rules specifically say not to make comments that are obvious or unimportant. The missing newline could potentially cause issues with some tools or in some development environments. It is a real issue that some style guides explicitly check for. While true, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual review comments. It's too trivial for a PR comment. Delete the comment as it's too minor of an issue and would be better handled by automated formatting tools.
Workflow ID: wflow_hPCUF6MiFbLVmyjT
You can customize 
sample-app/tool_calling.go
Outdated
|
|
||
| traceloop := sdk.NewClient(config.Config{ | ||
| // BaseURL: os.Getenv("TRACELOOP_BASE_URL"), | ||
| APIKey: "tl_4be59d06bb644ced90f8b21e2924a31e", |
There was a problem hiding this comment.
Avoid hardcoding sensitive API keys (e.g. 'tl_4be59d06bb644ced90f8b21e2924a31e'). In sample code use environment variables instead to improve security.
| APIKey: "tl_4be59d06bb644ced90f8b21e2924a31e", | |
| APIKey: os.Getenv("TRACELOOP_API_KEY"), |
| @@ -13,10 +13,16 @@ import ( | |||
| ) | |||
|
|
|||
| func newOtlpExporter(ctx context.Context, endpoint string, apiKey string) (*otlp.Exporter, error) { | |||
| // OTLP client expects just the hostname, not the full URL | |||
| cleanEndpoint := endpoint | |||
| if len(endpoint) > 8 && endpoint[:8] == "https://" { | |||
There was a problem hiding this comment.
When cleaning the endpoint in newOtlpExporter, consider handling both 'https://' and 'http://' prefixes. Currently only 'https://' is stripped.
tool-calling-sample/README.md
Outdated
| @@ -0,0 +1,57 @@ | |||
| # Tool Calling Sample with OpenAI Go SDK | |||
|
|
|||
| This sample demonstrates how to use tool calling with the official OpenAI Go SDK and the Traceloop Go OpenLLMetry SDK for comprehensive observability. | |||
There was a problem hiding this comment.
Typo: In 'Traceloop Go OpenLLMetry SDK', consider verifying if 'OpenLLMetry' is the intended spelling or if it should be 'OpenTelemetry'.
| This sample demonstrates how to use tool calling with the official OpenAI Go SDK and the Traceloop Go OpenLLMetry SDK for comprehensive observability. | |
| This sample demonstrates how to use tool calling with the official OpenAI Go SDK and the Traceloop Go OpenTelemetry SDK for comprehensive observability. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
sample-app/main.go (1)
18-21: Propagate context into the tool-calling example for consistencyPassing ctx improves cancellation, deadlines, and logging parity with the regular path.
- runToolCallingExample() + runToolCallingExample(ctx) returnOutside this hunk, update the example function signature accordingly:
// in sample-app/tool_calling.go func runToolCallingExample(ctx context.Context) { // ... }sample-app/README.md (1)
21-29: Clarify that TRACELOOP_BASE_URL must include a scheme (https://)Since the SDK now uses BaseURL as provided, the URL must include http(s) to avoid request errors.
-export TRACELOOP_BASE_URL="https://api.traceloop.com" # Optional +export TRACELOOP_BASE_URL="https://api.traceloop.com" # Optional (must include http(s)://)Optionally, align the regular sample to use TRACELOOP_BASE_URL as well (instead of a hard-coded host) so docs and behavior match.
tool-calling-sample/README.md (3)
41-43: Update attribute naming: functions -> tools (request-side)Per PR objectives, request tool definitions should be logged under tools, not functions.
-### Request Tools -- Tool function names, descriptions, and parameters -- Logged with `llm.request.functions.{i}.*` attributes +### Request Tools +- Tool names, descriptions, and parameters +- Logged with `llm.request.tools.{i}.*` attributes
56-56: Add a language to the fenced code block (markdownlint MD040)Specify a language (text) for the example output block.
-``` +```text User: What's the weather like in San Francisco?--- `44-47`: **Update README: Remove “type” and Reference `.name`/`.arguments` for Response Tool Calls** Please adjust the “### Response Tool Calls” section in tool-calling-sample/README.md (lines 44–47) to remove the incorrect **type** attribute and explicitly call out the `.name` and `.arguments` fields. - Replace this block: ```diff ### Response Tool Calls - Tool call IDs, types, and function calls - Logged with `llm.completions.{i}.tool_calls.{j}.*` attributes
- With:
### Response Tool Calls - Tool call IDs and function calls (name and arguments) - Logged with `llm.completions.{i}.tool_calls.{j}.name` and `llm.completions.{i}.tool_calls.{j}.arguments`• Please verify that the runtime indeed emits
.nameand.arguments(and no longer emits atypefield) for eachtool_callsentry.sample-app/go.mod (1)
6-6: Alpha dependency risk (openai-go v0.1.0-alpha.35).Alpha tags can break without notice. If possible, pin to a stable release or add a comment explaining the alpha necessity for tool-calling support.
I can check for a stable tag with the required APIs if you want.
tool-calling-sample/go.mod (2)
3-3: Standardize Go version across modules.Other modules use go 1.23.0; this sample is at 1.21. Recommend upgrading to 1.23.0 for consistency.
-go 1.21 +go 1.23.0
6-6: Alpha dependency risk (openai-go).Same note as in sample-app: alpha tag may change. Keep if needed for tool calling, but consider documenting the reason.
traceloop-sdk/sdk_test.go (1)
93-99: Be resilient to multiple spans.As the SDK evolves (e.g., nested spans for tools), asserting exactly 1 span is brittle. Prefer asserting at least one span and selecting the most recent or filtering by attributes.
- spans := exporter.GetSpans() - if len(spans) != 1 { - t.Fatalf("Expected 1 span, got %d", len(spans)) - } - span := spans[0] + spans := exporter.GetSpans() + if len(spans) < 1 { + t.Fatalf("Expected at least 1 span, got %d", len(spans)) + } + // Use the last span; alternatively, filter by an identifying attribute. + span := spans[len(spans)-1]traceloop-sdk/dto/tracing.go (3)
21-26: Clarify handling of ToolCall.Type vs span attributes.PR objective says the
typeattribute was removed from tool call span attributes. KeepingToolCall.Typein the DTO is fine for provider fidelity, but ensure it’s not emitted asllm.completions.X.tool_calls.Y.typeon spans. A short comment here would prevent accidental reintroduction.Example comment to add above ToolCall:
// Type reflects provider payload fidelity (e.g., "function"). // Do not emit as a span attribute to align with cross-SDK semantics.Also applies to: 27-30
10-14: Prefer concrete JSON types for parameters.Using
interface{}hinders validation and marshalling guarantees. Consideranyorjson.RawMessagefor fidelity, ormap[string]anyif you need structural access.- Parameters interface{} `json:"parameters"` + Parameters any `json:"parameters"` + // Alternatively: json.RawMessage for exact JSON preservation.
62-68: Duration semantics: consider renaming or documenting units.
Duration intis ambiguous (ms? ns?). Either document units or switch to time.Duration and encode as ms in JSON with a custom marshal, to avoid confusion.tool-calling-sample/main.go (2)
140-147: Logged temperature does not match the requestYou log
Temperature: 0.7, yet theChatCompletionNewParamsset noTemperaturevalue (defaults to 1). This skews your trace data—either pass the same value to the API or log the actual default.
241-244: Placeholder hides real tool output in the trace
"Tool result for get_weather"is hard-coded instead of the actualresultstring, reducing observability value. Populate the real output so traces stay faithful.traceloop-sdk/sdk.go (1)
172-187: High-cardinality attributes riskStoring full prompt/completion JSON (
LLMPrompts,LLMCompletions) as span attributes can exceed OTLP size limits and blow up cardinality. Consider emitting them as span events or truncating long texts.sample-app/tool_calling.go (1)
26-40: Utility duplication across samples
convertOpenAIToolCallsToDTOis now duplicated in two samples; extract it into a shared helper to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
sample-app/go.sumis excluded by!**/*.sumtool-calling-sample/go.sumis excluded by!**/*.sumtraceloop-sdk/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
sample-app/README.md(1 hunks)sample-app/go.mod(1 hunks)sample-app/main.go(1 hunks)sample-app/tool_calling.go(1 hunks)tool-calling-sample/README.md(1 hunks)tool-calling-sample/go.mod(1 hunks)tool-calling-sample/main.go(1 hunks)traceloop-sdk/dto/tracing.go(2 hunks)traceloop-sdk/go.mod(2 hunks)traceloop-sdk/sdk.go(3 hunks)traceloop-sdk/sdk_test.go(1 hunks)traceloop-sdk/tracing.go(1 hunks)traceloop-sdk/utils.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
traceloop-sdk/sdk_test.go (3)
traceloop-sdk/sdk.go (1)
Traceloop(23-28)traceloop-sdk/config/config.go (1)
Config(9-14)traceloop-sdk/dto/tracing.go (9)
PromptLogAttributes(62-68)Prompt(32-43)Tool(16-19)ToolFunction(10-14)Message(3-8)Completion(45-48)ToolCall(21-25)ToolCallFunction(27-30)Usage(56-60)
tool-calling-sample/main.go (4)
traceloop-sdk/dto/tracing.go (9)
ToolCall(21-25)ToolCallFunction(27-30)Tool(16-19)ToolFunction(10-14)Message(3-8)PromptLogAttributes(62-68)Prompt(32-43)Completion(45-48)Usage(56-60)traceloop-sdk/sdk.go (1)
NewClient(30-36)traceloop-sdk/config/config.go (1)
Config(9-14)sample-app/tool_calling.go (1)
WeatherParams(17-20)
🪛 markdownlint-cli2 (0.17.2)
tool-calling-sample/README.md
56-56: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
traceloop-sdk/utils.go (1)
22-22: Default HTTPS scheme and safe URL joining implementedThe update now trims
BaseURL, defaults tohttps://if no scheme is present, parses it withnet/url, and safely appends the request path. This ensures instances configured with host-only values (e.g."api.traceloop.com") in your README, sample app, and tests continue working without modification.traceloop-sdk/tracing.go (1)
16-26: Robust OTLP endpoint normalization using url.ParseThe updated
cleanEndpointlogic correctly handles bothhttp://andhttps://, strips any paths, and removes trailing slashes. Verification confirms expected behavior for all cases:
https://api.traceloop.com→api.traceloop.comhttp://api.traceloop.com/→api.traceloop.comapi.traceloop.com:4318→api.traceloop.com:4318https://api.traceloop.com/v1/traces→api.traceloop.comLocation:
- traceloop-sdk/tracing.go (lines 16–26)
import ( - "context" - "fmt" + "context" + "fmt" "net/url" "strings" "go.opentelemetry.io/otel" otlp "go.opentelemetry.io/otel/exporters/otlp/otlptrace" otlpclient "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp" "go.opentelemetry.io/otel/sdk/resource" "go.opentelemetry.io/otel/sdk/trace" semconv "go.opentelemetry.io/otel/semconv/v1.17.0" ) func newOtlpExporter(ctx context.Context, endpoint string, apiKey string) (*otlp.Exporter, error) { - // OTLP client expects just the hostname, not the full URL - cleanEndpoint := endpoint - if len(endpoint) > 8 && endpoint[:8] == "https://" { - cleanEndpoint = endpoint[8:] // Remove https:// prefix - } + // Normalize endpoint to host[:port], dropping protocol, paths, and trailing slash. + cleanEndpoint := endpoint + if u, err := url.Parse(endpoint); err == nil && u.Host != "" { + cleanEndpoint = u.Host + } else { + cleanEndpoint = strings.TrimPrefix(cleanEndpoint, "https://") + cleanEndpoint = strings.TrimPrefix(cleanEndpoint, "http://") + } + cleanEndpoint = strings.TrimSuffix(cleanEndpoint, "/") return otlp.New( ctx, otlpclient.NewClient( otlpclient.WithEndpoint(cleanEndpoint), otlpclient.WithHeaders(map[string]string{ "Authorization": fmt.Sprintf("Bearer %s", apiKey), }), ), ) }sample-app/go.mod (2)
5-9: Confirm you really need both OpenAI clients.You require both github.com/openai/openai-go and github.com/sashabaranov/go-openai. If both are intentional (different samples), consider a brief README note to prevent confusion; otherwise, drop the unused one.
Do you want me to scan the repo for imports to confirm both are used and where?
56-58: Local replaces LGTM.Local path replaces for traceloop-sdk and semconv-ai look correct for iterative development.
traceloop-sdk/go.mod (1)
3-6: Toolchain pin LGTM.go 1.23.0 with toolchain go1.23.12 is fine and matches the rest of the repo direction.
tool-calling-sample/go.mod (1)
53-55: Local replaces LGTM.Replace directives look correct.
traceloop-sdk/sdk_test.go (2)
26-34: Test setup for in-memory tracing is solid.Using tracetest.InMemoryExporter with a custom tracer provider is appropriate for validating span attributes.
139-146: Consider asserting tool definition attributes, not just prompts/completions.If the SDK emits tool definitions on spans (e.g., llm.tools.*), add assertions to cover them. This prevents regressions in tool metadata logging.
Do you want me to scan the SDK for emitted tool attributes and propose exact assertions?
| go.opentelemetry.io/auto/sdk v1.1.0 // indirect | ||
| go.opentelemetry.io/otel v1.37.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.22.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.22.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.37.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.37.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.37.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.37.0 // indirect | ||
| go.opentelemetry.io/proto/otlp v1.0.0 // indirect |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align OpenTelemetry module versions (avoid mixing 1.37.0 with 1.22.0).
Core is at 1.37.0 but otlptrace/otlptracehttp are 1.22.0. This mismatch can cause API/type incompatibilities. Align all OTel modules to the same minor version.
Apply this diff to bump exporters:
- go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.22.0 // indirect
- go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.22.0 // indirect
+ go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.37.0 // indirect
+ go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.37.0 // indirectAlso applies to: 38-39
🤖 Prompt for AI Agents
In sample-app/go.mod around lines 36 to 44, the OpenTelemetry module versions
are inconsistent, mixing 1.37.0 and 1.22.0, which can cause compatibility
issues. Update the versions of go.opentelemetry.io/otel/exporters/otlp/otlptrace
and go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp from v1.22.0
to v1.37.0 to align all OpenTelemetry modules to the same minor version.
sample-app/tool_calling.go
Outdated
| traceloop := sdk.NewClient(config.Config{ | ||
| // BaseURL: os.Getenv("TRACELOOP_BASE_URL"), | ||
| APIKey: "tl_4be59d06bb644ced90f8b21e2924a31e", | ||
| }) | ||
| defer func() { traceloop.Shutdown(ctx) }() |
There was a problem hiding this comment.
Hard-coded Traceloop API key committed to repo
"tl_4be59d06bb644ced90f8b21e2924a31e" is a live credential. Commit history is immutable, so rotate the key immediately and switch to os.Getenv("TRACELOOP_API_KEY") like elsewhere.
[security]
🤖 Prompt for AI Agents
In sample-app/tool_calling.go around lines 96 to 100, the Traceloop API key is
hard-coded as a string literal, which is a security risk. Replace the hard-coded
API key with a call to os.Getenv("TRACELOOP_API_KEY") to load the key from
environment variables. Also, ensure the live key is rotated immediately to
prevent unauthorized access.
tool-calling-sample/go.mod
Outdated
| go.opentelemetry.io/otel v1.22.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.22.0 // indirect | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.22.0 // indirect | ||
| go.opentelemetry.io/otel/metric v1.22.0 // indirect | ||
| go.opentelemetry.io/otel/sdk v1.22.0 // indirect | ||
| go.opentelemetry.io/otel/trace v1.22.0 // indirect | ||
| go.opentelemetry.io/proto/otlp v1.0.0 // indirect |
There was a problem hiding this comment.
🛠️ Refactor suggestion
OTel versions here are 1.22.0; align with SDK’s 1.37.0 or avoid pinning indirects.
Keeping indirect 1.22.0 while SDK brings 1.37.0 can cause confusion. Prefer letting traceloop-sdk drive OTel versions, or explicitly align to 1.37.0.
Two options:
- Run: go get go.opentelemetry.io/otel@v1.37.0 go.opentelemetry.io/otel/exporters/otlp/otlptrace@v1.37.0 go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp@v1.37.0 go.opentelemetry.io/otel/sdk@v1.37.0; go mod tidy
- Or remove these indirect pins and tidy, so the SDK controls versions.
Also applies to: 36-40
🤖 Prompt for AI Agents
In tool-calling-sample/go.mod lines 35 to 41, the OpenTelemetry (OTel)
dependencies are pinned at version 1.22.0, which conflicts with the SDK's
version 1.37.0 and may cause confusion. To fix this, either update all these
OTel dependencies to version 1.37.0 by running the appropriate 'go get' commands
followed by 'go mod tidy', or remove these indirect version pins entirely and
run 'go mod tidy' to let the SDK manage the OTel versions consistently.
tool-calling-sample/main.go
Outdated
| func createWeatherTool() openai.ChatCompletionToolParam { | ||
| return openai.ChatCompletionToolParam{ | ||
| Function: openai.F(openai.FunctionDefinitionParam{ | ||
| Name: openai.F("get_weather"), | ||
| Description: openai.F("Get the current weather for a given location"), | ||
| Parameters: openai.F(openai.FunctionParameters{ | ||
| "type": "object", | ||
| "properties": map[string]interface{}{ | ||
| "location": map[string]interface{}{ | ||
| "type": "string", | ||
| "description": "The city and state, e.g. San Francisco, CA", | ||
| }, | ||
| "unit": map[string]interface{}{ | ||
| "type": "string", | ||
| "enum": []string{"C", "F"}, | ||
| "description": "The unit for temperature", | ||
| }, | ||
| }, | ||
| "required": []string{"location"}, | ||
| }), | ||
| }), | ||
| } |
There was a problem hiding this comment.
Missing Type field may break OpenAI schema
createWeatherTool omits the mandatory Type: "function" field. The official schema requires "type":"function" alongside the function block; without it the request is rejected by the OpenAI API.
Add the field as done in the other sample file.
return openai.ChatCompletionToolParam{
+ Type: openai.F(openai.ChatCompletionToolTypeFunction),
Function: openai.F(openai.FunctionDefinitionParam{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func createWeatherTool() openai.ChatCompletionToolParam { | |
| return openai.ChatCompletionToolParam{ | |
| Function: openai.F(openai.FunctionDefinitionParam{ | |
| Name: openai.F("get_weather"), | |
| Description: openai.F("Get the current weather for a given location"), | |
| Parameters: openai.F(openai.FunctionParameters{ | |
| "type": "object", | |
| "properties": map[string]interface{}{ | |
| "location": map[string]interface{}{ | |
| "type": "string", | |
| "description": "The city and state, e.g. San Francisco, CA", | |
| }, | |
| "unit": map[string]interface{}{ | |
| "type": "string", | |
| "enum": []string{"C", "F"}, | |
| "description": "The unit for temperature", | |
| }, | |
| }, | |
| "required": []string{"location"}, | |
| }), | |
| }), | |
| } | |
| func createWeatherTool() openai.ChatCompletionToolParam { | |
| return openai.ChatCompletionToolParam{ | |
| Type: openai.F(openai.ChatCompletionToolTypeFunction), | |
| Function: openai.F(openai.FunctionDefinitionParam{ | |
| Name: openai.F("get_weather"), | |
| Description: openai.F("Get the current weather for a given location"), | |
| Parameters: openai.F(openai.FunctionParameters{ | |
| "type": "object", | |
| "properties": map[string]interface{}{ | |
| "location": map[string]interface{}{ | |
| "type": "string", | |
| "description": "The city and state, e.g. San Francisco, CA", | |
| }, | |
| "unit": map[string]interface{}{ | |
| "type": "string", | |
| "enum": []string{"C", "F"}, | |
| "description": "The unit for temperature", | |
| }, | |
| }, | |
| "required": []string{"location"}, | |
| }), | |
| }), | |
| } | |
| } |
🤖 Prompt for AI Agents
In tool-calling-sample/main.go lines 45 to 66, the createWeatherTool function is
missing the mandatory Type field set to "function" in the
FunctionDefinitionParam. Add Type: openai.F("function") alongside the existing
fields inside the openai.FunctionDefinitionParam struct to comply with the
OpenAI schema and prevent API request rejection.
tool-calling-sample/main.go
Outdated
| var toolMessages []openai.ChatCompletionMessageParamUnion | ||
| toolMessages = append(toolMessages, openai.UserMessage(userPrompt)) | ||
| toolMessages = append(toolMessages, openai.AssistantMessage(resp.Choices[0].Message.Content)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assistant message sent without tool_calls payload
OpenAI expects the assistant message that triggers tools to contain an empty content and the tool_calls array. Using openai.AssistantMessage drops that array and can confuse the follow-up call.
Use an explicit ChatCompletionMessage that includes ToolCalls, similar to the approach in sample-app/tool_calling.go.
🤖 Prompt for AI Agents
In tool-calling-sample/main.go around lines 189 to 192, the assistant message is
created using openai.AssistantMessage which omits the required tool_calls
payload. To fix this, replace openai.AssistantMessage with an explicit
ChatCompletionMessage struct that sets content to an empty string and includes
the tool_calls array from the response, following the pattern used in
sample-app/tool_calling.go. This ensures the assistant message properly triggers
tool usage.
| go.opentelemetry.io/otel v1.37.0 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.22.0 | ||
| go.opentelemetry.io/otel/trace v1.22.0 | ||
| go.opentelemetry.io/otel/exporters/stdout/stdouttrace v1.37.0 | ||
| go.opentelemetry.io/otel/trace v1.37.0 | ||
| ) |
There was a problem hiding this comment.
Fix OTel version drift inside the SDK module.
You’re mixing OTel 1.37.0 (core/trace/stdout/sdk) with 1.22.0 (otlptrace, otlptracehttp). This is high-risk for runtime/type issues. Keep all to 1.37.0.
Apply this diff:
- go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.22.0
+ go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.37.0
@@
- go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.22.0
+ go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.37.0Also applies to: 47-49
🤖 Prompt for AI Agents
In traceloop-sdk/go.mod around lines 11 to 15 and also lines 47 to 49, the
OpenTelemetry dependencies have mixed versions: 1.37.0 for most modules but
1.22.0 for otlptrace. Update the version of
go.opentelemetry.io/otel/exporters/otlp/otlptrace and any other OTel modules at
lines 47-49 to 1.37.0 to ensure all OTel dependencies use the same version and
avoid runtime or type conflicts.
| // Assert on specific attributes | ||
| expectedAttrs := map[string]interface{}{ | ||
| "llm.vendor": "openai", | ||
| "llm.request.model": "gpt-4o-mini", | ||
| "llm.request.type": "chat", | ||
| "llm.response.model": "gpt-4o-mini-2024-07-18", | ||
| "llm.usage.total_tokens": int64(99), | ||
| "llm.usage.completion_tokens": int64(17), | ||
| "llm.usage.prompt_tokens": int64(82), | ||
| "llm.prompts.0.content": "What's the weather like in San Francisco?", | ||
| "llm.prompts.0.role": "user", | ||
| "llm.completions.0.content": "", | ||
| "llm.completions.0.role": "assistant", | ||
| "llm.completions.0.tool_calls.0.id": "call_YkIfypBQrmpUpxsKuS9aNdKg", | ||
| "llm.completions.0.tool_calls.0.name": "get_weather", | ||
| "llm.completions.0.tool_calls.0.arguments": `{"location":"San Francisco, CA"}`, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Great: asserts validate new naming (tool_calls.*.name/arguments). Add guards against old keys and type.
To enforce the migration, explicitly assert that legacy keys are absent: function.name, function.arguments, and drop tool_calls.*.type per PR objective.
for expectedKey, expectedValue := range expectedAttrs {
actualValue, exists := attributeMap[expectedKey]
if !exists {
t.Errorf("Expected attribute %s not found", expectedKey)
} else if actualValue != expectedValue {
t.Errorf("Attribute %s: expected %v, got %v", expectedKey, expectedValue, actualValue)
}
}
+
+ // Ensure legacy/incorrect attributes are not present
+ disallowed := []string{
+ "llm.completions.0.tool_calls.0.function.name",
+ "llm.completions.0.tool_calls.0.function.arguments",
+ "llm.completions.0.tool_calls.0.type",
+ }
+ for _, k := range disallowed {
+ if _, exists := attributeMap[k]; exists {
+ t.Errorf("Unexpected attribute present: %s", k)
+ }
+ }Also applies to: 130-137
🤖 Prompt for AI Agents
In traceloop-sdk/sdk_test.go around lines 112 to 128 and 130 to 137, the test
asserts new attribute keys like tool_calls.*.name and tool_calls.*.arguments but
does not check for the absence of legacy keys function.name, function.arguments,
and tool_calls.*.type. To enforce the migration, add assertions that these old
keys are not present in the attributes map, ensuring the test fails if legacy
keys remain. Remove any assertions or references to tool_calls.*.type as it
should be dropped per the PR objective.
| spanCtx, span := (*instance.tracerProvider).Tracer(os.Args[0]).Start( | ||
| ctx, | ||
| spanName, | ||
| apitrace.WithTimestamp(startTime), | ||
| ) |
There was a problem hiding this comment.
Possible nil-pointer dereference on tracerProvider
(*instance.tracerProvider).Tracer(...) panics if tracerProvider is nil (e.g., LogPrompt called before Initialize). Guard with a nil check or initialise lazily.
🤖 Prompt for AI Agents
In traceloop-sdk/sdk.go around lines 165 to 169, the code dereferences
instance.tracerProvider without checking if it is nil, which can cause a panic
if tracerProvider is not initialized. Add a nil check before dereferencing
instance.tracerProvider; if it is nil, either return an error, initialize it
lazily, or handle the case gracefully to avoid the panic.
…ng sample - Fixed BaseURL in main.go to use proper HTTPS protocol - Removed hardcoded API key from tool_calling.go, now uses environment variables - Deleted duplicate tool-calling-sample directory (functionality exists in sample-app) - Removed binary files from repository 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
The tool calling functionality is already implemented in sample-app/tool_calling.go, so the duplicate directory is not needed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b58867c in 1 minute and 15 seconds. Click for details.
- Reviewed
516lines of code in4files - Skipped
0files when reviewing. - Skipped posting
5draft 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. tool-calling-sample/README.md:1
- Draft comment:
The entire README for the sample has been removed. The commit message mentions adding working sample applications, so its removal is unexpected. Please confirm that this is intentional and that updated documentation (or a pointer to the new location) is provided. - 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.
2. tool-calling-sample/go.mod:1
- Draft comment:
The go.mod file for the sample was entirely deleted. This removal should be verified to ensure that dependency management and module references for any remaining samples or tests remain correct. - 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.
3. tool-calling-sample/go.sum:1
- Draft comment:
The go.sum file has been removed. Make sure that if the sample is being deprecated or relocated, dependencies are managed appropriately elsewhere. - 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.
4. tool-calling-sample/main.go:1
- Draft comment:
The main sample application has been completely removed. Since the commit message mentions corrections to tool call span attributes and adding comprehensive tests, it is unclear if these removals replace outdated samples or if updated examples exist elsewhere. Please ensure that the new improvements are demonstrated via suitable tests or updated samples. - 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.
5. General:0
- Draft comment:
The commit description also mentions fixes like correcting span attribute naming to match the Python implementation and addressing a double 'https://' issue in the OTLP exporter URL. However, these changes are not evident in the diff (only sample code removals are shown). Please verify that the intended fixes are included in this commit or provided in a separate PR. - 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.
Workflow ID: wflow_BK1EVRcVHqTBYLTP
You can customize
🤖 Generated with Claude Code
Important
Aligns tool call attributes with Python OpenLLMetry, adds tests, fixes URL handling, and updates dependencies.
llm.completions.X.tool_calls.Y.nameandllm.completions.X.tool_calls.Y.argumentsinstead offunction.nameandfunction.argumentsinsdk.go.sdk.go.https://issue in OTLP exporter URL construction intracing.go.sdk.go.sdk_test.go.sample-app/main.goandsample-app/tool_calling.go.go.modfiles.This description was created by
for b58867c. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Refactor
Tests