adds gorm logger - #830
Conversation
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds Bedrock V4 signer, GORM logger adapters, docker services (redis, weaviate), expanded config schema (cluster/saml/load_balancer/guardrails), chat-based test API migration, model catalog pool population, CI enhancements for UI/integration tests, and wiring for custom DB loggers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Bifrost HTTP Server
participant BootstrapFlow
participant ModelCatalog
participant PricingMgr
Client->>Server: Bootstrap request
Server->>BootstrapFlow: Initialize services
BootstrapFlow->>Server: listModels()
alt listModels() success
alt PricingMgr exists
Server->>ModelCatalog: AddModelDataToPool(modelData)
ModelCatalog->>PricingMgr: Populate model pool
else PricingMgr missing
Note over Server: Skip AddModelDataToPool
end
else listModels() error
Note over Server: Skip pool update
end
BootstrapFlow->>Server: Bootstrap complete
sequenceDiagram
participant App
participant GORMDB as GORM DB
participant GORMLogger as custom gormLogger
participant ProjectLogger as schemas.Logger
App->>GORMDB: Open DB with gorm.Config{Logger: newGormLogger}
GORMDB->>GORMLogger: emit log events (Info/Warn/Error)
GORMLogger->>ProjectLogger: forward logs
Note over GORMLogger: Trace and LogMode are NOOPs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
transports/config.schema.json (2)
495-497: Fix non-standard JSON pointer path in logs_store conditional.The relative path
"../type"is not valid JSON Schema syntax. JSON Schema conditionals should use absolute paths (starting with#/) or be restructured using standard patterns."if": { "properties": { - "../type": { + "type": { "const": "sqlite" } } },Note: If this was intended to check the parent's type field, consider restructuring the conditional logic outside the nested object, as JSON Schema's
if/then/elseworks at the object level where the conditional is defined.
438-438: Inconsistent port field types between config_store and logs_store requires alignment.The
portfield uses"type": "string"in config_store (line 436) but"type": "integer"in logs_store (line 527). This inconsistency breaks API consistency and could cause runtime type mismatches. The gossip port configuration also uses integer, confirming integer is the correct type.Align config_store to use
"type": "integer":"port": { - "type": "string", + "type": "integer", "description": "Database port" },tests/core-chatbot/main.go (1)
620-693: Add Provider and Model to synthesisRequest to match required struct fields and SendMessage pattern.The
BifrostChatRequeststruct definition requires bothProvider(non-pointerModelProvider) andModel(non-pointerstring) fields—they are not optional. The normalSendMessagepath at line 485-490 correctly includes both, but thesynthesisRequestat line 632-638 omits them, resulting in zero-valued fields. All test scenarios throughout the codebase consistently set both fields for every request.- synthesisRequest := &schemas.BifrostChatRequest{ - Input: conversationWithSynthesis, - Params: &schemas.ChatParameters{ - Temperature: s.config.Temperature, - MaxCompletionTokens: s.config.MaxTokens, - }, - } + synthesisRequest := &schemas.BifrostChatRequest{ + Provider: s.config.Provider, + Model: s.config.Model, + Input: conversationWithSynthesis, + Params: &schemas.ChatParameters{ + Temperature: s.config.Temperature, + MaxCompletionTokens: s.config.MaxTokens, + }, + }
🧹 Nitpick comments (10)
transports/config.schema.json (2)
1513-1580: Add conditional required fields based on discovery type.The discovery section only requires
type, but depending on the selected discovery mechanism, other fields should be conditionally required:
- Kubernetes: require
k8s_namespaceandk8s_label_selector- DNS: require
dns_names- UDP: require
udp_broadcast_port- Consul: require
consul_address- Etcd: require
etcd_endpoints- mDNS: require
mdns_serviceCurrently, invalid configurations like
{"type": "kubernetes"}with no Kubernetes-specific fields would pass schema validation.Consider adding oneOf conditionals (similar to the mcp_client_config pattern at lines 1331–1362) to enforce type-specific required fields.
1769-1771: Add required fields to load_balancer_config if tracker_config is enabled.The load_balancer_config only requires
enabled. Ifenabledis true,tracker_configshould likely be required. Consider adding conditional schema validation similar to the plugin config patterns (lines 604–875) to enforce this.framework/logstore/logger.go (1)
23-35: Context is not propagated to the internal logger.The
context.Contextparameter inInfo,Warn, andErrormethods is ignored. If your logging system supports context-aware logging (e.g., extracting request IDs, trace IDs, or other metadata from the context), this information will be lost.If
schemas.Loggersupports context-aware logging, consider propagating the context. For example, if there's aWithContextmethod or similar:func (l *gormLogger) Info(ctx context.Context, msg string, data ...interface{}) { - l.logger.Info(msg, data...) + // If schemas.Logger supports context, use it here + l.logger.Info(msg, data...) }framework/modelcatalog/main.go (1)
266-282: Consider deduplicating models when adding to the pool.The method appends models without checking for duplicates. If
AddModelDataToPoolis called multiple times with overlapping data, the same model could appear multiple times in the provider's slice, potentially leading to inefficiencies or incorrect behavior in downstream code.Consider adding duplicate checks or using a set-based approach:
func (mc *ModelCatalog) AddModelDataToPool(modelData *schemas.BifrostListModelsResponse) { if modelData == nil { return } mc.mu.Lock() defer mc.mu.Unlock() for _, model := range modelData.Data { provider, model := schemas.ParseModelString(model.ID, "") if provider == "" { continue } provider = schemas.ModelProvider(provider) - mc.modelPool[provider] = append(mc.modelPool[provider], model) + // Check for duplicates before appending + if !slices.Contains(mc.modelPool[provider], model) { + mc.modelPool[provider] = append(mc.modelPool[provider], model) + } } }framework/docker-compose.yml (1)
69-70: Remove unusedredis_datavolume declaration.The
redis_datavolume is declared but not mounted by any service. If Redis persistence is not needed, remove this declaration to avoid confusion.volumes: postgres_data: driver: local weaviate_data: driver: local - redis_data: - driver: local.github/workflows/test-coverage.yml (2)
59-67: Service health check could be more robust.The health check logic uses
grep -q "healthy"which might match partial states or be fragile. The|| trueat the end means failures are silently ignored, and the fixed 5-second sleep may not be sufficient for all services.Consider checking each service explicitly:
- name: Start services for integration tests run: | echo "Starting Redis and Weaviate for vector store tests..." cd framework docker-compose up -d # Wait for services to be healthy echo "Waiting for services to be ready..." - timeout 60 bash -c 'until docker-compose ps | grep -q "healthy"; do sleep 2; done' || true - sleep 5 + for i in {1..30}; do + if docker-compose ps | grep -E "bifrost-redis.*healthy" > /dev/null && \ + docker-compose ps | grep -E "bifrost-weaviate.*healthy" > /dev/null; then + echo "All services are healthy" + break + fi + echo "Waiting for services... ($i/30)" + sleep 2 + done + docker-compose ps
69-79: Plugin build failure is silently ignored.The plugin build uses
|| echo "Plugin build failed, tests will skip"which suppresses the error. If tests depend on the plugin, they might pass incorrectly or produce confusing results when the plugin is missing.Consider making plugin build failures more visible or explicitly checking if tests require the plugin:
- name: Rebuild plugins run: | echo "Rebuilding example plugins..." if [ -d "examples/plugins/hello-world" ]; then cd examples/plugins/hello-world # Clean old build rm -rf build mkdir -p build # Rebuild plugin with current dependencies - go build -buildmode=plugin -o build/hello-world.so main.go || echo "Plugin build failed, tests will skip" + if ! go build -buildmode=plugin -o build/hello-world.so main.go; then + echo "::warning::Plugin build failed - plugin tests may be skipped" + fi fitests/core-chatbot/main.go (3)
539-595: Tool-call handling uses the new tool role/message types correctly; consider richer rendering of results.
- Assigning
toolCalls := assistantMessage.ToolCallsand iterating directly over them is fine given the guard inSendMessagethat only callshandleToolCallswhenToolCallsis non-nil and non-empty.- The error-path
ChatMessagecorrectly usesRole: ChatMessageRoleTooland aChatToolMessagewithToolCallID: toolCall.ID, preserving the linkage between tool calls and their results.- Appending tool results into
historyas values is consistent with the rest of the code.One improvement: in the non-agentic path you only render
result.Content.ContentStr. If MCP tools ever return structuredContentBlocksinstead of (or in addition to)ContentStr, those would be silently ignored here. You could reuse the same content-extraction helper you use for standard assistant messages to handle both cases uniformly.
703-725: History printing with chat roles is correct; consider a defensive nil check forContent.Skipping messages with
msg.Role == schemas.ChatMessageRoleSystemmatches how you seed the system prompt and prevents clutter in the history view. The content rendering (preferContentStr, otherwise joinContentBlocks.Text) mirrors the response-handling logic and will produce readable output.To make this more robust against unexpected provider behavior, you might add a quick guard like
if msg.Content == nil { continue }before dereferencingmsg.Content.*, which would avoid panics if a future provider/plugin emits a message without content.
522-533: Factor out repeatedChatMessageContenttext extraction into a helper.The pattern:
- Prefer
ContentStrwhen set.- Otherwise, iterate
ContentBlocksand concatenateblock.Text.is duplicated in
SendMessage,synthesizeToolResults, andPrintHistory. A small helper such as:func chatMessageText(msg schemas.ChatMessage) string { if msg.Content == nil { return "" } if msg.Content.ContentStr != nil { return *msg.Content.ContentStr } if msg.Content.ContentBlocks != nil { var parts []string for _, block := range msg.Content.ContentBlocks { if block.Text != nil { parts = append(parts, *block.Text) } } return strings.Join(parts, "\n") } return "" }would remove this duplication, centralize any future changes (e.g., nil checks or new modalities), and keep the call sites much simpler.
Also applies to: 682-693, 708-719
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/core-chatbot/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.github/workflows/test-coverage.yml(1 hunks)core/providers/bedrock/signer.go(2 hunks)docs/architecture/framework/model-catalog.mdx(1 hunks)framework/configstore/logger.go(1 hunks)framework/configstore/postgres.go(1 hunks)framework/configstore/sqlite.go(1 hunks)framework/docker-compose.yml(1 hunks)framework/logstore/logger.go(1 hunks)framework/logstore/postgres.go(2 hunks)framework/logstore/sqlite.go(1 hunks)framework/modelcatalog/main.go(1 hunks)framework/modelcatalog/pricing.go(9 hunks)plugins/otel/main.go(1 hunks)tests/core-chatbot/go.mod(1 hunks)tests/core-chatbot/main.go(12 hunks)transports/bifrost-http/server/server.go(1 hunks)transports/config.schema.json(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
framework/logstore/sqlite.go (1)
core/schemas/logger.go (1)
Logger(28-55)
framework/configstore/postgres.go (1)
core/schemas/logger.go (1)
Logger(28-55)
framework/configstore/sqlite.go (1)
core/schemas/logger.go (1)
Logger(28-55)
framework/logstore/postgres.go (1)
core/schemas/logger.go (1)
Logger(28-55)
framework/configstore/logger.go (1)
core/schemas/logger.go (2)
Logger(28-55)LogLevel(6-6)
transports/bifrost-http/server/server.go (2)
framework/modelcatalog/main.go (1)
Config(26-29)transports/bifrost-http/lib/config.go (1)
Config(139-169)
framework/logstore/logger.go (1)
core/schemas/logger.go (2)
Logger(28-55)LogLevel(6-6)
framework/modelcatalog/pricing.go (4)
framework/modelcatalog/main.go (1)
ModelCatalog(31-52)core/schemas/bifrost.go (6)
BifrostResponse(243-253)RequestType(81-81)Gemini(48-48)ResponsesRequest(89-89)ResponsesStreamRequest(90-90)ChatCompletionRequest(87-87)core/schemas/chatcompletions.go (1)
BifrostLLMUsage(558-565)core/schemas/transcriptions.go (1)
TranscriptionUsageInputTokenDetails(81-84)
tests/core-chatbot/main.go (1)
core/schemas/chatcompletions.go (6)
ChatMessageRoleSystem(340-340)ChatParameters(154-183)ChatToolChoice(265-268)BifrostChatRequest(11-18)ChatMessageRoleTool(341-341)ChatToolMessage(455-457)
🔇 Additional comments (23)
tests/core-chatbot/go.mod (2)
3-3: Verify Go version and new direct dependency addition.The go.mod file reflects systematic dependency updates across AWS SDK submodules and golang.org/x packages, which aligns with the PR's infrastructure modernization goals. However, two items warrant clarification:
Go version bump (line 3): go 1.24.1 is a very recent release. While the PR confirms successful builds, please confirm this version is intentionally selected and compatible with the rest of the codebase.
New direct dependency (line 9): golang.org/x/text v0.30.0 is now a direct requirement. Verify this was previously a transitive dependency and is required for the GORM logger or other core-chatbot test changes introduced in this PR.
Also applies to: 7-10
15-28: AWS SDK v2 package versions are coherent across submodules.The AWS SDK v2 indirect dependencies are updated consistently to v1.39.5+ across all related submodules (config, credentials, internal packages, service packages, smithy-go). This systematic update pattern is appropriate and reduces risk of version conflicts.
transports/config.schema.json (1)
564-575: Verify new top-level config references are registered correctly.These four new public properties are added to the root schema. Ensure that:
- These are intentional public API additions (not internal only).
- Documentation/migration guides exist for users upgrading to this schema version.
- Any existing tooling or validators have been updated to handle these new top-level fields.
framework/logstore/logger.go (1)
37-40: Verify that disabling SQL query tracing is intentional.The
Tracemethod is a NOOP, which means SQL queries, execution times, and row counts will not be logged. This is often desirable in production to reduce log volume, but it may hinder debugging of slow queries or database issues.If query logging is needed for development or debugging, consider implementing basic trace logging:
func (l *gormLogger) Trace(ctx context.Context, begin time.Time, fc func() (sql string, rowsAffected int64), err error) { - // NOOP + if err != nil { + sql, rows := fc() + l.logger.Debug("SQL error: %v, query: %s, rows: %d, duration: %v", err, sql, rows, time.Since(begin)) + } }Alternatively, if the NOOP is intentional for performance, consider adding a comment explaining the rationale.
docs/architecture/framework/model-catalog.mdx (1)
237-254: LGTM! Receiver rename improves consistency.The receiver rename from
pmtomcaligns with the broader refactoring across the ModelCatalog codebase. The internal references (mc.mu,mc.logger,mc.pricingData) are correctly updated.core/providers/bedrock/signer.go (2)
160-200: LGTM! URL decoding logic correctly preserves plus signs.The
percentDecodefunction properly handles percent-encoded sequences without treating+as a space (unlikeurl.QueryUnescape). The comment clearly explains the distinction from form encoding, and the implementation is correct.
235-244: URL encoding implementation is correct and thoroughly tested.The code review comment requests have been verified:
Already percent-encoded sequences – Test cases confirm:
key=%20→key=%20,percent=%25→percent=%25,path=%2Fto%2Ffile→path=%2Fto%2FfilePlus signs encode as
%2B– Confirmed:key=a+b→key=a%2Bb, and mixed casesearch=hello world+test→search=hello%20world%2BtestSpecial characters – All tested and correct:
=→%3D,&→%26,/→%2FThe implementation correctly uses the decode-then-encode normalization approach to prevent double-encoding while ensuring RFC 3986 compliance for AWS SigV4 canonical query strings. The
percentDecodefunction properly avoids treating+as a space (unlikeurl.QueryUnescape), andpercentEncodeRFC3986preserves only RFC 3986 unreserved characters. Test coverage is comprehensive with 25+ test cases inbuildCanonicalQueryStringtests alone, including AWS SigV4 examples and edge cases.framework/modelcatalog/pricing.go (1)
11-11: LGTM! Receiver rename improves code consistency.The receiver rename from
pmtomcacross all pricing methods aligns with the ModelCatalog type name and improves readability. All internal references are correctly updated.Also applies to: 101-101, 139-139, 262-262
plugins/otel/main.go (1)
87-89: LGTM! Graceful degradation when pricing manager is unavailable.Making the pricing manager optional allows the OTEL plugin to function in environments where cost tracking is not configured. The warning clearly communicates the limitation, and the downstream code (lines 249, 268) already handles nil pricing managers safely.
transports/bifrost-http/server/server.go (1)
816-818: LGTM! Defensive nil-check prevents potential panic.The guard ensures
AddModelDataToPoolis only called when the pricing manager is available, preventing a nil pointer dereference. This aligns with the broader PR theme of making the pricing manager optional.framework/logstore/postgres.go (1)
25-27: LGTM! GORM logger integration is clean.The custom logger adapter is correctly integrated into the Postgres log store configuration, enabling per-instance logging that bridges GORM to the internal logging system.
framework/configstore/postgres.go (1)
24-26: LGTM! GORM logger integration is clean.The custom logger adapter is correctly integrated into the Postgres config store configuration, enabling per-instance logging that bridges GORM to the internal logging system.
framework/logstore/sqlite.go (1)
32-32: LGTM! Logger integration improvement.The switch from the static GORM logger to a per-instance custom logger properly integrates with the project's internal logging system and provides better control over log output.
framework/configstore/sqlite.go (1)
31-31: LGTM! Consistent logger integration.The custom logger integration matches the pattern applied in logstore and properly routes GORM logs through the project's internal logging system.
framework/docker-compose.yml (2)
23-35: Redis service configuration looks good.The Redis service is properly configured with healthcheck, ports, and networking. However, note that no volume is mounted for data persistence—Redis will lose data on container restart.
If data persistence is needed, consider mounting the
redis_datavolume (defined at lines 69-70 but currently unused):redis: image: redis/redis-stack:latest container_name: bifrost-redis ports: - "6379:6379" + volumes: + - redis_data:/data healthcheck: test: ["CMD", "redis-cli", "ping"] interval: 10s timeout: 5s retries: 5 restart: unless-stopped networks: - bifrost_network
37-58: LGTM! Weaviate service properly configured.The Weaviate service includes proper healthcheck, volume mounting for persistence, and necessary environment variables for anonymous access and configuration.
framework/configstore/logger.go (2)
11-20: LGTM! Clean GORM logger adapter implementation.The adapter properly bridges GORM's logging interface to the project's internal logger. The
LogModeNOOP is acceptable since log level control is handled by the internalschemas.Logger.
37-40: SQL query logging is disabled—verify this is intentional.The
Tracemethod is a NOOP, which means GORM SQL queries, execution times, and row counts will not be logged. This improves performance and reduces log noise but makes debugging database issues more difficult.If SQL query logging is needed for debugging, consider implementing
Traceto conditionally log based on the internal logger's level:func (l *gormLogger) Trace(ctx context.Context, begin time.Time, fc func() (sql string, rowsAffected int64), err error) { - // NOOP + // Only log slow queries or errors + elapsed := time.Since(begin) + if err != nil || elapsed > 200*time.Millisecond { + sql, rows := fc() + l.logger.Debug("SQL: %s | rows: %d | elapsed: %v | error: %v", sql, rows, elapsed, err) + } }.github/workflows/test-coverage.yml (2)
25-30: LGTM! Node.js setup for UI builds.Proper Node.js setup with caching configured for the UI build steps.
100-105: LGTM! Proper cleanup withif: always().The cleanup step ensures Docker services are stopped regardless of test outcome, preventing resource leaks in the CI environment.
tests/core-chatbot/main.go (3)
289-297: System promptChatMessageconstruction is correct and keeps history invariant.Using
schemas.ChatMessageRoleSystemplus&schemas.ChatMessageContent{ContentStr: bifrost.Ptr(session.systemPrompt)}cleanly seeds the system prompt as the first history entry, which aligns with the/clearhandler’s assumption thathistory[0]is the system message. No issues here.
456-465: User message creation withChatMessage/ChatMessageContentlooks good.The user message is built with
ChatMessageRoleUserand aChatMessageContentholdingContentStr, then appended by value tohistory. This matches the system message shape and the expectations of downstream chat handling.
472-533: Chat request construction and assistant message handling align with new chat APIs.
schemas.ChatParametersis populated withTemperature,MaxCompletionTokens, andToolChoice: ChatToolChoiceStr="auto", which matches the new chatcompletions parameter shape.schemas.BifrostChatRequestis correctly filled withProvider,Model,Input: s.history, andParams: params, so the primary chat call should behave as before.- Appending
*assistantMessage(value, not pointer) intohistorykeeps the history consistent with other messages and avoids aliasing the choice struct.No functional issues spotted in this section.
9771bf4 to
3765d4d
Compare
Merge activity
|
## Summary Enhance test coverage workflow with improved integration testing capabilities and fix AWS Bedrock URL encoding issues. ## Changes - Enhanced test coverage workflow to include UI build, Redis and Weaviate services for integration tests, and plugin rebuilding - Fixed AWS Bedrock URL encoding by properly handling percent-encoded sequences in query parameters - Added GORM logger implementations for configstore and logstore to improve database logging - Updated core-chatbot test module to work with latest Bifrost API changes - Fixed model catalog pricing lookup method naming for consistency ## Type of change - [x] Bug fix - [x] Feature - [x] Refactor - [ ] Documentation - [x] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [x] Providers/Integrations - [x] Plugins - [x] UI (Next.js) - [ ] Docs ## How to test ```sh # Test AWS Bedrock URL encoding fix go test ./core/providers/bedrock -v # Test the enhanced test coverage workflow cd .github/workflows ./test-coverage.yml # Test GORM logger implementations go test ./framework/configstore -v go test ./framework/logstore -v # Test core-chatbot with latest API changes cd tests/core-chatbot go test -v ``` ## Breaking changes - [ ] Yes - [x] No ## Related issues Fixes AWS Bedrock URL encoding issues with special characters in query parameters. ## Security considerations The AWS Bedrock URL encoding fix improves request signing security by properly handling percent-encoded sequences. ## Checklist - [x] I added/updated tests where appropriate - [x] I verified builds succeed (Go and UI) - [x] I verified the CI pipeline passes locally if applicable
1 participant
Summary
Enhance test coverage workflow with improved integration testing capabilities and fix AWS Bedrock URL encoding issues.
Changes
Type of change
Affected areas
How to test
Breaking changes
Related issues
Fixes AWS Bedrock URL encoding issues with special characters in query parameters.
Security considerations
The AWS Bedrock URL encoding fix improves request signing security by properly handling percent-encoded sequences.
Checklist