refactor: migrate log.Printf to structured slog logging#123
Merged
Conversation
Replace all ~95 log.Printf calls across 5 production files with structured slog logging for consistency with the existing slog usage in the codebase. Remove unused observe.Logger and Trace wrappers that were never adopted in production code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: refactor: migrate log.Printf to structured slog loggingOverall this is a well-scoped refactor that makes log output significantly more useful. The direction is correct and the diff is clean. A few issues worth addressing before merge. Bug:
|
There was a problem hiding this comment.
Pull request overview
This PR migrates production logging from log.Printf to structured log/slog calls, removes unused internal/observe logging/tracing wrappers, and updates observability/migration documentation to describe direct slog usage.
Changes:
- Replaced
log.Printfusage with structuredslog.{Debug,Info,Warn,Error}across core runtime files. - Removed dead
internal/observetrace/logger code (and its tests), leaving the package focused on metrics + Prometheus export. - Updated docs to describe direct
slogconfiguration and metrics usage.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| template.go | Converts template/runtime logs to structured slog calls. |
| mount.go | Converts HTTP/WebSocket lifecycle logs to structured slog calls. |
| session_stores.go | Migrates Redis session store logs to slog with structured fields. |
| pubsub/redis.go | Migrates Redis pub/sub broadcaster logs to slog. |
| internal/upload/multipart.go | Migrates multipart upload logs to slog. |
| internal/observe/logger.go | Removes unused slog wrapper logger implementation. |
| internal/observe/trace.go | Removes unused trace ID helpers/middleware. |
| internal/observe/trace_test.go | Removes tests for the deleted trace functionality. |
| internal/observe/doc.go | Updates package docs to describe metrics-focused observe package. |
| docs/implementation-plans/MIGRATION.md | Updates migration guide examples to configure slog directly. |
| docs/OBSERVABILITY.md | Updates observability guide to describe direct slog usage and metrics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+146
to
149
| slog.Warn("Failed to remove temp file", | ||
| slog.String("entry_id", entryID), | ||
| slog.String("error", rmErr.Error())) | ||
| } |
There was a problem hiding this comment.
The attribute lines inside this slog.Warn call are not gofmt-formatted/indented (the slog.String(...) args align with slog.Warn rather than being indented under it). Please run gofmt (or fix indentation) so the call is consistently formatted and readable.
Comment on lines
+164
to
167
| slog.Warn("Failed to remove temp file", | ||
| slog.String("entry_id", entryID), | ||
| slog.String("error", rmErr.Error())) | ||
| } |
There was a problem hiding this comment.
This slog.Warn call also appears to have incorrect indentation for its attributes (likely needs gofmt). Consistent formatting matters here because this error-handling path is already nested and hard to scan.
| // Export metrics in Prometheus text format for scraping: | ||
| // | ||
| // exporter := observe.NewPrometheusExporter(metrics, limits) | ||
| // http.Handle("/metrics", exporter) |
There was a problem hiding this comment.
The doc example suggests http.Handle("/metrics", exporter), but *observe.PrometheusExporter does not implement http.Handler (it only exposes WriteMetrics(io.Writer)). Update the example to use http.HandleFunc (or provide a Handler method) so the snippet is correct and copy/pasteable.
Suggested change
| // http.Handle("/metrics", exporter) | |
| // http.HandleFunc("/metrics", func(w http.ResponseWriter, r *http.Request) { | |
| // exporter.WriteMetrics(w) | |
| // }) |
| ↓ ↓ | ||
| ┌────────────────────┐ ┌───────────────────────┐ | ||
| │ log/slog │ │ observe.Metrics │ ← Operational counters/gauges | ||
| │ (structured logs)│ │ PrometheusExporter │ ← /metrics endpoint |
There was a problem hiding this comment.
In the architecture diagram, PrometheusExporter is described as the /metrics endpoint, but the exporter type doesn't implement http.Handler (it exposes WriteMetrics(io.Writer)). Consider adjusting the diagram/text to show serving via http.HandleFunc (or add a ServeHTTP/Handler() helper) so the documentation matches the actual API.
Suggested change
| │ (structured logs)│ │ PrometheusExporter │ ← /metrics endpoint | |
| │ (structured logs)│ │ PrometheusExporter │ ← metrics exporter (used by /metrics handler) |
Comment on lines
219
to
223
| ```go | ||
| logger.ActionReceived("increment", userID) // DO NOT use user IDs, session IDs, etc. | ||
| slog.Info("Action received", | ||
| slog.String("action", "increment"), | ||
| slog.String("user_id", userID)) // DO NOT use user IDs, session IDs, etc. in metric labels | ||
| ``` |
There was a problem hiding this comment.
This section is about metric cardinality/labels, but the example shown is a log event (slog.Info(...)). That can be misleading because log field cardinality has different tradeoffs than metric label cardinality. Consider rewording to clarify you're discussing metric labels, or update the example to use the metrics API / Prometheus labels instead of log attributes.
Comment on lines
+133
to
136
| "time" | ||
| "github.com/livetemplate/livetemplate" | ||
| "github.com/livetemplate/livetemplate/internal/observe" | ||
| ) |
There was a problem hiding this comment.
The example imports github.com/livetemplate/livetemplate/internal/observe. Because this is an internal/ package, it cannot be imported by applications outside the github.com/livetemplate/livetemplate module (Go will reject it). If this guide is intended for library consumers, consider exposing metrics via a public package (e.g., observe at the module root) or document an alternative that doesn't require importing internal/ packages.
Comment on lines
154
to
158
| "os" | ||
| "time" | ||
|
|
||
| "github.com/livetemplate/livetemplate" | ||
| "github.com/livetemplate/livetemplate/internal/observe" | ||
| ) |
There was a problem hiding this comment.
This integration example imports github.com/livetemplate/livetemplate/internal/observe, which external applications cannot import due to Go's internal/ visibility rules. If this doc targets end users, it should avoid internal/ imports (e.g., provide a public metrics package or show how to use only slog plus the built-in /metrics handler if available).
✅ Performance Benchmark ResultsStatus: No significant regressions detected Benchmark ComparisonThresholds
See benchmarking guide for details. |
This was referenced Feb 22, 2026
FOLLOW-UP: Replace slog.String("error", err.Error()) with slog.Any("error", err) (from PR #123)
#124
Closed
adnaan
added a commit
to livetemplate/examples
that referenced
this pull request
Feb 22, 2026
…refs Remove the trace-correlation example directory (relied on deleted internal/observe Trace* types) and update observability/README.md to use slog directly instead of observe.NewLogger(). Companion to livetemplate/livetemplate#123. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 tasks
adnaan
added a commit
to livetemplate/examples
that referenced
this pull request
Feb 22, 2026
…refs (#29) Remove the trace-correlation example directory (relied on deleted internal/observe Trace* types) and update observability/README.md to use slog directly instead of observe.NewLogger(). Companion to livetemplate/livetemplate#123. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Mar 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
log.Printfcalls to structuredsloglogging across 5 production files (mount.go,template.go,session_stores.go,pubsub/redis.go,internal/upload/multipart.go)observe.Loggerandobserve.Trace*wrappers (zero production callers) — theobservepackage now only provides Metrics and Prometheus exportdocs/OBSERVABILITY.mdanddocs/implementation-plans/MIGRATION.mdto reflect directslogusageCloses #100
Test plan
GOWORK=off go test -timeout 300s ./...)log.Printfin production code (onlyinternal/testutil/redis.goexcluded by design)internal/observepackage compiles and tests pass after dead code removal"log"imports in migrated files🤖 Generated with Claude Code