Logging improvements#216
Conversation
cea3226 to
0ec1e75
Compare
e79f4e3 to
c450a17
Compare
0ec1e75 to
f9009b9
Compare
c450a17 to
4df689e
Compare
f9009b9 to
a9f8075
Compare
- Create internal/logging package with embedded zap logger - Add Trace level (-8) below Debug level with full sugar support - Implement Python process protection to cap log levels at debug - Replace external github.com/replicate/go/logging dependency - Wire new logger throughout codebase (main.go, server, service, manager) - Create loggingtest package for test logger compatibility - Update all test files to use new test logger infrastructure - Add comprehensive unit tests for logging functionality - Configure golangci-yml with depguard to prevent test package misuse - Support environment variables: COG_LOG_LEVEL, LOG_LEVEL, LOG_FORMAT, LOG_FILE
- Add custom level encoders (uppercase, lowercase, color) to display "TRACE" instead of "LEVEL(-8)" - Implement test logger with proper TRACE level display for zaptest compatibility - Add SugaredLogger chaining support (With/Named) to maintain custom type - Migrate 25 verbose Debug logs to Trace level across runner, server, service, webhook - Keep important Debug logs for actual debugging (errors, state changes, subprocess management) - Verbose implementation details now at Trace: file operations, response watchers, shutdown flow
4df689e to
9c5dbc9
Compare
- Move 16 Info logs to Debug: component initialization, runner management, prediction flow details - Keep critical Info logs: service lifecycle, prediction completion, force kills, webhooks - Move 4 Info logs to Trace: internal shutdown flow and cleanup state tracking - Maintain clean Info level for exceptional events only - Remove unused customLevelEncoder function to fix linter warnings
There was a problem hiding this comment.
Pull Request Overview
This PR implements trace logging and factors out replicate/go/logging by creating a new internal logging package. The changes include replacing all external logging dependencies with the custom internal logging solution and introducing trace-level logging functionality.
- Introduces custom internal logging package with trace level support
- Replaces zap/zaptest usage throughout codebase with internal logging implementations
- Updates log levels from debug/info to trace for more detailed debugging information
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/logging/logger.go | New custom logging implementation with trace level support |
| internal/logging/logging_test.go | Tests for the new logging functionality |
| internal/loggingtest/test_helper.go | Test utility for creating loggers in tests |
| internal/loggingtest/test_helper_test.go | Tests for the logging test utilities |
| cmd/cog/main.go | Updated to use internal logging instead of replicate/go/logging |
| internal/service/service.go | Updated logging calls and switched to internal logging |
| internal/server/server.go | Updated to use internal logging and trace-level calls |
| internal/webhook/webhook.go | Updated to use internal logging and trace-level calls |
| internal/runner/runner.go | Updated to use internal logging and trace-level calls |
| internal/runner/manager.go | Updated to use internal logging and trace-level calls |
| internal/webhook/webhook_test.go | Updated to use loggingtest helper |
| internal/tests/harness_test.go | Updated to use loggingtest helper |
| internal/runner/runner_test.go | Updated to use loggingtest helper |
| internal/runner/manager_test.go | Updated to use loggingtest helper |
| .golangci.yml | Added depguard rule to prevent loggingtest usage in production code |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
IIUC this is effectively a vendored copy of github.com/replicate/go/logging, right?
There was a problem hiding this comment.
Mostly. It does much of what replicate/go does but also implements the Trace level logging. We can either:
- pull this code back to replicate/go
- leave it here, diverged (we don't need trace logging in replicate/go really)
- plan to drop replicate/go/logging across all code bases (this is a bigger decision than should be determined here in a gh pr).
Implements trace logging and factors out replicate/go/logging.