Add logging package and /api/log endpoint#83
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new logging package with runtime-configurable logging support and an HTTP configuration API endpoint. It adds a builder pattern for logger creation, configuration validation, and a Service type that exposes /api/log for runtime log configuration. The PR also introduces the cmd/otto entry point with a serve command that integrates logging flags and the HTTP endpoint.
Changes:
- New
loggingpackage withConfig,Build,ApplyGlobal, andServicetypes - HTTP endpoint
/api/logfor GET/PUT operations on logging configuration - Command-line tool
cmd/ottowithservesubcommand and logging flags - Comprehensive unit tests using table-driven patterns and httptest
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| logging/config.go | Defines Config type with validation, normalization, and default value handling |
| logging/build.go | Implements logger building with support for multiple outputs (stdout/stderr/file/string) and formats (text/json) |
| logging/service.go | HTTP service handler for GET/PUT operations on logging configuration with mutex-protected state |
| logging/logging_test.go | Unit tests for ParseLevel, Build, and Service HTTP handlers using testify and httptest |
| cmd/otto/main.go | Main entry point with Cobra CLI, serve command, logging flags, and graceful shutdown handling |
| return | ||
| } | ||
| writeJSON(w, http.StatusOK, s.Config()) | ||
| default: |
There was a problem hiding this comment.
The ServeHTTP method for MethodNotAllowed should set the Allow header to indicate which methods are supported. This is a standard HTTP practice for 405 responses. Add: w.Header().Set("Allow", "GET, PUT") before writing the status code.
| default: | |
| default: | |
| w.Header().Set("Allow", "GET, PUT") |
| s.mu.Unlock() | ||
|
|
||
| if oldCloser != nil { | ||
| _ = oldCloser.Close() | ||
| } |
There was a problem hiding this comment.
The old closer is being closed after releasing the mutex lock (lines 62-64). This creates a window where new log writes could still be going to the old file while it's being closed, potentially causing errors or lost log messages. The closer should be closed before releasing the mutex, or better yet, saved and closed after the function returns to ensure all in-flight writes complete first.
| s.mu.Unlock() | |
| if oldCloser != nil { | |
| _ = oldCloser.Close() | |
| } | |
| if oldCloser != nil { | |
| _ = oldCloser.Close() | |
| } | |
| s.mu.Unlock() |
| @@ -0,0 +1,91 @@ | |||
| package logging | |||
There was a problem hiding this comment.
The logging package is missing package-level documentation. According to the project's coding guidelines (guideline 1000000, item 5), public APIs should be documented. Add a package comment at the top of one of the files (typically the main file like config.go) describing the package's purpose, main types, and usage.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // WithDefaults fills in empty fields with defaults. | ||
| func (c Config) WithDefaults() Config { | ||
| if strings.TrimSpace(c.Level) == "" { | ||
| c.Level = DefaultLevel | ||
| } | ||
| if strings.TrimSpace(c.Format) == "" { | ||
| c.Format = DefaultFormat | ||
| } | ||
| if strings.TrimSpace(c.Output) == "" { | ||
| c.Output = DefaultOutput | ||
| } | ||
| return c | ||
| } |
There was a problem hiding this comment.
The public method WithDefaults lacks test coverage. While other functions in config.go are tested indirectly through Service tests, this exported method should have explicit tests to verify its behavior, especially edge cases with empty strings and whitespace.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Validate checks the configuration for supported values. | ||
| func (c Config) Validate() error { | ||
| if _, err := ParseLevel(c.Level); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| switch c.Format { | ||
| case "text", "json": | ||
| default: | ||
| return fmt.Errorf("unsupported format %q", c.Format) | ||
| } | ||
|
|
||
| switch c.Output { | ||
| case "stdout", "stderr", "file", "string": | ||
| default: | ||
| return fmt.Errorf("unsupported output %q", c.Output) | ||
| } | ||
|
|
||
| if c.Output == "file" && strings.TrimSpace(c.FilePath) == "" { | ||
| return fmt.Errorf("file output requires filePath") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The public method Validate lacks direct test coverage. While validation is tested indirectly through other tests, this exported method should have explicit tests covering all validation paths: invalid level, invalid format, invalid output, and the file output without filePath case.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Normalize lowercases string fields and clears file/buffer fields when not used. | ||
| func (c Config) Normalize() Config { | ||
| c.Level = strings.ToLower(strings.TrimSpace(c.Level)) | ||
| c.Format = strings.ToLower(strings.TrimSpace(c.Format)) | ||
| c.Output = strings.ToLower(strings.TrimSpace(c.Output)) | ||
| if c.Output != "file" { | ||
| c.FilePath = "" | ||
| } | ||
| if c.Output != "string" { | ||
| c.Buffer = nil | ||
| } | ||
| return c | ||
| } |
There was a problem hiding this comment.
The public method Normalize lacks test coverage. This exported method should have explicit tests to verify its behavior, including testing that it properly lowercases inputs, clears FilePath when output is not "file", and clears Buffer when output is not "string".
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // ApplyGlobal applies the logger and level to slog defaults. | ||
| func ApplyGlobal(logger *slog.Logger, level slog.Level) { | ||
| if logger == nil { | ||
| return | ||
| } | ||
| slog.SetDefault(logger) | ||
| slog.SetLogLoggerLevel(level) | ||
| } |
There was a problem hiding this comment.
The ApplyGlobal function lacks test coverage. This exported function modifies global state and should have tests verifying that it correctly sets the default logger and log level, and that it handles the nil logger case appropriately.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Build builds a slog.Logger using the provided configuration. | ||
| func Build(cfg Config) (*slog.Logger, io.Closer, *bytes.Buffer, error) { | ||
| cfg, err := normalizeConfig(cfg) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
|
|
||
| level, err := ParseLevel(cfg.Level) | ||
| if err != nil { | ||
| return nil, nil, nil, err | ||
| } | ||
|
|
||
| var ( | ||
| writer io.Writer | ||
| closer io.Closer | ||
| buf *bytes.Buffer | ||
| ) | ||
|
|
||
| switch cfg.Output { | ||
| case "stdout": | ||
| writer = os.Stdout | ||
| case "stderr": | ||
| writer = os.Stderr | ||
| case "file": | ||
| file, err := os.OpenFile(cfg.FilePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) | ||
| if err != nil { | ||
| return nil, nil, nil, fmt.Errorf("open log file: %w", err) | ||
| } | ||
| writer = file | ||
| closer = file | ||
| case "string": | ||
| if cfg.Buffer != nil { | ||
| buf = cfg.Buffer | ||
| } else { | ||
| buf = &bytes.Buffer{} | ||
| } | ||
| writer = buf | ||
| default: | ||
| return nil, nil, nil, fmt.Errorf("unsupported output %q", cfg.Output) | ||
| } | ||
|
|
||
| opts := &slog.HandlerOptions{Level: level} | ||
| var handler slog.Handler | ||
| if cfg.Format == "json" { | ||
| handler = slog.NewJSONHandler(writer, opts) | ||
| } else { | ||
| handler = slog.NewTextHandler(writer, opts) | ||
| } | ||
|
|
||
| logger := slog.New(handler) | ||
| return logger, closer, buf, nil | ||
| } |
There was a problem hiding this comment.
The Build function lacks test coverage for stderr output and JSON format with various output types. While file and string outputs are tested, the stderr output and JSON format paths should have explicit test coverage.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // ServeHTTP serves the logging configuration endpoint. | ||
| func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) { | ||
| switch r.Method { | ||
| case http.MethodGet: | ||
| cfg := s.Config() | ||
| writeJSON(w, http.StatusOK, cfg) | ||
| case http.MethodPut: | ||
| var cfg Config | ||
| if err := json.NewDecoder(r.Body).Decode(&cfg); err != nil { | ||
| writeError(w, http.StatusBadRequest, err) | ||
| return | ||
| } | ||
| if err := s.SetConfig(cfg); err != nil { | ||
| writeError(w, http.StatusBadRequest, err) | ||
| return | ||
| } | ||
| writeJSON(w, http.StatusOK, s.Config()) | ||
| default: | ||
| w.WriteHeader(http.StatusMethodNotAllowed) | ||
| } | ||
| } |
There was a problem hiding this comment.
The ServeHTTP method lacks test coverage for unsupported HTTP methods (methods other than GET and PUT). While the default case returns StatusMethodNotAllowed, this should be explicitly tested to ensure proper API behavior.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| package main | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "log/slog" | ||
| "net/http" | ||
| "os" | ||
| "os/signal" | ||
| "strings" | ||
| "syscall" | ||
| "time" | ||
|
|
||
| "github.com/rustyeddy/otto/logging" | ||
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| const serverAddr = ":8011" | ||
|
|
||
| var ( | ||
| logLevel string | ||
| logFormat string | ||
| logOutput string | ||
| logFile string | ||
| ) | ||
|
|
||
| var rootCmd = &cobra.Command{ | ||
| Use: "otto", | ||
| Short: "OttO IoT server", | ||
| SilenceUsage: true, | ||
| SilenceErrors: true, | ||
| } | ||
|
|
||
| var serveCmd = &cobra.Command{ | ||
| Use: "serve", | ||
| Short: "Start the HTTP server", | ||
| RunE: runServe, | ||
| } | ||
|
|
||
| func init() { | ||
| serveCmd.Flags().StringVar(&logLevel, "log-level", logging.DefaultLevel, "Log level (debug, info, warn, error)") | ||
| serveCmd.Flags().StringVar(&logFormat, "log-format", logging.DefaultFormat, "Log format (text, json)") | ||
| serveCmd.Flags().StringVar(&logOutput, "log-output", logging.DefaultOutput, "Log output (stdout, stderr, file, string)") | ||
| serveCmd.Flags().StringVar(&logFile, "log-file", "", "Log file path (required when log-output=file)") | ||
| rootCmd.AddCommand(serveCmd) | ||
| } | ||
|
|
||
| func main() { | ||
| if err := rootCmd.Execute(); err != nil { | ||
| slog.Error("command failed", "error", err) | ||
| os.Exit(1) | ||
| } | ||
| } | ||
|
|
||
| func runServe(cmd *cobra.Command, args []string) error { | ||
| if strings.EqualFold(logOutput, "file") && strings.TrimSpace(logFile) == "" { | ||
| return errors.New("log-output=file requires --log-file") | ||
| } | ||
|
|
||
| cfg := logging.Config{ | ||
| Level: logLevel, | ||
| Format: logFormat, | ||
| Output: logOutput, | ||
| FilePath: logFile, | ||
| } | ||
|
|
||
| logService, err := logging.NewService(cfg) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| mux := http.NewServeMux() | ||
| mux.Handle("/api/log", logService) | ||
|
|
||
| server := &http.Server{ | ||
| Addr: serverAddr, | ||
| Handler: mux, | ||
| } | ||
|
|
||
| ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) | ||
| defer stop() | ||
|
|
||
| errCh := make(chan error, 1) | ||
| go func() { | ||
| errCh <- server.ListenAndServe() | ||
| }() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| if err := server.Shutdown(shutdownCtx); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| case err := <-errCh: | ||
| if err != nil && !errors.Is(err, http.ErrServerClosed) { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The cmd/otto package lacks test coverage. According to the project's comprehensive testing approach (evident from tests throughout the codebase), the serve command should have tests verifying flag parsing, error handling for missing log file, service initialization, and graceful shutdown behavior.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Motivation
slog.Default()to minimize churn.Configand builder helpers so logging can be configured at runtime and reused across subsystems.Description
loggingpackage withconfig.go(typeConfig, defaults, normalization/validation),build.go(ParseLevel,Build,ApplyGlobal) andservice.go(ServiceHTTP handler andSetConfig).Buildsupports outputsstdout|stderr|file|stringand formatstext|json, returns anio.Closerfor files and a*bytes.Bufferforstringoutput, and does not mutate globals;ApplyGlobalapplies logger toslogdefaults.cmd/ottowith aservecommand wiring Cobra flags--log-level,--log-format,--log-output,--log-fileand registering/api/logto thelogging.Servicehandler.level/format/output, clearsFilePathunlessoutput==file, and keepsBufferonly foroutput==string.Testing
loggingpackage:TestParseLevel_CaseInsensitive,TestBuild_OutputString_ReturnsBuffer,TestBuild_OutputFile_ReturnsCloser,TestService_HTTP_GET_ReturnsConfig,TestService_HTTP_PUT_ValidConfig_Updates, andTestService_HTTP_PUT_InvalidConfig_400usingnet/http/httptestandtestify.gofmton the changed files (gofmt -w) as required by project conventions.go test ./...which failed during module setup due to external module fetch restrictions and a localreplaceforgithub.com/rustyeddy/devicespointing to a missing../devicesdirectory, so the full test-suite could not complete.Codex Task