Add error logging for negative scenarios to improve visibility#97
Add error logging for negative scenarios to improve visibility#97RahulRengeshOfficial wants to merge 1 commit into
Conversation
RahulRengeshOfficial
commented
Mar 23, 2026
RahulRengeshOfficial
commented
- Add log.Error for 500 error paths in recooking_status_handler.go
- Add log.Error for lockdown settings failure in recooking_lockdown_settings_handler.go
- Add log.Error for marshal failures and missing error logging in percentfilter_handler.go
- Add log.Error for marshal failure in ip_mac_ruleconfig_handler.go
- Add log.Error for 500 error paths in recooking_status_handler.go - Add log.Error for lockdown settings failure in recooking_lockdown_settings_handler.go - Add log.Error for marshal failures and missing error logging in percentfilter_handler.go - Add log.Error for marshal failure in ip_mac_ruleconfig_handler.go
There was a problem hiding this comment.
Pull request overview
Adds structured error logging on several 500/error paths across admin API handlers to improve operational visibility when requests fail.
Changes:
- Add
log.Error/log.WithFields(...).Error(...)in XCRP recooking status handlers for DB/marshal failures. - Add error logging when fetching lockdown settings fails during recooking lockdown updates.
- Add missing error logs for marshal failures and other negative paths in percent filter handlers.
- Add error logging for marshal failure in IP/MAC rule config handler.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| adminapi/xcrp/recooking_status_handler.go | Logs Cassandra client/type assertion failures and DB/marshal errors for recooking status endpoints. |
| adminapi/xcrp/recooking_lockdown_settings_handler.go | Logs failure to load lockdown settings in POST handler. |
| adminapi/queries/percentfilter_handler.go | Logs marshal failures and additional negative paths; touches error response strings. |
| adminapi/configuration/ip-macrule/ip_mac_ruleconfig_handler.go | Adds logrus and logs marshal failure for IP/MAC rule config response. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "github.com/rdkcentral/xconfadmin/adminapi/auth" | ||
| "github.com/rdkcentral/xconfadmin/common" | ||
| ccommon "github.com/rdkcentral/xconfadmin/common" | ||
| xhttp "github.com/rdkcentral/xconfadmin/http" | ||
|
|
||
| log "github.com/sirupsen/logrus" | ||
| ) |
There was a problem hiding this comment.
The import block brings in github.com/rdkcentral/xconfadmin/common twice (once as common and once as ccommon). Go does not allow importing the same package path twice; this should be reduced to a single import and the references updated accordingly (e.g., use one package name for both MacIpRuleConfig and IpMacIsConditionLimit).
| globalpercent, err := GetPercentFilterGlobal(applicationType) | ||
| if err != nil { | ||
| log.WithFields(log.Fields{"error": err}).Error("failed to get global percent filter") | ||
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get globalpercent reponse. error: %v", err)) |
There was a problem hiding this comment.
This handler returns an error message containing typos ("reponse"), which will be surfaced to API clients. Please correct the wording (and consider adding a space in "globalpercent" -> "global percent" for readability).
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get globalpercent reponse. error: %v", err)) | |
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get global percent response. error: %v", err)) |
| globalpercent, err := GetPercentFilterGlobal(applicationType) | ||
| if err != nil { | ||
| log.WithFields(log.Fields{"error": err}).Error("failed to get global percent filter") | ||
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get globalpercent reponse. error: %v", err)) | ||
| return |
There was a problem hiding this comment.
GetPercentFilterGlobal currently always returns nil error (it logs and then returns globalPercentage, nil), so this if err != nil branch is effectively dead/unreachable. Either propagate the DB error from GetPercentFilterGlobal so callers can handle it, or remove the error return/branch to avoid misleading control flow.
| globalpercent, err := GetGlobalPercentFilter(applicationType) | ||
| if err != nil { | ||
| log.WithFields(log.Fields{"error": err}).Error("failed to get global percent filter") | ||
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get globalpercent reponse. error: %v", err)) |
There was a problem hiding this comment.
This handler returns an error message containing typos ("reponse"), which will be surfaced to API clients. Please correct the wording (and consider adding a space in "globalpercent" -> "global percent" for readability).
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get globalpercent reponse. error: %v", err)) | |
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get global percent response. error: %v", err)) |
| globalpercent, err := GetGlobalPercentFilter(applicationType) | ||
| if err != nil { | ||
| log.WithFields(log.Fields{"error": err}).Error("failed to get global percent filter") | ||
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, fmt.Sprintf("unable to get globalpercent reponse. error: %v", err)) | ||
| return |
There was a problem hiding this comment.
GetGlobalPercentFilter currently always returns nil error, so this if err != nil branch is effectively dead/unreachable. Either return an actual error from GetGlobalPercentFilter (e.g., when DB calls fail) or simplify the API/handler to remove the unused error path.
| lockdownSettingFromDB, err = lockdown.GetLockdownSettings() | ||
| if err != nil { | ||
| log.WithFields(log.Fields{"error": err}).Error("failed to get lockdown settings") | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) |
There was a problem hiding this comment.
This handler mostly uses xhttp.WriteAdminErrorResponse / xhttp.AdminError for error responses, but this branch uses http.Error, which changes the response format for clients (plain text vs the usual admin error payload). Consider switching this to xhttp.WriteAdminErrorResponse for consistency with the rest of the handler.
| http.Error(w, err.Error(), http.StatusInternalServerError) | |
| xhttp.WriteAdminErrorResponse(w, http.StatusInternalServerError, err.Error()) |
