Skip to content

refactor: replace all unstructured log.Printf with structured slog JSON#102

Open
dantonioluigi wants to merge 3 commits into
marco-spagn:mainfrom
dantonioluigi:feature/enhancedlogging
Open

refactor: replace all unstructured log.Printf with structured slog JSON#102
dantonioluigi wants to merge 3 commits into
marco-spagn:mainfrom
dantonioluigi:feature/enhancedlogging

Conversation

@dantonioluigi

Copy link
Copy Markdown
  • Create internal/log package with shared JSON logger (Debug, Info, Warn, Error, Fatal)
  • Migrate 157+ log calls across 23 files to structured key-value format
  • Remove all emoji from log messages for log aggregator compatibility
  • Assign correct log levels per severity instead of flat Printf/Println
  • Add Mask helper for sensitive strings (DB URLs, keys)

Summary

Type of change

  • Bug fix
  • New feature / API change
  • Documentation
  • Refactor / tooling / CI
  • Breaking change (describe migration)

Checklist

  • make test (and make lint for Go changes)
  • make ci-like-github or targeted job parity (make act-lint, make act-test, make act-integration-smoke) for CI-touching changes
  • Integration tests updated if behavior/API changed (-tags=integration where relevant)
  • CHANGELOG.md updated under [Unreleased] if user-visible
  • Version bump in internal/version/version.go + docs/openapi.yaml + Helm appVersion if releasing API version (smoke picks up Tag automatically)
  • OpenAPI / docs/grpc-vs-http.md / SDK HTTP-API.md updated for API changes

Test plan

@marco-spagn

Copy link
Copy Markdown
Owner

Review of PR #102 — refactor: replace all unstructured log.Printf with structured slog JSON

Overall: Good direction. Moving to structured logging is the right call for PCMI. The scope is broad and the mechanical migration is mostly solid. However, there are several material gaps that should be addressed before merging.

Positives
• Structured logging with key-value pairs is a clear improvement.
• Removal of all emojis is correct.
• log.Mask(...) helper is a nice security touch.
• Consistent use of log levels in many places.

Issues that should be fixed

  1. No tests for the new logger (internal/log)
    • The package has zero tests. This is a foundational piece that will be used everywhere.

  2. Always JSON output hurts local DX
    • JSONHandler is hardcoded. Running go run ./cmd/api or ./cmd/worker now produces noisy JSON locally. We need an easy way to get human-readable text logs during development (env var or auto-detect TTY).

  3. Missing OpenTelemetry trace/span correlation
    • PCMI already has solid OTEL instrumentation. The new logger does not inject trace_id/span_id. This is a missed opportunity for observability.

  4. Mask implementation is too naive
    • Current version can still leak secrets or produce ugly output on URLs/keys.
    • Needs better handling or at least clear documentation of its limitations.

  5. Other polish items
    • No AddSource (file:line) in logs — very useful in production.
    • SetLevel replaces the logger without strong concurrency safety.
    • PR description/checklist is mostly empty.

Recommendation
Do not merge yet.

This is a valuable refactor but needs:
• Tests for internal/log
• Local dev experience fix (text mode)
• Decision on trace correlation
• Hardening of Mask
• Filled PR checklist + verification steps (make test, make lint, smoke commands, etc.)

- Create internal/log package with shared JSON logger (Debug, Info, Warn, Error, Fatal)
- Migrate 157+ log calls across 23 files to structured key-value format
- Remove all emoji from log messages for log aggregator compatibility
- Assign correct log levels per severity instead of flat Printf/Println
- Add Mask helper for sensitive strings (DB URLs, keys)
- Replace bare pointer with atomic.Pointer[slog.Logger] for safe concurrent SetLevel
- Move os.Getenv out of log.go into config.Config (LogFormat, LogLevel, LogSource)
  to satisfy getenv audit invariant
- Add Configure(format, level, addSource) hook wired to cmd/api, worker, migrate
- TTY auto-detect: falls back to human-readable text handler when stdout is a terminal
- Add *Context family (InfoContext, WarnContext, ErrorContext, DebugContext) that
  inject trace_id/span_id when an OTel span is active on the context
- Add otelHandler wrapper on every slog.Handler for native trace correlation
- Harden Mask(): URL credential redaction (user:pass@ → ***@), prefix+suffix truncation,
  clamp prefix 4–20, handle empty/short strings gracefully
- Add 30 test cases covering Mask, level parsing, concurrent SetLevel, JSON/text output,
  env config, and context enrichment (all -race clean)
@dantonioluigi dantonioluigi force-pushed the feature/enhancedlogging branch from d82df99 to 19fc7f8 Compare May 26, 2026 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants