adds support for gemini format (native and openai) #19
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request introduces Gemini API provider support to the logging infrastructure. Changes include a new dependency (godotenv), a provider constant, dual-format response parsing logic (native Gemini and OpenAI-compatible), integration with existing Generation and ParseResult workflows, and comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Generation
participant Parser
participant GeminiParser
participant Result
Client->>Generation: SetResult(providerGemini, jsonData)
activate Generation
Generation->>Parser: ParseResult(providerGemini, jsonData)
activate Parser
Parser->>GeminiParser: ParseGeminiResult(jsonData)
activate GeminiParser
rect rgb(200, 220, 240)
Note over GeminiParser: Try OpenAI format first
GeminiParser->>GeminiParser: Unmarshal as MaximLLMResult
end
alt OpenAI format found
GeminiParser->>Result: Return parsed result
else Parse native Gemini format
rect rgb(240, 220, 200)
Note over GeminiParser: Parse native Gemini response
GeminiParser->>GeminiParser: Extract candidates & content
GeminiParser->>GeminiParser: Map finish reasons & roles
GeminiParser->>GeminiParser: Extract tool calls
end
GeminiParser->>Result: Return transformed MaximLLMResult
end
deactivate GeminiParser
Parser->>Generation: Return parsed result
deactivate Parser
Generation->>Client: Return Generation with parsed result
deactivate Generation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
akshaydeo
force-pushed
the
10-30-adds_support_for_gemini_format_native_and_openai_
branch
from
dd15d27 to
9e07b3e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
logging/gemni.go (5)
12-24: OpenAI format detection could be more robust.The detection logic only checks if
Choicesis non-empty after unmarshaling. Consider:
- An empty
Choicesarray in valid OpenAI format will incorrectly fall through to native parsing- The unmarshal error is silently ignored - valid OpenAI responses that fail to unmarshal won't be reported
Apply this diff to improve format detection:
func ParseGeminiResult(jsonData []byte) (*MaximLLMResult, error) { // First, try to parse as OpenAI-compatible format var openAIResp MaximLLMResult if err := json.Unmarshal(jsonData, &openAIResp); err == nil { - // Check if it looks like an OpenAI format (has choices array) - if len(openAIResp.Choices) > 0 { + // Check if it looks like an OpenAI format (has id or choices field) + if openAIResp.ID != "" || len(openAIResp.Choices) > 0 { return &openAIResp, nil } } // If not OpenAI format, parse as native Gemini format return parseNativeGeminiResult(jsonData) }
64-64: ID generation could produce collisions in high-throughput scenarios.Using
time.Now().UnixNano()for ID generation can produce duplicate IDs if multiple requests complete in the same nanosecond.Consider using a UUID library (already imported in the project) or combining timestamp with a counter:
- resp.ID = fmt.Sprintf("gemini-%d", time.Now().UnixNano()) + resp.ID = fmt.Sprintf("gemini-%d-%s", time.Now().Unix(), uuid.New().String()[:8])Note: This requires importing
"github.com/google/uuid"at the top of the file.
101-104: Text concatenation may merge words incorrectly.Concatenating text parts without a separator (line 103) could merge the last word of one part with the first word of the next part, creating invalid text.
Consider whether parts should be joined with a space or newline:
for partIdx, part := range candidate.Content.Parts { if part.Text != "" { + if fullContent != "" { + fullContent += " " + } fullContent += part.Text }
119-127: Tool call ID generation could produce collisions across responses.The ID format
call_{i}_{partIdx}only uses indices, which will create duplicate IDs across different API responses.Include response-level uniqueness:
- toolCall := ChatCompletionToolCall{ - ID: fmt.Sprintf("call_%d_%d", i, partIdx), - Type: "function", + toolCall := ChatCompletionToolCall{ + ID: fmt.Sprintf("call_%s_%d_%d", resp.ID, i, partIdx), + Type: "function",Note: This assumes
resp.IDhas been set on line 64 before this code runs, which is currently the case.
88-95: Role mapping defaults to "assistant" for unrecognized roles.Lines 90-94 only explicitly handle "model" and "user" roles. Any other role value will default to "assistant" (line 89), which may not be the intended behavior.
Consider logging unknown roles or handling additional role types if Gemini supports them:
// Map Gemini role to OpenAI role format - role := "assistant" - if candidate.Content.Role == "model" { + var role string + switch candidate.Content.Role { + case "model": role = "assistant" - } else if candidate.Content.Role == "user" { + case "user": role = "user" + default: + // Default to assistant but could log unexpected values + role = "assistant" }logging/gemini_test.go (1)
206-292: Usex-goog-api-keyheader instead of URL query parameter for API authentication.Google Gemini API supports authentication via the
x-goog-api-keyrequest header as an alternative to URL query parameters. Update line 220 to use:req.Header.Set("x-goog-api-key", apiKey)and remove the API key from the URL to avoid exposure in logs and browser history.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.mod(1 hunks)logging/base.go(1 hunks)logging/gemini_test.go(1 hunks)logging/gemni.go(1 hunks)logging/generation.go(4 hunks)logging/parser.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
logging/gemini_test.go (2)
logging/gemni.go (1)
ParseGeminiResult(12-24)logging/generation.go (1)
Usage(120-124)
logging/gemni.go (1)
logging/generation.go (4)
MaximLLMResult(16-33)ChatCompletionToolCall(93-97)ToolCallFunction(88-91)Usage(120-124)
logging/generation.go (2)
logging/base.go (5)
ProviderOpenAI(10-10)ProviderAzure(11-11)ProviderBedrock(13-13)ProviderAnthropic(12-12)ProviderGemini(14-14)logging/gemni.go (1)
ParseGeminiResult(12-24)
logging/parser.go (2)
logging/base.go (1)
ProviderGemini(14-14)logging/gemni.go (1)
ParseGeminiResult(12-24)
🔇 Additional comments (7)
logging/base.go (1)
14-14: LGTM!The Gemini provider constant follows the established naming convention and integrates cleanly with the existing provider constants.
logging/parser.go (1)
27-28: LGTM!The Gemini case integrates properly into the existing ParseResult switch statement and follows the same pattern as other providers.
logging/generation.go (2)
228-231: LGTM!The
handleGeminiResultmethod follows the established pattern used by other provider handlers and properly delegates toParseGeminiResult.
269-270: LGTM!The Gemini case is properly integrated into the
SetResultswitch and follows the same pattern as other providers.logging/gemini_test.go (1)
17-344: Excellent test coverage!The test suite comprehensively covers:
- Native Gemini response format parsing
- OpenAI-compatible format parsing
- Function call handling
- Finish reason mappings
- Real API integration (when credentials available)
This provides strong confidence in the parsing implementation.
logging/gemni.go (1)
150-166: LGTM!The finish reason mapping comprehensively handles all known Gemini finish reasons and includes a sensible default case.
go.mod (1)
7-7: godotenv package is current and secure.Version 1.5.1 is the latest release, and no known security vulnerabilities are present. No action required.
| // Validating provider | ||
| if c.Provider != ProviderOpenAI && c.Provider != ProviderAzure && c.Provider != ProviderBedrock && c.Provider != ProviderAnthropic && c.Provider != ProviderGemini { | ||
| fmt.Printf("[MaximSDK] Invalid provider %s, allowed providers are %s, %s, %s, %s, %s\n", c.Provider, ProviderOpenAI, ProviderAzure, ProviderBedrock, ProviderAnthropic, ProviderGemini) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Provider validation uses stdout and returns nil on failure.
The new provider validation introduces breaking behavior:
- Returns
nilfor unrecognized providers (previously any string was accepted) - Uses
fmt.Printfto log to stdout instead of a proper logger or stderr
Consider whether this break in backward compatibility is intentional, and whether the error should be logged to stderr or returned as an error.
Verify whether there are existing calls to newGeneration with provider strings outside the allowed set:
🏁 Script executed:
#!/bin/bash
# Search for NewGeneration calls with provider parameters
# Looking for potential usage of providers other than the allowed ones
echo "=== Searching for Generation creation patterns ==="
rg -n -C3 'newGeneration|NewGeneration' --type=go
echo -e "\n=== Searching for provider string literals ==="
rg -n -C2 '"[a-z]+".*[Pp]rovider|[Pp]rovider.*"[a-z]+"' --type=go -g '!*_test.go'Length of output: 4293
I need to gather more context to verify the review concerns. Let me search for additional information about error handling patterns and test files.
I need to search for test files and understand the error handling patterns used in this codebase.The web search result wasn't helpful for understanding the specific codebase. Let me search for test files and error handling patterns in the codebase directly.
🏁 Script executed:
#!/bin/bash
# Search for test files related to generation
echo "=== Looking for generation test files ==="
fd -e 'go' '.*test.go' -g '*generation*'
echo -e "\n=== Searching for test files with provider tests ==="
rg -l 'TestGeneration|newGeneration' --type=go
echo -e "\n=== Searching for error handling patterns in the codebase ==="
rg -n 'return.*error|fmt.Errorf|GenerationError' --type=go | head -20
echo -e "\n=== Searching for how GenerationError is defined ==="
rg -B5 -A5 'GenerationError.*struct' --type=go
echo -e "\n=== Checking if provider validation existed before (searching for validation patterns) ==="
rg -n 'Provider.*!=' --type=goLength of output: 2836
🏁 Script executed:
#!/bin/bash
# Get the complete newGeneration function signature and full implementation
echo "=== Complete newGeneration function signature and initial lines ==="
rg -A30 '^func newGeneration' --type=go
echo -e "\n=== Checking how callers handle the return value ==="
rg -B1 -A3 'newGeneration\(c' --type=goLength of output: 3550
Fix nil pointer dereference and error handling in provider validation.
The review comment correctly identifies critical issues:
-
Nil pointer dereference panic: All four callers (
trace.go:39,span.go:28,logger.go:129,logger.go:273) immediately callg.data()on the returned pointer without nil checks. If an invalid provider is passed,newGenerationreturnsnil, causing a runtime panic in every caller. -
Non-idiomatic error handling: The codebase pattern (per
maxim.go,logger.go,writer.go) is to return errors from functions usingfmt.Errorf(), not to print to stdout and return nil. -
Breaking change: The validation enforces specific providers where any string was previously accepted.
Fix options:
- Change
newGenerationsignature to return(*Generation, error)and handle errors at callers, OR - Move validation to public methods and validate before calling
newGeneration, OR - Add nil checks at all four call sites (less ideal, as it shifts responsibility)
🤖 Prompt for AI Agents
In logging/generation.go around lines 169-173, newGeneration currently prints an
error and returns nil on invalid provider which causes nil pointer derefs in
callers and breaks idiomatic error handling; change newGeneration to return
(*Generation, error) instead of *Generation, replace the fmt.Printf call with
returning fmt.Errorf("invalid provider %s; allowed: %s, %s, %s, %s, %s",
c.Provider, ProviderOpenAI, ProviderAzure, ProviderBedrock, ProviderAnthropic,
ProviderGemini), and return (nil, err); then update all callers (trace.go:39,
span.go:28, logger.go:129, logger.go:273) to check and propagate or handle the
error (avoid calling g.data() on a nil receiver) so callers no longer panic.
Add support for Google Gemini models
TL;DR
Added support for Google's Gemini models in the Maxim logging SDK.
What changed?
ProviderGeminiconstant to the list of supported providershandleGeminiResultfunction to process Gemini API responsesgodotenvpackage dependencyHow to test?
Why make this change?
Google's Gemini models are becoming increasingly popular for AI applications. Adding support for Gemini allows users of the Maxim SDK to track and log interactions with these models alongside other providers like OpenAI, Azure, Anthropic, and AWS Bedrock.