feat: add log format option to server and coordinator config#978
feat: add log format option to server and coordinator config#978mattisonchao wants to merge 3 commits intomainfrom
Conversation
Allow configuring log format (text/json) via the config file for server and coordinator. Config file takes precedence over CLI --log-json flag; if not set in config, CLI value is preserved.
mattisonchao
requested review from
RobertIndie,
coderzc and
merlimat
as code owners
- Add tests for ReconfigureLogger: empty level defaults to info, level-only update preserves format - Add tests for config file log format loading in server and coordinator - Add tests verifying empty format is preserved when not in config file - Fix unused-receiver lint error in LogOptions.WithDefault()
There was a problem hiding this comment.
Pull request overview
Adds support for configuring log output format (text vs JSON) via server/coordinator configuration, intended to override CLI defaults only when explicitly set in the config file.
Changes:
- Extend
LogOptionswith aFormatfield and related constants. - Apply config-driven log level/format during startup for server and coordinator when
--sconfigis used. - Update example YAML configs and JSON schemas to include
observability.log.format; add/adjust config parsing tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| oxiad/common/option/option.go | Adds LogOptions.Format and log format constants; changes defaulting behavior. |
| oxiad/common/logging/logger_test.go | Adds tests around reconfiguration behavior. |
| conf/schema/dataserver.json | Adds observability.log.format schema definition. |
| conf/schema/coordinator.json | Adds observability.log.format schema definition. |
| conf/dataserver.yaml | Adds observability.log.format to example config. |
| conf/coordinator.yaml | Adds observability.log.format to example config. |
| cmd/server/cmd_test.go | Extends config parsing tests for log format presence/absence. |
| cmd/server/cmd.go | Applies config-based log level/format at startup when using --sconfig. |
| cmd/coordinator/cmd_test.go | Extends config parsing tests for log format presence/absence. |
| cmd/coordinator/cmd.go | Applies config-based log level/format at startup when using --sconfig. |
Comments suppressed due to low confidence (1)
oxiad/common/option/option.go:79
- LogOptions.Validate currently always returns nil, so invalid log formats (e.g., typos/case variations) will be accepted and then silently treated as text when applying the config. Please validate LogOptions.Format (allow ""|"text"|"json" and return an error otherwise) so bad configs fail fast.
const (
LogFormatText = "text"
LogFormatJSON = "json"
DefaultLogFormat = LogFormatText
)
type LogOptions struct {
Level string `yaml:"level,omitempty" json:"level,omitempty" jsonschema:"description=Log level,example=info,default=info"`
Format string `yaml:"format,omitempty" json:"format,omitempty" jsonschema:"description=Log format (text or json),example=text,default=text,enum=text,enum=json"`
}
func (*LogOptions) WithDefault() {
}
func (*LogOptions) Validate() error {
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestReconfigureLogger_EmptyLevelDefaultsToInfo(t *testing.T) { | ||
| LogLevel = slog.LevelDebug | ||
| ConfigureLogger() | ||
|
|
||
| // Empty level defaults to info via ParseLogLevel fallback | ||
| changed := ReconfigureLogger(&option.LogOptions{}) | ||
| assert.True(t, changed) | ||
| assert.Equal(t, slog.LevelInfo, LogLevel) | ||
| } |
There was a problem hiding this comment.
This test asserts that an empty LogOptions.Level triggers a change to INFO. That behavior conflicts with the PR’s stated "empty means not configured" semantics for preserving CLI precedence when the config omits the level. Consider adjusting the production logic (and this test) so an empty level is treated as "do not change" rather than "force INFO".
| v.WatchConfig() | ||
|
|
||
| logOptions := &dataServerOptions.Observability.Log | ||
| logging.ReconfigureLogger(logOptions) |
There was a problem hiding this comment.
ReconfigureLogger is called unconditionally when a config file is provided. If the config omits observability.log.level, LogOptions.Level remains empty and ParseLogLevel will fall back to INFO, which can override a user-specified CLI --log-level. To preserve the intended precedence, only call ReconfigureLogger when logOptions.Level is non-empty (or change ReconfigureLogger to no-op on empty).
| logging.ReconfigureLogger(logOptions) | |
| if logOptions.Level != "" { | |
| logging.ReconfigureLogger(logOptions) | |
| } |
| v.WatchConfig() | ||
|
|
||
| logOptions := &coordinatorOptions.Observability.Log | ||
| logging.ReconfigureLogger(logOptions) |
There was a problem hiding this comment.
Same precedence issue as cmd/server: calling ReconfigureLogger unconditionally will treat an empty config level as INFO and can override a CLI --log-level. Gate the call on logOptions.Level != "" (or make ReconfigureLogger ignore empty).
| logging.ReconfigureLogger(logOptions) | |
| if logOptions.Level != "" { | |
| logging.ReconfigureLogger(logOptions) | |
| } |
| func TestReconfigureLogger_NoChangeReturnsFalse(t *testing.T) { | ||
| LogLevel = slog.LevelInfo | ||
| LogJSON = true | ||
| ConfigureLogger() | ||
|
|
||
| changed := ReconfigureLogger(&option.LogOptions{Level: "info"}) | ||
| assert.False(t, changed) | ||
| } | ||
|
|
||
| func TestReconfigureLogger_EmptyLevelDefaultsToInfo(t *testing.T) { | ||
| LogLevel = slog.LevelDebug | ||
| ConfigureLogger() | ||
|
|
||
| // Empty level defaults to info via ParseLogLevel fallback | ||
| changed := ReconfigureLogger(&option.LogOptions{}) | ||
| assert.True(t, changed) | ||
| assert.Equal(t, slog.LevelInfo, LogLevel) | ||
| } |
There was a problem hiding this comment.
These tests use package-level globals (LogJSON/LogLevel) but TestReconfigureLogger_NoChangeReturnsFalse and TestReconfigureLogger_EmptyLevelDefaultsToInfo don't set LogJSON. Since test execution order isn't guaranteed, ConfigureLogger() can run with a stale LogJSON value from another test, making the suite order-dependent. Set LogJSON explicitly (or reset globals in t.Cleanup) in every test.
Motivation
Server and coordinator need a way to configure log format (text/json) via config file, with proper precedence over CLI flags.
Modification
Formatfield toLogOptionswithLogFormatTextandLogFormatJSONconstants--log-json, absent means CLI preserved)formatfield to YAML configs and JSON schemas for both dataserver and coordinatorDefaultLogLevelfrom option package (CLI default inloggingpackage is the source of truth)LogOptions.WithDefault()empty — level and format use "empty means not configured" semantics to avoid overriding CLI values