server logs config - #4653
server logs config#4653
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new ChangesDump errors config propagation
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant updateConfig as updateConfig
participant BifrostHTTPServer as BifrostHTTPServer
participant CorsMiddleware as CorsMiddleware
participant Fasthttp as fasthttp handler chain
Client->>updateConfig: send client config with dump_errors_in_console_logs
updateConfig->>BifrostHTTPServer: update Config
BifrostHTTPServer->>CorsMiddleware: UpdateConfig(s.Config)
Fasthttp->>CorsMiddleware: Middleware()
CorsMiddleware-->>Fasthttp: per-request cfg snapshot
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/handlers/middlewares.go`:
- Around line 74-96: The CORS middleware still shares mutable `*lib.Config`, so
atomic pointer swaps do not prevent races on nested fields like
`ClientConfig.DumpErrorsInConsoleLogs`, `AllowedOrigins`, and `AllowedHeaders`.
Refactor `CorsMiddleware`, `NewCorsMiddleware`, `UpdateConfig`, and
`Middleware()` to store and read a small immutable snapshot struct instead of
`*lib.Config`, and clone the slices when building or updating the snapshot.
Ensure `Middleware()` only reads the snapshot fields (`dumpErrorsInConsoleLogs`,
`allowedOrigins`, `allowedHeaders`) so each request sees a consistent config.
In `@ui/app/workspace/config/views/clientSettingsView.tsx`:
- Around line 350-355: The new toggle in clientSettingsView’s Switch is missing
a stable Playwright hook. Add a data-testid to this control using the existing
3-part convention and match sibling naming patterns in clientSettingsView, e.g.
a client-settings-dump-errors-switch identifier on the Switch tied to
dump_errors_in_console_logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 47690e10-7198-487a-8926-1367863acf06
📒 Files selected for processing (18)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/clientconfig.gohelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamltests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/cmd/seedvks/go.modtransports/bifrost-http/handlers/config.gotransports/bifrost-http/handlers/middlewares.gotransports/bifrost-http/handlers/middlewares_test.gotransports/bifrost-http/server/server.gotransports/config.schema.jsonui/app/workspace/config/views/clientSettingsView.tsxui/lib/types/config.ts
35a9370 to
d852f25
Compare
d852f25 to
0371f7b
Compare
Merge activity
|
## Summary Adds a new `dump_errors_in_console_logs` client configuration option that, when enabled, writes full HTTP error response bodies to the server console logs. This is intended to aid debugging without requiring a server restart or log level change. ## Changes - Added `DumpErrorsInConsoleLogs` field to `ClientConfig`, `TableClientConfig`, and the RDB read/write paths. - Added a database migration (`add_dump_errors_in_console_logs_column`) to introduce the column with a default of `false`. - Refactored `CorsMiddleware` from a plain function into a `CorsMiddleware` struct backed by an `atomic.Pointer[lib.Config]`, allowing the config (including the new flag) to be swapped at runtime without restarting the server and without data races on in-flight requests. - When `DumpErrorsInConsoleLogs` is `true`, the CORS/logging middleware appends the response body as `http.error` to the structured log entry for any response with a status code ≥ 400. - Wired `DumpErrorsInConsoleLogs` into the config update handler so changes take effect immediately via the atomic config pointer. - Added the field to the config hash, using a non-default-only hashing strategy to avoid hash churn on upgrade for existing deployments. - Exposed the setting in the Helm chart (`values.yaml`, `values.schema.json`, `_helpers.tpl`, `README.md`), the transport config schema (`config.schema.json`), and the UI settings view with a toggle and description. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./framework/configstore/... go test ./transports/bifrost-http/... # UI cd ui pnpm i pnpm build ``` 1. Set `dump_errors_in_console_logs: true` in the client config (via UI toggle or config file). 2. Issue a request that produces a 4xx or 5xx response. 3. Confirm the server console log for that request includes an `http.error` field containing the response body. 4. Toggle the setting off and confirm the field no longer appears in logs without restarting the server. **New config field:** | Field | Type | Default | Description | |---|---|---|---| | `dump_errors_in_console_logs` | `boolean` | `false` | When `true`, full error response bodies are written to server console logs. Useful for debugging; may be noisy in production. | ## Breaking changes - [x] No The `CorsMiddleware` function signature changed to a struct-based API (`NewCorsMiddleware` + `.Middleware()`). Any code outside this repository calling `CorsMiddleware(config)` directly will need to be updated to `NewCorsMiddleware(config).Middleware()`. ## Security considerations Error response bodies logged to the console may contain sensitive information (e.g., upstream provider error messages, request details). This feature is disabled by default and should be used with care in production environments. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds a new `dump_errors_in_console_logs` client configuration option that, when enabled, writes full HTTP error response bodies to the server console logs. This is intended to aid debugging without requiring a server restart or log level change. ## Changes - Added `DumpErrorsInConsoleLogs` field to `ClientConfig`, `TableClientConfig`, and the RDB read/write paths. - Added a database migration (`add_dump_errors_in_console_logs_column`) to introduce the column with a default of `false`. - Refactored `CorsMiddleware` from a plain function into a `CorsMiddleware` struct backed by an `atomic.Pointer[lib.Config]`, allowing the config (including the new flag) to be swapped at runtime without restarting the server and without data races on in-flight requests. - When `DumpErrorsInConsoleLogs` is `true`, the CORS/logging middleware appends the response body as `http.error` to the structured log entry for any response with a status code ≥ 400. - Wired `DumpErrorsInConsoleLogs` into the config update handler so changes take effect immediately via the atomic config pointer. - Added the field to the config hash, using a non-default-only hashing strategy to avoid hash churn on upgrade for existing deployments. - Exposed the setting in the Helm chart (`values.yaml`, `values.schema.json`, `_helpers.tpl`, `README.md`), the transport config schema (`config.schema.json`), and the UI settings view with a toggle and description. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./framework/configstore/... go test ./transports/bifrost-http/... # UI cd ui pnpm i pnpm build ``` 1. Set `dump_errors_in_console_logs: true` in the client config (via UI toggle or config file). 2. Issue a request that produces a 4xx or 5xx response. 3. Confirm the server console log for that request includes an `http.error` field containing the response body. 4. Toggle the setting off and confirm the field no longer appears in logs without restarting the server. **New config field:** | Field | Type | Default | Description | |---|---|---|---| | `dump_errors_in_console_logs` | `boolean` | `false` | When `true`, full error response bodies are written to server console logs. Useful for debugging; may be noisy in production. | ## Breaking changes - [x] No The `CorsMiddleware` function signature changed to a struct-based API (`NewCorsMiddleware` + `.Middleware()`). Any code outside this repository calling `CorsMiddleware(config)` directly will need to be updated to `NewCorsMiddleware(config).Middleware()`. ## Security considerations Error response bodies logged to the console may contain sensitive information (e.g., upstream provider error messages, request details). This feature is disabled by default and should be used with care in production environments. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
## Summary Adds a new `dump_errors_in_console_logs` client configuration option that, when enabled, writes full HTTP error response bodies to the server console logs. This is intended to aid debugging without requiring a server restart or log level change. ## Changes - Added `DumpErrorsInConsoleLogs` field to `ClientConfig`, `TableClientConfig`, and the RDB read/write paths. - Added a database migration (`add_dump_errors_in_console_logs_column`) to introduce the column with a default of `false`. - Refactored `CorsMiddleware` from a plain function into a `CorsMiddleware` struct backed by an `atomic.Pointer[lib.Config]`, allowing the config (including the new flag) to be swapped at runtime without restarting the server and without data races on in-flight requests. - When `DumpErrorsInConsoleLogs` is `true`, the CORS/logging middleware appends the response body as `http.error` to the structured log entry for any response with a status code ≥ 400. - Wired `DumpErrorsInConsoleLogs` into the config update handler so changes take effect immediately via the atomic config pointer. - Added the field to the config hash, using a non-default-only hashing strategy to avoid hash churn on upgrade for existing deployments. - Exposed the setting in the Helm chart (`values.yaml`, `values.schema.json`, `_helpers.tpl`, `README.md`), the transport config schema (`config.schema.json`), and the UI settings view with a toggle and description. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [x] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go test ./framework/configstore/... go test ./transports/bifrost-http/... # UI cd ui pnpm i pnpm build ``` 1. Set `dump_errors_in_console_logs: true` in the client config (via UI toggle or config file). 2. Issue a request that produces a 4xx or 5xx response. 3. Confirm the server console log for that request includes an `http.error` field containing the response body. 4. Toggle the setting off and confirm the field no longer appears in logs without restarting the server. **New config field:** | Field | Type | Default | Description | |---|---|---|---| | `dump_errors_in_console_logs` | `boolean` | `false` | When `true`, full error response bodies are written to server console logs. Useful for debugging; may be noisy in production. | ## Breaking changes - [x] No The `CorsMiddleware` function signature changed to a struct-based API (`NewCorsMiddleware` + `.Middleware()`). Any code outside this repository calling `CorsMiddleware(config)` directly will need to be updated to `NewCorsMiddleware(config).Middleware()`. ## Security considerations Error response bodies logged to the console may contain sensitive information (e.g., upstream provider error messages, request details). This feature is disabled by default and should be used with care in production environments. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds a new
dump_errors_in_console_logsclient configuration option that, when enabled, writes full HTTP error response bodies to the server console logs. This is intended to aid debugging without requiring a server restart or log level change.Changes
DumpErrorsInConsoleLogsfield toClientConfig,TableClientConfig, and the RDB read/write paths.add_dump_errors_in_console_logs_column) to introduce the column with a default offalse.CorsMiddlewarefrom a plain function into aCorsMiddlewarestruct backed by anatomic.Pointer[lib.Config], allowing the config (including the new flag) to be swapped at runtime without restarting the server and without data races on in-flight requests.DumpErrorsInConsoleLogsistrue, the CORS/logging middleware appends the response body ashttp.errorto the structured log entry for any response with a status code ≥ 400.DumpErrorsInConsoleLogsinto the config update handler so changes take effect immediately via the atomic config pointer.values.yaml,values.schema.json,_helpers.tpl,README.md), the transport config schema (config.schema.json), and the UI settings view with a toggle and description.Type of change
Affected areas
How to test
dump_errors_in_console_logs: truein the client config (via UI toggle or config file).http.errorfield containing the response body.New config field:
dump_errors_in_console_logsbooleanfalsetrue, full error response bodies are written to server console logs. Useful for debugging; may be noisy in production.Breaking changes
The
CorsMiddlewarefunction signature changed to a struct-based API (NewCorsMiddleware+.Middleware()). Any code outside this repository callingCorsMiddleware(config)directly will need to be updated toNewCorsMiddleware(config).Middleware().Security considerations
Error response bodies logged to the console may contain sensitive information (e.g., upstream provider error messages, request details). This feature is disabled by default and should be used with care in production environments.
Checklist
docs/contributing/README.mdand followed the guidelines