Improve error logging for 500/ISE scenarios by RahulRengeshOfficial · Pull Request #129 · rdkcentral/xconfwebconfig

RahulRengeshOfficial · 2026-03-12T09:11:59Z

Add Error-level logging in Error() for status >= 500 with audit context
Elevate LogRequestEnds() to Error level for 500+ responses
Log cache refresh/stats failures in data_service_info
Log BSE config lookup and firmware rule evaluation failures
Log firmware config response marshal errors
Log percent filter and firmware config DB lookup failures
Log RFC post-processing and feature control response marshal errors
Log telemetry profile and settings response marshal errors

- Add Error-level logging in Error() for status >= 500 with audit context - Elevate LogRequestEnds() to Error level for 500+ responses - Log cache refresh/stats failures in data_service_info - Log BSE config lookup and firmware rule evaluation failures - Log firmware config response marshal errors - Log percent filter and firmware config DB lookup failures - Log RFC post-processing and feature control response marshal errors - Log telemetry profile and settings response marshal errors

Copilot

Pull request overview

This PR improves observability for internal server error (500/ISE) scenarios by elevating relevant logs to error-level and capturing previously ignored errors (e.g., marshal/DB/config lookups).

Changes:

Emit Error-level logs for requests/responses with HTTP status >= 500 (request end logging + centralized Error() path).
Add error logging for JSON marshal failures and various firmware/config lookup failures.
Add error logging when cache refresh/stats operations fail.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
http/webconfig_server.go	Elevates “request ends” log level to Error for 5xx responses.
http/response.go	Adds centralized Error() logging for 5xx responses using audit context.
dataapi/log_uploader_handler.go	Logs telemetry/settings response marshal failures.
dataapi/feature_control_handler.go	Logs RFC post-processing marshal failures and feature control response marshal failures.
dataapi/estbfirmware/estb_firmware_rule_eval.go	Logs firmware config DB lookup failures during rule evaluation.
dataapi/estbfirmware/estb_evaluation.go	Logs percent filter + firmware config DB lookup failures.
dataapi/estb_firmware_handler.go	Logs BSE config lookup, firmware rule evaluation, and response marshal failures.
dataapi/data_service_info.go	Logs refresh/stats failures for cache info endpoints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-12T09:16:08Z

-			response, _ := util.JSONMarshal(*telemetryProfile)
+			response, err := util.JSONMarshal(*telemetryProfile)
+			if err != nil {
+				log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal telemetry profile: %v", err)


If util.JSONMarshal fails, the handler still responds 200 with a likely empty/invalid body (response may be nil/empty). This can mislead clients and masks a server-side failure. Recommended: on marshal error, return a 500 (or use the shared error responder) and stop processing instead of writing a success response.

Suggested change

log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal telemetry profile: %v", err)

log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal telemetry profile: %v", err)

xhttp.Error(w, http.StatusInternalServerError, common.NotOK)

return

Copilot · 2026-03-12T09:16:09Z

+			}
 			xhttp.WriteXconfResponse(w, 200, response)


Same issue as above: a marshal failure still results in HTTP 200. Please return an error status (e.g., 500) and avoid emitting a successful response when serialization fails.

Suggested change

}

xhttp.WriteXconfResponse(w, 200, response)

xhttp.WriteXconfResponse(w, http.StatusInternalServerError, []byte("internal server error"))

return

}

xhttp.WriteXconfResponse(w, http.StatusOK, response)

Copilot · 2026-03-12T09:16:09Z

-	response, _ := util.XConfJSONMarshal(featureControlMap, true)
+	response, err := util.XConfJSONMarshal(featureControlMap, true)
+	if err != nil {
+		log.WithFields(common.FilterLogFields(fields)).Errorf("GetFeatureControlSettingsHandler failed to marshal feature control response: %v", err)


If XConfJSONMarshal returns an error, the code still responds 200 OK and may send an empty/invalid JSON payload. Suggested fix: on err != nil, return an HTTP 500 response (and stop), since the handler cannot reliably produce a valid success body.

Suggested change

log.WithFields(common.FilterLogFields(fields)).Errorf("GetFeatureControlSettingsHandler failed to marshal feature control response: %v", err)

log.WithFields(common.FilterLogFields(fields)).Errorf("GetFeatureControlSettingsHandler failed to marshal feature control response: %v", err)

xhttp.WriteXconfResponseWithHeaders(w, headers, http.StatusInternalServerError, []byte(`{"error":"internal server error"}`))

return

Copilot · 2026-03-12T09:16:10Z

-		response, _ := util.JSONMarshal(firmwareConfigResponse)
+		response, err := util.JSONMarshal(firmwareConfigResponse)
+		if err != nil {
+			log.WithFields(common.FilterLogFields(fields)).Errorf("GetEstbFirmwareSwuHandler failed to marshal firmware config response: %v", err)


On marshal failure this still returns 200 with a likely empty/invalid body. Please return a 5xx error (and stop) when the response cannot be marshaled.

Suggested change

log.WithFields(common.FilterLogFields(fields)).Errorf("GetEstbFirmwareSwuHandler failed to marshal firmware config response: %v", err)

log.WithFields(common.FilterLogFields(fields)).Errorf("GetEstbFirmwareSwuHandler failed to marshal firmware config response: %v", err)

xhttp.Error(w, http.StatusInternalServerError, common.NotOK)

return

Copilot · 2026-03-12T09:16:10Z

+	evaluationResult, err := estbFirmwareRuleBase.Eval(contextMap, convertedContext, contextMap[common.APPLICATION_TYPE], fields)
+	if err != nil {
+		log.WithFields(common.FilterLogFields(fields)).Errorf("GetFirmwareResponse firmware rule evaluation failed: %v", err)
+	}
 	explanation := GetExplanation(contextMap, evaluationResult)
 	if evaluationResult == nil || evaluationResult.Blocked || evaluationResult.FirmwareConfig == nil || evaluationResult.FirmwareConfig.Properties == nil {
 		return http.StatusNotFound, []byte(fmt.Sprintf("\"<h2>404 NOT FOUND</h2><div>%s<div>\"", explanation)), evaluationResult, convertedContext, explanation, contextMap


When Eval(...) returns an error, the function may still return 404 Not Found (if evaluationResult is nil/invalid), conflating internal evaluation failures with genuine 'not found' cases. If err != nil, consider returning 500 Internal Server Error (and an appropriate error body) to accurately reflect an evaluation failure.

Copilot · 2026-03-12T09:16:10Z

 	"github.com/rdkcentral/xconfwebconfig/shared"
 	coreef "github.com/rdkcentral/xconfwebconfig/shared/estbfirmware"
 	"github.com/rdkcentral/xconfwebconfig/shared/firmware"
+


The import block formatting looks like it may not be gofmt-ed (extra blank line before the logrus import within the same group). Please run gofmt (or goimports) so imports are grouped/ordered consistently and CI styling checks (if any) don’t fail.

Suggested change

gravi21 · 2026-03-16T09:38:59Z

 		w.WriteHeader(status)
 	default:
+		if status >= http.StatusInternalServerError {
+			LogError(w, err)


@RahulRengeshOfficial Let's check double check code once if this is needed since we import xconfwebconfig into multiple repo's

gravi21 · 2026-03-16T09:39:19Z

 	tfields := common.FilterLogFields(fields)

-	log.WithFields(tfields).Info("request ends")
+	if statusCode >= http.StatusInternalServerError {


@RahulRengeshOfficial Let's check double check this code once if this is needed since we import xconfwebconfig into multiple repo's

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T17:32:08Z

-			response, _ := util.JSONMarshal(settingsResponse)
+			response, err := util.JSONMarshal(settingsResponse)
+			if err != nil {
+				log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal settings response: %v", err)


If marshaling the settings response fails, the handler still returns HTTP 200 with a potentially empty/invalid response body. Consider returning a 500 (and exiting) when util.JSONMarshal fails so clients don’t receive a successful status with an invalid payload.

Suggested change

log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal settings response: %v", err)

log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal settings response: %v", err)

xhttp.WriteXconfResponseAsText(w, http.StatusInternalServerError, []byte("\"Internal Server Error\""))

return

Copilot · 2026-03-23T17:32:08Z

+	filterValue, err := coreef.GetDefaultPercentFilterValueOneDB()
+	if err != nil {
+		log.Errorf("PercentFilterfilter failed to get default percent filter value: %v", err)
+	}


GetDefaultPercentFilterValueOneDB returns (nil, err) on DB failures. When that happens, filterValue is nil here, but the code immediately dereferences filterValue.EnvModelPercentages, filterValue.Whitelist, etc., which will panic. Add a nil guard (e.g., return false / treat as 0% / default behavior) when filterValue == nil after the call fails.

Suggested change

}

}

if filterValue == nil {

// On DB failure or missing default filter value, avoid panicking and

// fall back to allowing firmware output (no percent-based blocking).

return true

}

Copilot · 2026-03-23T17:32:09Z

+	bseConfiguration, err := estbFirmwareRuleBase.GetBseConfiguration(ip)
+	if err != nil {
+		log.Errorf("GetEstbFirmwareSwuBseHandler failed to get BSE configuration: %v", err)
+	}
 	if bseConfiguration == nil {
 		xhttp.WriteXconfResponseAsText(w, 404, []byte("\"<h2>404 NOT FOUND</h2>\""))
 		return


If GetBseConfiguration returns an error (e.g., DB failure), this logs the error but then returns a 404 when bseConfiguration is nil. That conflates internal failures with ‘not found’. Consider returning 500 on err != nil (and only returning 404 when err == nil and the config is truly absent).

Copilot AI review requested due to automatic review settings March 12, 2026 09:12

Copilot started reviewing on behalf of RahulRengeshOfficial March 12, 2026 09:12 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

bhushan-mdn previously approved these changes Mar 12, 2026

View reviewed changes

gravi21 reviewed Mar 16, 2026

View reviewed changes

Update response.go

7e5ef32

RahulRengeshOfficial dismissed bhushan-mdn’s stale review via 7e5ef32 March 23, 2026 17:24

Update webconfig_server.go

be4f26f

Copilot AI review requested due to automatic review settings March 23, 2026 17:25

Copilot started reviewing on behalf of RahulRengeshOfficial March 23, 2026 17:25 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

gravi21 approved these changes Mar 24, 2026

View reviewed changes

-			}
-			xhttp.WriteXconfResponse(w, 200, response)
+				xhttp.WriteXconfResponse(w, http.StatusInternalServerError, []byte("internal server error"))
+				return
+			}
+			xhttp.WriteXconfResponse(w, http.StatusOK, response)

-	}
+	}
+	if filterValue == nil {
+		// On DB failure or missing default filter value, avoid panicking and
+		// fall back to allowing firmware output (no percent-based blocking).
+		return true
+	}

Conversation

RahulRengeshOfficial commented Mar 12, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

gravi21 Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gravi21 Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gravi21 Mar 16, 2026 •

edited

Loading