Add functionality to upload log-file #488
Conversation
WalkthroughThe changes introduce support for uploading log files to a specified URL after job completion. This is achieved by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Logger
participant LogUploader
participant HTTPServer
Main->>Logger: setupTargetLogger(flags, logTarget, logUploadTarget, commandOutput)
Logger->>Logger: getLogfilePath(logTarget)
Logger->>Logger: getFileHandler(resolvedPath)
Logger->>Logger: createLogUploadingLogHandler(handler, resolvedPath, logUploadTarget)
Logger-->>Main: returns log handler (possibly wrapped with uploader)
Main->>Logger: closer.Close()
alt logUploadTarget and logfilePath set
Logger->>LogUploader: logUploadingLogCloser.Close()
LogUploader->>Logger: Close embedded log handler
LogUploader->>LogUploader: Open log file
LogUploader->>HTTPServer: POST log file to logUploadTarget
HTTPServer-->>LogUploader: HTTP response
alt HTTP 2xx
LogUploader-->>Main: success
else HTTP error or failure
LogUploader-->>Main: error
end
else
Logger-->>Main: Close log handler (no upload)
end
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
main.go (1)
121-129: Good error handling for log closure, but consider adding a newline.The error handling for the log closer is a good improvement, especially since log upload failures need to be reported. However, the error message doesn't end with a newline, which might lead to awkward terminal output if other messages follow.
- fmt.Fprintf(os.Stderr, "Error closing logfile: %v", err) + fmt.Fprintf(os.Stderr, "Error closing logfile: %v\n", err)logger_test.go (1)
151-161: Use HTTP status constants from the standard library instead of magic numbers.Using constants improves readability and maintainability.
- w.WriteHeader(500) + w.WriteHeader(http.StatusInternalServerError)Also, the test correctly verifies that the upload failure is reported in the error message.
🧰 Tools
🪛 golangci-lint (1.64.8)
154-154: "500" can be replaced by http.StatusInternalServerError
(usestdlibvars)
logger.go (3)
7-7: Consider using context for HTTP requests.
Importing"net/http"is appropriate, but you may want to use a context-based approach (http.NewRequestWithContext) for uploading logs to handle timeouts and cancellations gracefully.
54-59: Validate logUploadTarget if necessary.
PassinglogUploadTargetdirectly works, but you might want to validate it or confirm it can resolve to a valid URL before usage. This extra check can help avoid edge cases where an invalid URL is silently ignored.
143-155: Report errors when creating temp directories.
Silently discarding the error if we fail to retrieve or create temp storage might hide issues. Consider logging it to help diagnose potential filesystem problems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.vscode/launch.json(1 hunks)config/global.go(1 hunks)logger.go(5 hunks)logger_test.go(3 hunks)main.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
logger.go (1)
dial/url.go (2)
GetAddr(24-38)IsURL(47-50)
🪛 golangci-lint (1.64.8)
logger_test.go
175-175: ineffectual assignment to err
(ineffassign)
154-154: "500" can be replaced by http.StatusInternalServerError
(usestdlibvars)
170-170: "200" can be replaced by http.StatusOK
(usestdlibvars)
logger.go
113-113: httpresponse: using resp before checking for errors
(govet)
112-112: net/http.Post must not be called
(noctx)
🔇 Additional comments (6)
config/global.go (1)
30-30: LGTM! Good addition of the LogUploadUrl configuration.The new configuration field follows the same pattern as other fields in the struct, with appropriate MapStructure tag and description.
.vscode/launch.json (1)
14-22: LGTM! Convenient debug configuration added.This is helpful for developers to quickly test with a specific configuration file.
logger_test.go (1)
45-46: LGTM! Properly updated to use the new getLogfilePath function.The test has been correctly updated to match the refactored logger implementation.
logger.go (3)
66-67: Confirm write permissions for log files.
This logic is clear; just ensure errors from creating or appending to the log file are properly captured. The current approach already propagates them viaerr.
85-86: Scrutinise potential disclosure of sensitive information.
WhenlogUploadTargetis set and logs are uploaded, there is a risk of sending sensitive data. Confirm whether logs may contain credentials or personal details, and consider redacting private information if needed.
157-179: File handler creation appears correct.
The logic to create adeferredFileWriterand wrap it in aclog.StandardLogHandleris handled properly.
logger_test.go
Outdated
| func TestLogUpload(t *testing.T) { | ||
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| buffer := bytes.Buffer{} | ||
| _, err := buffer.ReadFrom(r.Body) | ||
| assert.NoError(t, err) | ||
| r.Body.Close() | ||
|
|
||
| w.WriteHeader(200) | ||
| assert.Equal(t, strings.Trim(buffer.String(), "\r\n"), "TestLogLine") | ||
| }) | ||
| server := httptest.NewServer(handler) | ||
| defer server.Close() | ||
| closer, err := setupTargetLogger(commandLineFlags{}, filepath.Join(constants.TemporaryDirMarker, "file.log"), server.URL, "log") | ||
| _, err = term.Println("TestLogLine") | ||
| assert.NoError(t, err) | ||
| assert.NoError(t, closer.Close()) | ||
| } |
There was a problem hiding this comment.
Fix ineffectual error assignment and use HTTP status constants.
There are two issues with this test:
- The error from setupTargetLogger is never checked
- Numeric HTTP status code is used instead of the standard library constant
Apply these fixes:
server := httptest.NewServer(handler)
defer server.Close()
closer, err := setupTargetLogger(commandLineFlags{}, filepath.Join(constants.TemporaryDirMarker, "file.log"), server.URL, "log")
+assert.NoError(t, err)
- _, err = term.Println("TestLogLine")
+ _, printErr := term.Println("TestLogLine")
- assert.NoError(t, err)
+ assert.NoError(t, printErr)
assert.NoError(t, closer.Close())- w.WriteHeader(200)
+ w.WriteHeader(http.StatusOK)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestLogUpload(t *testing.T) { | |
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| buffer := bytes.Buffer{} | |
| _, err := buffer.ReadFrom(r.Body) | |
| assert.NoError(t, err) | |
| r.Body.Close() | |
| w.WriteHeader(200) | |
| assert.Equal(t, strings.Trim(buffer.String(), "\r\n"), "TestLogLine") | |
| }) | |
| server := httptest.NewServer(handler) | |
| defer server.Close() | |
| closer, err := setupTargetLogger(commandLineFlags{}, filepath.Join(constants.TemporaryDirMarker, "file.log"), server.URL, "log") | |
| _, err = term.Println("TestLogLine") | |
| assert.NoError(t, err) | |
| assert.NoError(t, closer.Close()) | |
| } | |
| func TestLogUpload(t *testing.T) { | |
| handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| buffer := bytes.Buffer{} | |
| _, err := buffer.ReadFrom(r.Body) | |
| assert.NoError(t, err) | |
| r.Body.Close() | |
| w.WriteHeader(http.StatusOK) | |
| assert.Equal(t, strings.Trim(buffer.String(), "\r\n"), "TestLogLine") | |
| }) | |
| server := httptest.NewServer(handler) | |
| defer server.Close() | |
| closer, err := setupTargetLogger(commandLineFlags{}, filepath.Join(constants.TemporaryDirMarker, "file.log"), server.URL, "log") | |
| assert.NoError(t, err) | |
| _, printErr := term.Println("TestLogLine") | |
| assert.NoError(t, printErr) | |
| assert.NoError(t, closer.Close()) | |
| } |
🧰 Tools
🪛 golangci-lint (1.64.8)
175-175: ineffectual assignment to err
(ineffassign)
170-170: "200" can be replaced by http.StatusOK
(usestdlibvars)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
logger.go (1)
115-120: Ensure response body is always closed.The response body should be closed regardless of the status code check. While your code does defer the close operation, it's better to handle any potential nil response cases with a more defensive approach.
resp, err := client.Post(w.logUploadTarget, "application/octet-stream", logData) if err != nil { return err } - defer resp.Body.Close() + defer func() { + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + }()🧰 Tools
🪛 golangci-lint (1.64.8)
116-116: (*net/http.Client).Post must not be called
(noctx)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
logger.go(5 hunks)logger_test.go(3 hunks)main.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- main.go
- logger_test.go
🧰 Additional context used
🪛 golangci-lint (1.64.8)
logger.go
116-116: (*net/http.Client).Post must not be called
(noctx)
🔇 Additional comments (5)
logger.go (5)
54-94: LGTM! New log upload URL capability properly integrated.The changes to
setupTargetLoggerfunction signature and implementation correctly add support for log file uploading. The function maintains its original functionality while adding the new feature in a clean way.
85-90: Good validation of the log upload URL.The code correctly checks that both the log upload target is a valid URL and that we have a valid filepath before proceeding with the upload functionality.
129-131: LGTM! Clear factory function for log handler.This is a good use of the factory pattern to create the log upload decorator. The function is concise and easy to understand.
147-159: Good refactoring of path handling.Extracting the logic for normalising log file paths into a separate function improves code organisation and makes the functionality more reusable.
161-183: LGTM! Clean separation of concerns.The modified
getFileHandlerfunction now focuses solely on creating the file handler, which is a good separation of concerns from the path normalisation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
logger.go (1)
116-116: 🛠️ Refactor suggestionReplace context.TODO() with a proper cancellation context.
Using
context.TODO()is generally a placeholder. Since you're already setting a timeout on the HTTP client, it would be better to use a context with proper cancellation handling.- req, err := http.NewRequestWithContext(context.TODO(), "POST", w.logUploadTarget, logData) + ctx, cancel := context.WithTimeout(context.Background(), 30 * time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodPost, w.logUploadTarget, logData)🧰 Tools
🪛 golangci-lint (1.64.8)
116-116: "POST" can be replaced by http.MethodPost
(usestdlibvars)
🧹 Nitpick comments (1)
logger.go (1)
116-116: Use standard HTTP method constant instead of string literal.Replace the string "POST" with the standard library constant for better maintainability.
- req, err := http.NewRequestWithContext(context.TODO(), "POST", w.logUploadTarget, logData) + req, err := http.NewRequestWithContext(context.TODO(), http.MethodPost, w.logUploadTarget, logData)🧰 Tools
🪛 golangci-lint (1.64.8)
116-116: "POST" can be replaced by http.MethodPost
(usestdlibvars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
logger.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
logger.go (2)
dial/url.go (2)
GetAddr(24-38)IsURL(47-50)remote/client.go (1)
Client(13-17)
🪛 golangci-lint (1.64.8)
logger.go
116-116: "POST" can be replaced by http.MethodPost
(usestdlibvars)
🔇 Additional comments (8)
logger.go (8)
4-8: Update imports to handle the new HTTP client functionality.Good additions of "context" and "net/http" imports needed for the log upload feature.
55-55: Function signature updated to support log file uploading.The signature change properly accommodates the new
logUploadTargetparameter needed for upload functionality.
67-68: Good refactoring of log file path resolution logic.Extracting the path normalization to a separate function improves code organization and reusability.
86-91: Proper validation of upload URL before attempting file upload.The code correctly checks both that the upload target is provided and that it's a valid URL before wrapping the handler.
97-101: Well-structured type for log upload functionality.The
logUploadingLogCloserstruct properly embeds theLogCloserinterface while adding the necessary fields for upload functionality.
127-130: Include HTTP status code and response body in error message.The error handling is good, including both the status code and response body for debugging. This will be helpful when troubleshooting failed uploads.
134-136: Clean, concise factory function for the log upload handler.The
createLogUploadingLogHandlerfunction provides a nice encapsulation of creating the uploading wrapper.
152-164: Good extraction of path normalization logic.Moving the log file path resolution logic to a separate function improves code organization and makes the code more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
logger.go (2)
124-130: Client timeout is redundant but not harmful.You've applied both a context timeout (line 116) and a client-level timeout (line 124). While this is redundant, it provides dual protection against hanging requests so it's not problematic.
- client := &http.Client{Timeout: 30 * time.Second} + client := &http.Client{}
116-136: Consider handling large log files more efficiently.For very large log files, reading the entire file into memory before uploading could cause memory pressure. Consider using streaming or chunking for large files.
- logData, err := os.Open(w.logfilePath) - if err != nil { - return err - } + // Get file stat to check size + fileStat, err := os.Stat(w.logfilePath) + if err != nil { + return err + } + + logData, err := os.Open(w.logfilePath) + if err != nil { + return err + } + defer logData.Close() + + // For very large files, consider using a different approach + const largeFileSizeThreshold = 100 * 1024 * 1024 // 100 MB + if fileStat.Size() > largeFileSizeThreshold { + clog.Warningf("Log file size %d bytes exceeds threshold, upload may use significant memory", fileStat.Size()) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
logger.go(5 hunks)
🔇 Additional comments (11)
logger.go (11)
4-8: Appropriate imports added for the new functionality.The addition of the "context" and "net/http" imports is necessary for implementing the log upload functionality using HTTP requests with proper context management.
55-62: Function signature updated correctly for new capability.The function signature has been appropriately updated to accept the
logUploadTargetparameter, and the local variablefilepathhas been added to store the normalized log file path, which is a good practice for maintaining clarity.
67-68: Good refactoring of file path handling.The extraction of path normalization logic into a separate
getLogfilePathfunction improves code organization by separating concerns.
86-91: Strong validation for log upload configuration.This block ensures that log uploading only occurs when both a valid URL and file path are provided, with clear error messaging when the URL is invalid.
97-101: Clean struct definition for log uploading handler.The
logUploadingLogCloserstruct follows Go's composition pattern by embedding theLogCloserinterface and adding the necessary fields for upload functionality.
105-115: Sequential error handling in Close method.The method properly handles errors from the original handler's Close method before attempting to open the log file, ensuring proper cleanup.
116-123: HTTP request setup follows best practices.The implementation follows Go best practices by using context with timeout for cancellation support and setting the appropriate Content-Type header.
131-134: Thorough status code checking and error reporting.The code correctly validates HTTP response status codes and provides informative error messages that include both the status code and response body content.
138-140: Clean factory function implementation.The
createLogUploadingLogHandlerfunction provides a clean way to create the log uploading handler, following Go's functional construction pattern.
156-168: Well-extracted path normalization function.The
getLogfilePathfunction cleanly extracts the path normalization logic from the originalgetFileHandlerfunction, improving code organization and readability.
170-192: Simplified file handler creation.The
getFileHandlerfunction is now focused solely on creating the file handler, which improves separation of concerns after extracting the path normalization logic.
| func (w logUploadingLogCloser) Close() error { | ||
| err := w.LogCloser.Close() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Open logfile for reading | ||
| logData, err := os.Open(w.logfilePath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Upload logfile to server | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, w.logUploadTarget, logData) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| req.Header.Set("Content-Type", "application/octet-stream") | ||
|
|
||
| client := &http.Client{Timeout: 30 * time.Second} | ||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
| // HTTP-Status-Codes 200-299 signal success, return an error for everything else | ||
| if resp.StatusCode < 200 || resp.StatusCode > 299 { | ||
| respBody, _ := io.ReadAll(resp.Body) | ||
| return fmt.Errorf("log-upload: Got invalid http status %v: %v", resp.StatusCode, string(respBody)) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify potential race conditions with log file uploading.
While the code correctly closes the original log handler before uploading, there could be a risk of uploading an incomplete log file if other processes are still writing to it.
Consider checking if there are any safeguards in place to ensure the log file is fully written before upload. You may want to add a small delay or use file locking if available in your target environments.
🏁 Script executed:
#!/bin/bash
# Check if there are any file locking or synchronization mechanisms elsewhere in the codebase
rg -A 3 -B 3 "file.*lock|flock|sync.*file" --ignore-caseLength of output: 56016
Action: Address potential race condition in log file upload
After reviewing the code and repository, it appears that while the log file is closed via w.LogCloser.Close() before being re-opened for upload, there is no explicit safeguard—such as file locking or a delay—to ensure that no concurrent writes occur during or immediately after this close. In our search, no synchronisation mechanism was identified in the logger code to guarantee that the logfile is fully flushed and complete before being uploaded.
- File locking or delay: Consider implementing file locking on the log file during the upload process or adding a short delay to ensure that any pending writes have finished.
- Concurrent writes risk: Ensure that no other process or goroutine writes to the log file once the original handler is closed, or introduce additional safeguards if such concurrent writes are possible.
Please review your logging and file upload strategy to mitigate any risk of uploading incomplete logs.
|
Hey! Thanks for this PR, I think it will be useful indeed. The issue in the tests are fixed: you need to rebase your branch from |
Hello,
I use restic to backup different machines, including some windows machines.
To monitor for any problems, I like to receive the log-files on some central server to check for problems.
Since on windows syslog is not supported, that sadly is not an option.
So I added a config option to upload the log-file to some configured URL.
Because I think this could be useful for somebody else I opened this pull request.
Greetings
FH