Fix 51 smartctlr metrics od there serve #53
Conversation
WalkthroughThis pull request introduces a new API endpoint for SMART metrics. A new route ( Changes
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
🪧 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 (
|
|
@mertssmnoglu have drafted first compatible code, let me know what needs to be renewed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/handler/metrics.go (1)
43-47: Apply goimports for consistent formattingThe file is flagged by static analysis as not goimports-ed. Please run goimports to standardize import ordering and formatting.
🧰 Tools
🪛 golangci-lint (1.62.2)
45-45: File is not
goimports-ed(goimports)
internal/metric/metric.go (1)
23-41: Switch to numeric types for numerical fieldsThese fields (e.g., temperature, power cycles) might represent numeric data. Storing them as numbers could simplify computations or aggregations.
internal/metric/smart_metrics.go (3)
1-9: Apply goimports for formattingThe file is flagged as not being goimports-ed. Please run goimports to address any formatting or import ordering discrepancies.
11-27: Use %w to wrap errors for better unwrappingWhen returning errors with
fmt.Errorf, consider using the%wverb to wrap the underlying error, allowing for more seamless error checking and unwrapping later.🧰 Tools
🪛 golangci-lint (1.62.2)
15-15: non-wrapping format verb for fmt.Errorf. Use
%wto format errors(errorlint)
117-130: Use errors.As for exit error checks & %w for wrapping• Use
errors.As(err, &exitErr)instead of a direct type assertion to handle any wrapped errors properly.
• Replace%vwith%winfmt.Errorfcalls to maintain error context.🧰 Tools
🪛 golangci-lint (1.62.2)
121-121: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors
(errorlint)
126-126: non-wrapping format verb for fmt.Errorf. Use
%wto format errors(errorlint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/capture/main.go(1 hunks)internal/handler/metrics.go(1 hunks)internal/metric/metric.go(2 hunks)internal/metric/smart_metrics.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
internal/handler/metrics.go
45-45: File is not goimports-ed
(goimports)
internal/metric/metric.go
17-17: File is not goimports-ed
(goimports)
internal/metric/smart_metrics.go
51-51: File is not goimports-ed
(goimports)
15-15: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
121-121: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors
(errorlint)
126-126: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
🔇 Additional comments (6)
cmd/capture/main.go (1)
53-54: Route registration looks goodThis new endpoint properly extends the metrics API to include SMART data.
internal/metric/metric.go (4)
16-19: Well-structured container for SMART dataThe
SmartMetricstruct effectively encapsulates SMART data alongside any associated errors.🧰 Tools
🪛 golangci-lint (1.62.2)
17-17: File is not
goimports-ed(goimports)
21-22: Implements the Metric interfaceThis no-op method ensures
SmartMetricfulfills theMetricinterface contract.
43-44: Consistent interface implementationThis method is consistent with the existing pattern for implementing
Metric.
127-127: Return statement aligns with overall structureThe final return of
AllMetricsand the collected errors aligns with the existing patterns in this file.internal/metric/smart_metrics.go (1)
132-154: Confirm handling multiple devicesOverwriting
metricsfor each device returns only the last device’s data. If you intend to handle multiple devices, consider aggregating them in a slice or combined structure.Would you like me to propose a refactor that accumulates metrics for all devices?
There was a problem hiding this comment.
Hey, thanks for this PR! @Owaiseimdad
I manually tested these changes in my PC and it works when I run Capture as root.
- Can we check if smartctl command is installed or not and early exit if it's not installed. Like executing
command -v smartctland check if error code is 127 or it's valid command. - I noticed non-standard code style, this can be auto fixed with
gofmt -w ./orjust formatcommands. - Instead of declaring another struct with fields
Data and Error, we can use APIResponse.metricmodule functions should return Metric interface.
Conclusion
This is very good to start to expose smartctl metrics in the happy path. But we should care about error handling to merge these changes. I commented on some lines to improve error handling.
Thank you so much.
…de according to the comments.
@mertssmnoglu have made the updates now, also added more readability and maintaince of code in order to make sure that of someone read in future they understand the code. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/metric/metric.go (1)
16-33: Consider using numeric field types for better data manipulation.All fields in
SmartDataare stored as strings, which might make further numeric computations or comparisons more cumbersome. Consider changing those fields to numeric types (e.g., int or float64), while properly handling parsing logic. If you only need them as raw strings for display purposes, please feel free to ignore this suggestion.internal/metric/smart_metrics.go (3)
10-18: Consider usingexec.LookPathfor a more direct check.Instead of running
"command -v smartctl", usingexec.LookPath("smartctl")can be a more idiomatic way in Go to check ifsmartctlis in the system's PATH.
43-74: Consider handling or logging unexpected keys in the output.Right now, any key not included in these switch cases is ignored. If
smartctladds new metrics in the future, those might go unnoticed. Logging them could be helpful.
76-130: Parsing approach may be brittle against future format changes.Manually parsing lines could break if
smartctloutput changes significantly. Consider a more robust strategy or keep this well-documented and tested against multiple versions ofsmartctl.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/handler/metrics.go(1 hunks)internal/metric/metric.go(2 hunks)internal/metric/smart_metrics.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/handler/metrics.go
🔇 Additional comments (6)
internal/metric/metric.go (1)
33-33: No-op method is acceptable.
isMetric()is a marker method to satisfy theMetricinterface. No concerns here.internal/metric/smart_metrics.go (5)
1-9: Good separation of the new metrics logic into its own package.No issues with the package declaration and imports.
20-36: Device scanning logic looks sound.The function accurately captures device paths returned by
smartctl --scan. No immediate issues identified.
38-41: Case-insensitive check for error keywords is appropriate.This logic properly detects "failed" or "error" in any case variant.
132-154: Confirm ignoring exit code 4 is appropriate.According to
smartctldocs, exit code 4 might be non-critical, but please verify this behavior aligns with your desired workflow. If it's truly safe, a clarifying comment may help future maintainers.
156-194: Returning only the last device's SMART data.This function overwrites
metricson each loop iteration, ultimately returning data for only the final device. If you need to collect metrics for all devices, consider storing them in a slice or mapping them by device name.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
internal/metric/metric.go (1)
1-1:⚠️ Potential issueFix formatting issues flagged by pipeline.
The pipeline failure indicates formatting issues with this file. Please run
go fmt ./...orgoimports -w internal/metric/metric.goto fix these issues.#!/bin/bash # Show what the formatting changes would be goimports -d internal/metric/metric.go🧰 Tools
🪛 GitHub Actions: lint
[error] 1-1: File is not properly formatted (goimports)
🧹 Nitpick comments (2)
internal/metric/metric.go (2)
16-31: Add missing SMART metrics fields according to PR objectives.The
SmartDatastruct is well-structured with appropriate JSON tags. However, it's missing some fields that were mentioned in the PR objectives, notably:
error_information_log_entriesmedia_and_data_integrity_errorsThese should be added to ensure comprehensive SMART data collection.
Also, the JSON tag for
SmartOverallHealthResultuses hyphens (smart_overall-health_self-assessment_test_result), which is inconsistent with the underscore format used in other fields.- SmartOverallHealthResult string `json:"smart_overall-health_self-assessment_test_result"` + SmartOverallHealthResult string `json:"smart_overall_health_self_assessment_test_result"` + ErrorInformationLogEntries string `json:"error_information_log_entries"` + MediaAndDataIntegrityErrors string `json:"media_and_data_integrity_errors"`🧰 Tools
🪛 golangci-lint (1.64.8)
17-17: File is not properly formatted
(goimports)
16-31: Consider using appropriate data types for numeric metrics.Currently, all fields in the
SmartDatastruct are defined as strings. For metrics that are naturally numeric (likePercentageUsed,PowerCycles, etc.), consider using appropriate numeric types (int, uint64, float64) to facilitate data processing and validation.For example:
- PowerCycles string `json:"power_cycles"` - PowerOnHours string `json:"power_on_hours"` - PercentageUsed string `json:"percentage_used"` + PowerCycles uint64 `json:"power_cycles"` + PowerOnHours uint64 `json:"power_on_hours"` + PercentageUsed float64 `json:"percentage_used"`This would require proper parsing of the values from the
smartctloutput, but it would make the data more usable for calculations and comparisons.🧰 Tools
🪛 golangci-lint (1.64.8)
17-17: File is not properly formatted
(goimports)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/metric/metric.go(2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
internal/metric/metric.go
17-17: File is not properly formatted
(goimports)
🪛 GitHub Actions: lint
internal/metric/metric.go
[error] 1-1: File is not properly formatted (goimports)
🔇 Additional comments (2)
internal/metric/metric.go (2)
33-33: LGTM: Empty implementation of isMetric() satisfies the Metric interface.This implementation is sufficient to make
SmartDatasatisfy theMetricinterface defined on line 7-9.
16-31: Complete implementation based on previous review feedback.The previous review comments suggest that you've correctly removed the redundant
SmartMetricstruct in favor of usingSmartDatadirectly in the metric module, which aligns with best practices.🧰 Tools
🪛 golangci-lint (1.64.8)
17-17: File is not properly formatted
(goimports)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/metric/smart_metrics.go (2)
42-73: Consider adding validation for unrecognized keysThe
setFieldfunction handles known SMART metrics by mapping them to appropriate fields in theSmartDatastruct, but it silently ignores unrecognized keys. Consider logging unrecognized keys for debugging purposes or returning a boolean to indicate whether the field was set.func setField(key, value string, data *SmartData) { + originalKey := key switch strings.ToLower(key) { case "available spare": data.AvailableSpare = value // ... other cases ... case "unsafe shutdowns": data.UnsafeShutdowns = value + default: + // Optionally log unrecognized keys for debugging + // log.Printf("Unrecognized SMART metric key: %s", originalKey) } }
75-129: Enhance error handling for invalid smart data sectionsThe parsing logic is generally good, but there are a few edge cases to consider:
- When
startMarkeris found but the section is malformed (no second "===" marker), theendIdxcheck may not correctly limit the section.- You're appending errors for any keys containing "error" or "failed" without checking if the value actually indicates an error.
Consider refining these conditions for more accurate error reporting.
// Extract the section starting from the marker section := output[startIdx:] endIdx := strings.Index(section, "===") -if endIdx > len(startMarker) { +if endIdx > 0 && endIdx > len(startMarker) { section = section[:endIdx] } // ... // If the key contains "error" or "failed", add it to the errors if isErrorKey(key) { + // Only report as error if the value indicates an issue + if strings.Contains(strings.ToLower(value), "failed") || strings.Contains(strings.ToLower(value), "error") { errors = append(errors, CustomErr{ Metric: []string{key}, Error: fmt.Sprintf("Unable to retrieve the '%s'", key), }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/metric/smart_metrics.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
internal/metric/smart_metrics.go (1)
internal/metric/metric.go (4) (4)
SmartData(16-31)CustomErr(44-47)errors(97-97)Metric(7-9)
🪛 golangci-lint (1.64.8)
internal/metric/smart_metrics.go
23-23: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
136-136: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors
(errorlint)
159-159: File is not properly formatted
(goimports)
🪛 GitHub Actions: lint
internal/metric/smart_metrics.go
[error] 23-23: non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
[error] 136-136: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[error] 159-159: File is not properly formatted (goimports)
🔇 Additional comments (3)
internal/metric/smart_metrics.go (3)
10-17: Function successfully handles checking forsmartctlThe
checkSmartCtlInstalledfunction properly usesexec.LookPathto determine if the smartctl utility is available in the system path, which addresses the requirement mentioned in the PR comments.
37-40: LGTM! Case-insensitive error key detection implementedThe function correctly implements case-insensitive checks for error indicators by converting the key to lowercase before checking for "failed" or "error" substrings, which addresses the previous review comment.
163-201:❓ Verification inconclusive
Address potential overwriting of metrics from multiple devices
In the current implementation, if multiple devices are found, the metrics from the last device processed will overwrite all previous metrics. This logic may lose valuable information from other devices:
// This will overwrite any previously collected metrics if metric != nil { metrics = *metric }Consider modifying the approach to either:
- Aggregate metrics by device (return a map of device -> metrics)
- Combine metrics with clear labeling to indicate which device they came from
func GetSmartMetrics() (map[string]SmartData, []CustomErr) { - var metrics SmartData + metrics := make(map[string]SmartData) var smartCtlrErrs []CustomErr // ... existing code ... // Iterate over devices and collect metrics for _, device := range devices { metric, metricErr := getMetrics(device) if metric != nil { - metrics = *metric + metrics[device] = *metric } if len(metricErr) > 0 { - smartCtlrErrs = append(smartCtlrErrs, metricErr...) + // Add device information to each error + for i := range metricErr { + metricErr[i].Metric = append([]string{"smart: " + device}, metricErr[i].Metric...) + } + smartCtlrErrs = append(smartCtlrErrs, metricErr...) } } return metrics, smartCtlrErrs }Alternatively, if the current API design requires a single
SmartDataobject to be returned, consider adding additional fields to theSmartDatastruct to indicate which metrics came from which devices.
🏁 Script executed:
#!/bin/bash # Check how the metrics API endpoint is used elsewhere in the codebase # to understand if changing the return type would break existing code # Look for references to GetSmartMetrics rg -A 2 -B 2 "GetSmartMetrics" --type goLength of output: 707
Revisit Metrics Aggregation for Multiple Devices
The implementation currently overwrites metrics from each device, meaning only the last device’s metrics are returned. Note that downstream code (e.g., in
internal/handler/metrics.go, which callsGetSmartMetricsexpecting a singleSmartDatavalue) may be impacted if the API contract is changed. You have two potential directions:
Aggregate Metrics Per Device:
Change the return type from a singleSmartDatato something like a map (or slice) mapping each device to its metrics. For example:func GetSmartMetrics() (map[string]SmartData, []CustomErr) { - var metrics SmartData + metrics := make(map[string]SmartData) var smartCtlrErrs []CustomErr // … existing code … for _, device := range devices { metric, metricErr := getMetrics(device) if metric != nil { - metrics = *metric + metrics[device] = *metric } if len(metricErr) > 0 { - smartCtlrErrs = append(smartCtlrErrs, metricErr...) + // Prefix error metrics with device information + for i := range metricErr { + metricErr[i].Metric = append([]string{"smart: " + device}, metricErr[i].Metric...) + } + smartCtlrErrs = append(smartCtlrErrs, metricErr...) } } return metrics, smartCtlrErrs }Caveat: This change would also require updates in all consumers (for instance, in
internal/handler/metrics.go) that currently expect aSmartDataobject.Enhance the Existing API:
If the API design must remain unchanged, consider extending theSmartDatastructure to support aggregation internally (for example, by adding fields that identify the source device). This way, you can combine metrics from multiple devices without altering the return type.Please verify which approach fits your design goals and ensure any modifications are propagated to dependent parts of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/metric/smart_metrics.go (2)
23-23:⚠️ Potential issueUse error wrapping with
%winstead of%vWhen using
fmt.Errorf, use the%wverb instead of%vto properly wrap the original error. This allows calling code to useerrors.Unwrap()orerrors.Is()to inspect and handle the underlying error.- return nil, fmt.Errorf("failed to scan devices: %v", err) + return nil, fmt.Errorf("failed to scan devices: %w", err)🧰 Tools
🪛 golangci-lint (1.64.8)
23-23: non-wrapping format verb for fmt.Errorf. Use
%wto format errors(errorlint)
🪛 GitHub Actions: lint
[error] 23-23: non-wrapping format verb for fmt.Errorf. Use
%wto format errors (errorlint)
136-150:⚠️ Potential issueUse
errors.Asinstead of type assertion for error checkingType assertions on errors will fail for wrapped errors. Use
errors.Asto check for specific error types, as this properly handles wrapped errors.+import "errors" // ... // If there's an exit error with exit code 4, we ignore the error -if exitErr, ok := err.(*exec.ExitError); ok { +var exitErr *exec.ExitError +if errors.As(err, &exitErr) { switch exitErr.ExitCode() { // ... rest of the code remains the same🧰 Tools
🪛 golangci-lint (1.64.8)
136-136: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors
(errorlint)
🪛 GitHub Actions: lint
[error] 136-136: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
🧹 Nitpick comments (2)
internal/metric/smart_metrics.go (2)
75-129: Consider optimizing the regular expression compilationThe regex at line 113 is compiled each time the function is called. For better performance, consider defining it as a package-level variable that's compiled once.
+// Compile the regex once at package level +var spaceRegex = regexp.MustCompile(`\s+`) func parseSmartctlOutput(output string) (*SmartData, []CustomErr) { // ... // Clean up value by removing extra spaces and brackets - value = regexp.MustCompile(`\s+`).ReplaceAllString(value, " ") + value = spaceRegex.ReplaceAllString(value, " ")
42-73: Consider handling unknown metricsThe
setFieldfunction only handles specifically defined metrics. You might want to consider adding a default case to log or handle unknown metrics for future extensibility.func setField(key, value string, data *SmartData) { switch strings.ToLower(key) { // existing cases... + default: + // Optional: Log unknown metrics or store them in a map of additional metrics + // log.Printf("Unknown SMART metric: %s with value: %s", key, value) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/metric/smart_metrics.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.64.8)
internal/metric/smart_metrics.go
23-23: non-wrapping format verb for fmt.Errorf. Use %w to format errors
(errorlint)
136-136: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors
(errorlint)
🪛 GitHub Actions: lint
internal/metric/smart_metrics.go
[error] 23-23: non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
[error] 136-136: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
🔇 Additional comments (3)
internal/metric/smart_metrics.go (3)
168-175: LGTM! Good error handling for smartctl installation checkThe code properly checks if smartctl is installed before attempting to use it, and returns a clear error message if it's not available.
37-40: Good implementation of case-insensitive error detectionThe
isErrorKeyfunction correctly implements case-insensitive checks for error indicators by converting the key to lowercase before checking, which addresses a previous review comment.
187-198:❓ Verification inconclusive
Review metric aggregation logic
Currently, each device's metrics overwrite the previous device's metrics (line 192). If you intend to aggregate metrics from all devices, you need to adjust this logic to combine metrics instead of overwriting them.
🏁 Script executed:
#!/bin/bash # Check if there are multiple NVMe devices that would be detected by smartctl scan smartctl --scan | grep -c nvmeLength of output: 133
Action Required: Revise Metrics Aggregation Logic and Verify NVMe Presence
- The current loop in
internal/metric/smart_metrics.go(lines 187–198) assigns*metrictometricsfor each device, effectively overwriting previously collected metrics rather than aggregating them.- If the intent is to combine metrics from all devices, you should update this logic (for example, by appending or merging the metrics).
- Note: The
smartctl --scancommand did not produce useful output (possibly due to smartctl not being installed). Please manually verify whether multiple NVMe devices are present in your target environment, as this impacts whether aggregation is necessary.
|
@mertssmnoglu thanks so much for this ticket learned alot, apologies for lot of errors. Will make sure to take care of them next time. Let me know if you have anything else as well. |
I apologize for late(5 days) respond. @Owaiseimdad No problem, thank you so much. Merged 💯 |
Sample out put of the api end point
/api/v1/metrics/smart.Summary by CodeRabbit
smartctlutility.