stop logging errors for api error responses

[lieut-data]

Summary

From a cursory glance, when a real error occurs, we already log something at the application layer, so there's no reason to echo every api error response as an error log too. Errors should be actionable, but these logs were not, e.g.:

access denied to templates

or:

category ID specified in input does not exist for user

This PR duplicates mattermost-community/focalboard#5022.

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-59325

[marianunez]

From a cursory glance, when a real error occurs, we already log something at the application layer,

@lieut-data can you point to where this is happening? For the two examples provided, I don't see where the error is logged in the application layer. Worried that we may be missing seeing actual errors in the log with changing for all that call errorResponse

[lieut-data]

@lieut-data can you point to where this is happening? For the two examples provided, I don't see where the error is logged in the application layer. Worried that we may be missing seeing actual errors in the log with changing for all that call errorResponse

Oops, I didn't meant to frame it as such, @marianunez: the two examples are "non-actionable" errors, and indeed there are no additional error logs expected from the application layer. With this change, we'll still log a warning when an API returns a failure code to the client, but the default position is to assume these aren't actionable (i.e. permission issues).

There are examples of actionable errors in the app layer, e.g.:

mattermost-plugin-boards/server/app/initialize.go

Lines 10 to 16 in 0b6e443

	func (a *App) initialize(skipTemplateInit bool) {
	if !skipTemplateInit {
	if err := a.InitTemplates(); err != nil {
	a.logger.Error(`InitializeTemplates failed`, mlog.Err(err))
	}
	}
	}

but I note they are not consistent. There are certainly opportunities for greater consistency here.

[marianunez]

the two examples are "non-actionable" errors, and indeed there are no additional error logs expected from the application layer. With this change, we'll still log a warning when an API returns a failure code to the client, but the default position is to assume these aren't actionable (i.e. permission issues).

I see, makes sense. Thanks for clarifying

[marianunez]

This explains the double log entries for each error

[Rajat-Dabade]

LGTM 🚀