[log] Add debug logging to config_core.go#4074
Conversation
…ults - Log undecoded TOML key count before unknown-field validation - Log server count before stdio containerization validation - Log server count before auth configuration validation - Log when opentelemetry config is merged into tracing - Log applied gateway defaults (port, timeouts, keepalive interval) These log calls use the existing logConfig logger and are activated via DEBUG=config:* or DEBUG=* to help diagnose configuration loading issues at startup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds additional debug logging in the config loader to make TOML parsing/defaulting/validation progress visible under DEBUG=config:*.
Changes:
- Log resolved gateway defaults after default application.
- Add progress logs around unknown-key checking, stdio containerization validation, and auth validation in
LoadFromFile. - Log when
gateway.opentelemetryis present and merged into tracing.
Show a summary per file
| File | Description |
|---|---|
internal/config/config_core.go |
Adds debug logs for gateway default resolution and intermediate LoadFromFile validation/merge steps. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 3
| } | ||
|
|
||
| // Validate TOML stdio servers use Docker for containerization (Spec Section 3.2.1) | ||
| logConfig.Printf("Validating stdio server containerization requirements for %d servers", len(cfg.Servers)) |
There was a problem hiding this comment.
This debug log uses len(cfg.Servers), but the validation is specifically about stdio server containerization. Using the total server count can be confusing (HTTP servers are included in the number); consider logging the number of stdio servers being checked (or adjust the wording to avoid implying all servers are stdio).
| logConfig.Printf("Validating stdio server containerization requirements for %d servers", len(cfg.Servers)) | |
| stdioServerCount := 0 | |
| for _, serverCfg := range cfg.Servers { | |
| if serverCfg.Type == "stdio" { | |
| stdioServerCount++ | |
| } | |
| } | |
| logConfig.Printf("Validating stdio server containerization requirements for %d stdio servers", stdioServerCount) |
| // Merge opentelemetry key into tracing when present (spec §4.1.3.6). | ||
| // opentelemetry takes precedence over the legacy tracing key. | ||
| if cfg.Gateway.Opentelemetry != nil { | ||
| logConfig.Printf("opentelemetry section found: merging into tracing config, endpoint=%s", cfg.Gateway.Opentelemetry.Endpoint) |
There was a problem hiding this comment.
Logging cfg.Gateway.Opentelemetry.Endpoint verbatim can leak sensitive information if the endpoint URL ever contains credentials (userinfo), tokens in query params, or internal hostnames, and it will also be written to the debug log file. Consider logging only whether the endpoint is set, or logging a sanitized form (e.g., scheme/host only, with userinfo/query redacted).
| if cfg.KeepaliveInterval == 0 { | ||
| cfg.KeepaliveInterval = DefaultKeepaliveInterval | ||
| } | ||
| logConfig.Printf("Applied gateway defaults: port=%d, startupTimeout=%d, toolTimeout=%d, keepaliveInterval=%d", |
There was a problem hiding this comment.
The log message "Applied gateway defaults" is emitted unconditionally, even when no defaults were actually applied (e.g., all fields were already set). This can be misleading during debugging; consider either (a) tracking whether any field was defaulted and only logging when changes occurred, or (b) rewording the message to reflect that this is the resolved gateway config after defaulting.
| logConfig.Printf("Applied gateway defaults: port=%d, startupTimeout=%d, toolTimeout=%d, keepaliveInterval=%d", | |
| logConfig.Printf("Resolved gateway config after defaulting: port=%d, startupTimeout=%d, toolTimeout=%d, keepaliveInterval=%d", |
|
@copilot address the review feedback #4074 (review) |
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/14d88f97-3dd7-4220-8f4f-f53f53e0e15d Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed in commit f07cbfd. I implemented the requested review feedback in
|
Adds 5 meaningful debug logging calls to
internal/config/config_core.gousing the existinglogConfiglogger ("config:config").Changes
applyGatewayDefaults— logs resolved defaults after all zero-value fields are filled in:LoadFromFile— adds intermediate progress logs between the existing entry/exit log calls:Checking N undecoded TOML keys against allowed fieldsValidating stdio server containerization requirements for N serversValidating auth configuration for N serversopentelemetrysection is present:opentelemetry section found: merging into tracing config, endpoint=...Why
LoadFromFilealready logs at entry and exit but its many intermediate validation steps were invisible under debug logging. These additions make it easy to pinpoint which validation step fails or how far initialization progressed when troubleshooting startup issues withDEBUG=config:*.Validation
go build ./...— passes ✅go vet ./...— passes ✅go test ./internal/...— all packages pass; onlyTestFetchAndFixSchema_NetworkErrorfails, which is a pre-existing network-dependent test failure unrelated to these changes ✅Warning
The following domain was blocked by the firewall during workflow execution:
invalidhostthatdoesnotexist12345.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.