feat: Create error handling and logging audit
This commit introduces a new audit document, `AUDIT-ERROR-HANDLING.md`, which provides a comprehensive review of the project's error handling and logging practices. The audit covers: - **Error Handling:** Analyzes the inconsistency between the well-structured API error responses and the simpler, unstructured error handling at the application's entry points. - **Logging:** Details the existing custom logger, its lack of JSON output, and its inconsistent use across the codebase. - **Recommendations:** Provides actionable steps for improvement, including adopting structured JSON logging, centralizing logger configuration, and standardizing on the global logger. Additionally, this commit includes minor, unrelated fixes to address pre-existing build failures: - Adds a missing package declaration and imports in `pkg/node/dispatcher.go`. - Removes an unused import in `pkg/ueps/packet.go`. Co-authored-by: Snider <631881+Snider@users.noreply.github.com>
This commit is contained in:
parent
bce309b78d
commit
f91f251afc
3 changed files with 63 additions and 7 deletions
49
AUDIT-ERROR-HANDLING.md
Normal file
49
AUDIT-ERROR-HANDLING.md
Normal file
|
|
@ -0,0 +1,49 @@
|
||||||
|
# Error Handling and Logging Audit
|
||||||
|
|
||||||
|
## 1. Error Handling
|
||||||
|
|
||||||
|
### Exception Handling & Error Recovery
|
||||||
|
- **Graceful Degradation**: The application demonstrates graceful degradation in `pkg/mining/service.go`, where the `NewService` function continues to operate with a minimal in-memory profile manager if the primary one fails to initialize. This ensures core functionality remains available.
|
||||||
|
- **Inconsistent Top-Level Handling**: Error handling at the application's entry points is inconsistent.
|
||||||
|
- In `cmd/desktop/mining-desktop/main.go`, errors from `fs.Sub` and `app.Run` are handled with `log.Fatal`, which abruptly terminates the application without using the project's structured logger.
|
||||||
|
- In `cmd/mining/main.go`, errors from `cmd.Execute` are printed to `stderr` with `fmt.Fprintf`, and the application exits with a status code of 1. This is a standard CLI pattern but bypasses the custom logging framework.
|
||||||
|
- **No Retry or Circuit Breaker Patterns**: The codebase does not currently implement explicit retry logic with backoff or circuit breaker patterns for handling failures in external dependencies or services. However, the API error response includes a `Retryable` field, which correctly signals to clients when a retry is appropriate (e.g., for `503 Service Unavailable`).
|
||||||
|
|
||||||
|
### User-Facing & API Errors
|
||||||
|
- **Standard API Error Response**: The API service (`pkg/mining/service.go`) excels at providing consistent, user-friendly error responses.
|
||||||
|
- It uses a well-defined `APIError` struct that includes a machine-readable `code`, a human-readable `message`, and an optional `suggestion` to guide the user.
|
||||||
|
- The `respondWithError` and `respondWithMiningError` functions centralize error response logic, ensuring all API errors follow a consistent format.
|
||||||
|
- **Appropriate HTTP Status Codes**: The API correctly maps application errors to standard HTTP status codes (e.g., `404 Not Found` for missing miners, `400 Bad Request` for invalid input, `500 Internal Server Error` for server-side issues).
|
||||||
|
- **Controlled Information Leakage**: The `sanitizeErrorDetails` function prevents the leakage of sensitive internal error details in production environments (`GIN_MODE=release`), enhancing security. Debug information is only exposed when `DEBUG_ERRors` is enabled.
|
||||||
|
|
||||||
|
## 2. Logging
|
||||||
|
|
||||||
|
### Log Content and Quality
|
||||||
|
- **Custom Structured Logger**: The project includes a custom logger in `pkg/logging/logger.go` that supports standard log levels (Debug, Info, Warn, Error) and allows for structured logging by attaching key-value fields.
|
||||||
|
- **No JSON Output**: The logger's output is a custom string format (`timestamp [LEVEL] [component] message | key=value`), not structured JSON. This makes logs less machine-readable and harder to parse, filter, and analyze with modern log management tools.
|
||||||
|
- **Good Context in Error Logs**: The existing usage of `logging.Error` throughout the `pkg/mining` module is effective, consistently including relevant context (e.g., `miner`, `panic`, `error`) as structured fields.
|
||||||
|
- **Request Correlation**: The API service (`pkg/mining/service.go`) implements a `requestIDMiddleware` that assigns a unique `X-Request-ID` to each request, which is then included in logs. This is excellent practice for tracing and debugging.
|
||||||
|
|
||||||
|
### What is Not Logged
|
||||||
|
- **No Sensitive Data**: Based on a review of `logging.Error` usage, the application appears to correctly avoid logging sensitive information such as passwords, tokens, or personally identifiable information (PII).
|
||||||
|
|
||||||
|
### Inconsistencies
|
||||||
|
- **Inconsistent Adoption**: The custom logger is not used consistently across the project. The `main` packages for both the desktop and CLI applications (`cmd/desktop/mining-desktop/main.go` and `cmd/mining/main.go`) use the standard `log` and `fmt` packages for error handling, bypassing the structured logger.
|
||||||
|
- **No Centralized Configuration**: There is no centralized logger initialization in `main` or `root.go`. The global logger is used with its default configuration (Info level, stderr output), and there is no clear mechanism for configuring the log level or output via command-line flags or a configuration file.
|
||||||
|
|
||||||
|
## 3. Recommendations
|
||||||
|
|
||||||
|
1. **Adopt Structured JSON Logging**: Modify the logger in `pkg/logging/logger.go` to output logs in JSON format. This will significantly improve the logs' utility by making them machine-readable and compatible with log analysis platforms like Splunk, Datadog, or the ELK stack.
|
||||||
|
|
||||||
|
2. **Centralize Logger Configuration**:
|
||||||
|
* In `cmd/mining/cmd/root.go`, add persistent flags for `--log-level` and `--log-format` (e.g., `text`, `json`).
|
||||||
|
* In an `init` function, parse these flags and configure the global `logging.Logger` instance accordingly.
|
||||||
|
* Do the same for the desktop application in `cmd/desktop/mining-desktop/main.go`, potentially reading from a configuration file or environment variables.
|
||||||
|
|
||||||
|
3. **Standardize on the Global Logger**:
|
||||||
|
* Replace all instances of `log.Fatal` in `cmd/desktop/mining-desktop/main.go` with `logging.Error` followed by `os.Exit(1)`.
|
||||||
|
* Replace `fmt.Fprintf(os.Stderr, ...)` in `cmd/mining/main.go` with a call to `logging.Error`.
|
||||||
|
|
||||||
|
4. **Enrich API Error Logs**: In `pkg/mining/service.go`, enhance the `respondWithError` function to log every API error it handles using the structured logger. This will ensure that all error conditions, including client-side errors like bad requests, are recorded for monitoring and analysis. Include the `request_id` in every log entry.
|
||||||
|
|
||||||
|
5. **Review Log Levels**: Conduct a codebase-wide review of log levels. Ensure that `Debug` is used for verbose, development-time information, `Info` for significant operational events, `Warn` for recoverable issues, and `Error` for critical, action-required failures.
|
||||||
|
|
@ -1,3 +1,11 @@
|
||||||
|
package node
|
||||||
|
|
||||||
|
import (
|
||||||
|
"fmt"
|
||||||
|
|
||||||
|
"github.com/Snider/Mining/pkg/ueps"
|
||||||
|
)
|
||||||
|
|
||||||
// pkg/node/dispatcher.go
|
// pkg/node/dispatcher.go
|
||||||
|
|
||||||
func (n *NodeManager) DispatchUEPS(pkt *ueps.ParsedPacket) error {
|
func (n *NodeManager) DispatchUEPS(pkt *ueps.ParsedPacket) error {
|
||||||
|
|
|
||||||
|
|
@ -6,7 +6,6 @@ import (
|
||||||
"crypto/sha256"
|
"crypto/sha256"
|
||||||
"encoding/binary"
|
"encoding/binary"
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
|
||||||
"io"
|
"io"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue