Standardize API error logs#70
Merged
Merged
Conversation
Go coverage report (utils) |
Go coverage report (api) |
Go coverage report (webserver) |
There was a problem hiding this comment.
Pull request overview
Adds a small internal logging wrapper to standardize error log prefixes across the API, addressing issue #69 (“ERROR:” prefix on error log lines).
Changes:
- Introduce
api/internal/logginghelpers (Error,Errorf,Fatalf) that prefix logs withERROR:. - Update contact/email/app startup code paths to use the new helpers instead of
log/fmtfor error logging. - Ensure the API entrypoint uses the standardized fatal error logging.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
api/internal/logging/logging.go |
Adds centralized error logging helpers that prepend ERROR:. |
api/internal/contact/service.go |
Routes email-building/sending error logs through internal/logging. |
api/internal/app/run.go |
Standardizes statsd init failure logging via internal/logging. |
api/internal/app/email.go |
Replaces fmt.Println error output with standardized logging helper. |
api/cmd/api/main.go |
Replaces log.Fatal with standardized logging.Fatalf. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5
to
+16
| func Error(args ...interface{}) { | ||
| args = append([]interface{}{"ERROR:"}, args...) | ||
| log.Println(args...) | ||
| } | ||
|
|
||
| func Errorf(format string, args ...interface{}) { | ||
| log.Printf("ERROR: "+format, args...) | ||
| } | ||
|
|
||
| func Fatalf(format string, args ...interface{}) { | ||
| log.Fatalf("ERROR: "+format, args...) | ||
| } |
There was a problem hiding this comment.
The new logging package doesn’t have any unit tests to assert the required "ERROR:" prefix behavior. Since other internal packages are covered by tests, consider adding a small test that captures log output and verifies that Error/Errorf/Fatalf all emit messages containing the standardized prefix.
Comment on lines
+5
to
+15
| func Error(args ...interface{}) { | ||
| args = append([]interface{}{"ERROR:"}, args...) | ||
| log.Println(args...) | ||
| } | ||
|
|
||
| func Errorf(format string, args ...interface{}) { | ||
| log.Printf("ERROR: "+format, args...) | ||
| } | ||
|
|
||
| func Fatalf(format string, args ...interface{}) { | ||
| log.Fatalf("ERROR: "+format, args...) |
There was a problem hiding this comment.
The "ERROR:" prefix is duplicated and applied slightly differently across helpers (Error prepends "ERROR:" as an arg, while Errorf/Fatalf embed "ERROR: " in the format string). Consider centralizing the prefix in a constant and applying it consistently so log parsing is predictable (including spacing).
Suggested change
| func Error(args ...interface{}) { | |
| args = append([]interface{}{"ERROR:"}, args...) | |
| log.Println(args...) | |
| } | |
| func Errorf(format string, args ...interface{}) { | |
| log.Printf("ERROR: "+format, args...) | |
| } | |
| func Fatalf(format string, args ...interface{}) { | |
| log.Fatalf("ERROR: "+format, args...) | |
| const errorPrefix = "ERROR: " | |
| func Error(args ...interface{}) { | |
| args = append([]interface{}{errorPrefix}, args...) | |
| log.Println(args...) | |
| } | |
| func Errorf(format string, args ...interface{}) { | |
| log.Printf(errorPrefix+format, args...) | |
| } | |
| func Fatalf(format string, args ...interface{}) { | |
| log.Fatalf(errorPrefix+format, args...) |
Comment on lines
+3
to
+15
| import "log" | ||
|
|
||
| func Error(args ...interface{}) { | ||
| args = append([]interface{}{"ERROR:"}, args...) | ||
| log.Println(args...) | ||
| } | ||
|
|
||
| func Errorf(format string, args ...interface{}) { | ||
| log.Printf("ERROR: "+format, args...) | ||
| } | ||
|
|
||
| func Fatalf(format string, args ...interface{}) { | ||
| log.Fatalf("ERROR: "+format, args...) |
There was a problem hiding this comment.
These helpers wrap the stdlib log package, which means if log flags ever include file/line (e.g., Lshortfile/Llongfile), the reported callsite will be inside logging.go rather than the original caller. To preserve correct callsites, consider using log.Default().Output with an adjusted call depth (and formatting the message yourself) instead of calling log.Printf/log.Println directly.
Suggested change
| import "log" | |
| func Error(args ...interface{}) { | |
| args = append([]interface{}{"ERROR:"}, args...) | |
| log.Println(args...) | |
| } | |
| func Errorf(format string, args ...interface{}) { | |
| log.Printf("ERROR: "+format, args...) | |
| } | |
| func Fatalf(format string, args ...interface{}) { | |
| log.Fatalf("ERROR: "+format, args...) | |
| import ( | |
| "fmt" | |
| "log" | |
| "os" | |
| "strings" | |
| ) | |
| func Error(args ...interface{}) { | |
| args = append([]interface{}{"ERROR:"}, args...) | |
| _ = log.Default().Output(2, strings.TrimSuffix(fmt.Sprintln(args...), "\n")) | |
| } | |
| func Errorf(format string, args ...interface{}) { | |
| _ = log.Default().Output(2, fmt.Sprintf("ERROR: "+format, args...)) | |
| } | |
| func Fatalf(format string, args ...interface{}) { | |
| _ = log.Default().Output(2, fmt.Sprintf("ERROR: "+format, args...)) | |
| os.Exit(1) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+55
to
+61
| if os.Getenv("LOGGING_FATALF_TEST") == "1" { | ||
| log.SetOutput(os.Stderr) | ||
| log.SetFlags(0) | ||
| log.SetPrefix("") | ||
| Fatalf("unable to send email %d", 42) | ||
| return | ||
| } |
There was a problem hiding this comment.
TestFatalfAddsPrefixAndExits will take the helper-process branch (and call os.Exit(1)) if LOGGING_FATALF_TEST is already set in the caller’s environment. To make the test robust, explicitly clear/override this env var in the parent process (e.g., via t.Setenv before the getenv check) and only set it to "1" for the exec’d child process.
| statsConn, err := statsd.New(statsd.Address(cfg.StatsdAddress)) | ||
| if err != nil { | ||
| log.Printf("warning: statsd initialization failed: %v", err) | ||
| logging.Errorf("statsd initialization failed: %v", err) |
There was a problem hiding this comment.
This error is logged but the application continues to run without StatsD (statsConn can be nil). The updated message drops the previous “warning”/non-fatal signal, which can be misleading operationally; consider including explicit wording like “continuing without metrics” (while still keeping the required "ERROR:" prefix).
Suggested change
| logging.Errorf("statsd initialization failed: %v", err) | |
| logging.Errorf("ERROR: statsd initialization failed: %v; continuing without metrics", err) |
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.
fixes #69